Possible bug in LLVM/clang

[newbrain]

Perhaps my brains are old and scrambled.

I was getting crazy to find a bug in my code, but then I discovered that the problem actually lied in the test code!

Here is a minimal piece of code that shows what I suspect is a clang bug:

Code: [Select]

typedef struct {
    unsigned short int flag;
    unsigned char counter;
} Block;

static Block * const block = &(Block){
    .flag = 0,
    .counter = 0,
};

extern void Write(Block *b);

void f(void) {
    block->counter = 252;
    for (int i = 0; i < 10; i++) {
        /* Corrupt element 255 */
        if (block->counter == 255u)
            block->flag = 0x1234;
        else
            block->flag = 0x4321;

        Write(block);
        block->counter++;
    }
}

What I expected to see:
I expected the flag member in *block to take the value 0x1234 when the counter member was equal to 255.
This is, in fact, what happens with gcc.
Optimization have been left to -O0, but the result is the same at any other level.

What I actually see:
The flag member never takes the value 0x1234.
The check for block->counter == 255u is completely missing.
Optimization levels do not affect the result.

Note that I used a compound literal with a const pointer (not, of course, a pointer to const!).
If, instead, a Block structure is declared and initialized separately and a const pointer declared and initialized to its address, the code works as expected.

C11 standard states quite clearly that compound literals are (unless const-qualified) modifiable lvalues:

6.5.2.5 Compound literals
...
A postfix expression that consists of a parenthesized type name followed by a brace-enclosed list of initializers is a compound literal. It provides an unnamed object whose value is given by the initializer list.
...
the result is an lvalue.
...
EXAMPLE 5 The following three expressions have different meanings:

Code: [Select]

"/tmp/fileXXXXXX"
(char []){"/tmp/fileXXXXXX"}
(const char []){"/tmp/fileXXXXXX"}
The first always has static storage duration and has type array of char, but need not be modifiable; the last two have automatic storage duration when they occur within the body of a function, and the first of these two is modifiable.

I'm using the latest release of clang, 13.0.1, on Windows.

I invoke the collective knowledge here to avoid filing an unmotivated issue.
Am I overlooking something silly?

[DiTBho]

can you try without "const"?

[SiliconWizard]

It's a pretty odd use of literals. I would have never thought of doing that.

What's wrong with declaring a struct variable instead, initialized the same way, and taking a pointer to it later on?

The part you quoted said compund literals would be modifiable if with auto storage, but in your case, it's a global variable, so it's not auto storage?

[SiliconWizard]

can you try without "const"?

The const as it is in the OP's code is just about the pointer itself, and not the pointed to data. So it wouldn't make a difference, unless there was a very serious bug in the compiler not related to compound literals.

[ataradov]

Removing const does help and generated the same code as declaring a separate variable and taking a pointer to that. Looks like a bug to me. Or some obscure UB that C like that much.

This is indeed a very confusing code to read.

[SiliconWizard]

Really? So it's possibly a bug, but the way I read the standard, as the OP is using a compound literal outside of a function, it is not supposed to be modifiable anyway. Or did I miss something? (Again that looks so odd to me that I would have never thought of doing that.)

[ataradov]

Slightly modifying the code:

Code: [Select]

int  f(void) {
    block->counter = 252;
    for (int i = 0; i < 10; i++) {
        /* Corrupt element 255 */
        if (block->counter == 255u)
            block->flag = 0x1234;
        else
            block->flag = 0x4321;

        Write(block);
        block->counter++;
    }
 return block->counter;
}
and assigning 123 to the counter in the initialization just calls write and returns 123.  So, it is sure that this code can't modify the data and traces the value 123 to the end of the function.

Yet it generates code for "block->flag = 0x4321;" and "block->counter++;" But it still ignores the face that block->counter has changed and returns 123. So, something is busted.

[DiTBho]

unless there was a very serious bug in the compiler not related to compound literals.

That is precisely what I suspect

[ataradov]

Ok, so it thinks the value of the block->counter is the one that was declared originally. So, it removes the condition (since it thinks the value is fixed and known) and it also returns a constant for the same reason.

If you set counter = 255 initially, it will use "block->flag = 0x1234;" instead of "block->flag = 0x4321;"

[SiliconWizard]

That sounds compliant with the std to me?
But is block->flag modified after that?

[ataradov]

The code for "block->counter = 252;" was not generated at all. EDIT: The code for "block->counter = 252;" is  generated. The code for "block->counter++;" was generated, but the rest of the code is generated as if block->counter is constant (as at the time of declaration). The flag is assigned correctly, given the previous issue.

So, it generates all the code necessary for assignments to block->counter, yet its optimizer does not think it assigns to that variable.

I'm not sure this is correct behaviour. It makes no sense.

This is broken in v4.0.0. It generated correct code in all version prior to that.

[ataradov]

Much smaller repro:

Code: [Select]

typedef struct {
    int counter;
} Block;

static Block * const block = &(Block){ 255 };

int  f(void) {
   block->counter = 252;
   return block->counter;
}
This code returns 255.

Changing the code to int instead of Block does generate correct code, so it has something to do with the way structures are handled.

[SiliconWizard]

OK... =)

As I said, I would frankly never have thought of doing this. Nevertheless, a compiler should not just optimize it out (and at least do it right and not just partially): it should also give proper warning, or even throw an error.

[ataradov]

But there is no error here.  The code is correct as far as I can see. The compiler is wrong here.

[SiliconWizard]

But there is no error here.  The code is correct as far as I can see. The compiler is wrong here.

As I said earlier, from what I read in the standard (admittedly quiclkly), the compound literal should be non-modifiable in the context it was in. So Attempting to modify it should be an error.
Any other compiler behavior being of course a bug.

Now do not hesitate to correct me about my interpretation of the standard.

[ataradov]

I don't see anything like that in the standard. Furthermore there is an example that is similar to this case

EXAMPLE 1
The file scope definition
int *p = (int []){2, 4};
initializes p to point to the first element of an array of two ints, the first having the value two and the second, four. The expressions in this compound literal are required to be constant. The unnamed object
has static storage duration.

The elements of the compound literal must be constants (they are in this case), but the object itself is just a static object if it was used outside of the function scope and automatic object if it was used inside the function scope.

Although with arrays it actually works.

[brucehoult]

But there is no error here.  The code is correct as far as I can see. The compiler is wrong here.

As I said earlier, from what I read in the standard (admittedly quiclkly), the compound literal should be non-modifiable in the context it was in. So Attempting to modify it should be an error.
Any other compiler behavior being of course a bug.

Now do not hesitate to correct me about my interpretation of the standard.

I believe attempting to modify a constant is not something the compiler is expected to catch, but rather an error on the part of the programmer, and results in UB (Undefined Behaviour).

Apple's system compiler (which is available as "gcc" but is in fact "Apple clang version 12.0.0 (clang-1200.0.32.29)") does the same thing.

Annoyingly, with a loop trip count of 10 it unrolls the whole thing. So I changed it to 1000.

It (unconditionally) stores the #17185 (0x4321) every time around the loop. Then calls Write(). Then increments .counter in memory.

Removing the "const" produces the expected behaviour.


I think the bug, if any, in LLVM is failing to optimise out "block->counter = 252", the stores to block->flag and the increment of block->counter.

[ataradov]

There is a constant pointer here, but it points to the static variable (anonymous) that is initialized with constant values. There are no mistakes in the code, it does not modify any constants. This is compiler's fault.

If you move 'const' to make the pointed value to be constant, it will generate an error.

Clang's optimizer propagates initialization value and fails to notice that the value is changed by the code.

[AntiProtonBoy]

Code: [Select]

static Block * const block = &(Block){
    .flag = 0,
    .counter = 0,
};

In the example above, which valid part of memory is pointer block addressing?

What you're doing there is appears to be undefined behaviour. You are taking an address of a temporary object. The pointer to the struct becomes an invalid address soon after the assignment operator, because the struct instance was stored in some implementation defined temporary memory which is immediately "freed".

Try this instead:

Code: [Select]


Block b = {
    .flag = 0,
    .counter = 0,
};

static Block * const block = &b;


[ataradov]

In the example above, which valid part of memory is pointer block addressing?

Compound literal creates an object. Here is what the standard says about this:

The value of the compound literal is that of an unnamed object initialized by the initializer list. If the compound literal occurs outside the body of a function, the object has static storage duration; otherwise, it has automatic storage duration associated with the enclosing block.

In this case the literal is used outside of the function, so the unnamed object is created as a static value.

The code is correct, but given how much confusion it creates, it is better to not do that indeed. Still, this is a compiler bug, and should be reported.

And if you look in the generated code, you can clearly see that compiler correctly allocates static memory for this object. And it correctly performs the assignments to that memory. It just fails to detect any assignments to the fields that it performs and propagates the initial value to the places where it is used.

[newbrain]

Thanks everyone for the comments!

First, a side point:

It's a pretty odd use of literals. I would have never thought of doing that.

I see that many, as SiliconWizard, did not like the style.
No problem with that, it's to a large extent personal - to me it's more readable than having a uselessly declared variable, I make extensive use of compound literal when a variable name is not needed, mostly in two cases: as (struct *, usually) arguments to a function and the one we are discussing.

About the modifiability of lvalues (brucehoult, AntiProtonBoy, SiliconWizard): non modifiable lvalues in C are a specific subset, and outside of its constraints lvalues are in general modifiable.

6.3.2.1 Lvalues, arrays, and function designators
...
A modifiable lvalue is an lvalue that does not have array type, does not have an incomplete type, does not have a const-qualified type, and if it is a structure or union, does not have any member (including, recursively, any member or element of all contained aggregates or unions) with a const-qualified type.

In my case the unnamed static object is not an array type (a moot point, its elements would still be modifiable, as in the example the standard provides), it is not const-qualified, and it does not have any const-qualified member, so it is by default modifiable.
C++ is, as usual, much more complicated, but lacks compound literals - so I cannot directly compare the standards (though clang implements them as an extension, see the issue mentioned at the end).

attempting to modify a constant

This not what I'm doing. I'm trying to modify a non const-qualified static object, albeit an unnamed one, through a const-qualified pointer to a non-const-qualified type.
For static duration objects the initializers must be constant expression or string literals, but of course this does not prevent us to do:
static int i=42;
void f(void)
{
    i = 0;
}

You are taking an address of a temporary object. The pointer to the struct becomes an invalid address soon after the assignment operator, because the struct instance was stored in some implementation defined temporary memory which is immediately "freed".

The unnamed object has static storage duration, it is not temporary, as per "6.5.2.5 Compound literals", §5:

If the compound literal occurs outside the body of a function, the object has static storage duration;

I'll open an issue on LLVM GitHub repo - let's see what they think.
I've searched past issues with no luck, with the exception of this, which might be somehow relevant.
Ataradov, I'll use your  minimal example, thanks again.

EtA: The issue can be found here

[brucehoult]

attempting to modify a constant

This not what I'm doing. I'm trying to modify a non const-qualified static object, albeit an unnamed one, through a const-qualified pointer to a non-const-qualified type.

Ahh, yup, right.

[DiTBho]

My IDE and ICE were also confused by that code.
Frankly, it's not a good idea to write things this way.

[DiTBho]

I don't use LLVM/C-89/99 but rather LLVM/C-dialect, based on CLANG but with a slightly modified C-grammar.

Anyway, these samples can be easily converted into standard C-89

Code: [Select]

private block_t         block =
{
    .flag    = 0,
    .counter = 0,
};
private p_block_t       p_block = get_address(block);
This works perfectly, both my ICE and IDE are not confused by symbols, their addresses and their meaning.

While these two make them confused:

Code: [Select]

private p_block_t p_block = get_address(block_t)
{
    .flag    = 0,
    .counter = 0,
};
Here the IDE doesn't correctly list p block in the list of pointers, the ICE is confused and doesn't track the pointer' context, but at least this code works as expected.

Code: [Select]

private p_block_t const p_block = get_address(block_t)
{
    .flag    = 0,
    .counter = 0,
};
Instead here the IDE doesn't correctly list p block in the list of pointers, the ICE is confused and can't figure out what it needs to track, and the code doesn't work as expected.

Get_address (block_t) is syntactically correct, but confuses the tools, while Get_address (block) is syntactically correct, and doesn't confuse the tools.

• Get_address (block_t) -> "block_t" is a typedef, so there is no entry in the map file
• Get_address (block) -> "block" is a variable, so there is also an entry in the map file, telling its address in ram

[Siwastaja]

My IDE and ICE were also confused by that code.

That is no reason to avoid writing valid code. IDEs are notoriusly buggy in parsing / recognizing code. You just have to live with that.

Now I can accept that compiler bug is a much more valid reason to avoid some constructs. You can ignore IDE, but you can't ignore compiler.

Frankly, it's not a good idea to write things this way.

Seems like a very good idea to me. A simple, readable construct. I didn't know this is possible, but understood immediately what the code means, what it does, and why it is written like it is.

But "new" constructs are always prone to compiler bugs. They will get sorted out.

[newbrain]

Frankly, it's not a good idea to write things this way.

Frankly, I think instead that code says exactly what is meant.

The construct is legal and used for the precise purpose it was introduced, i.e. to provide an (initialized) object without a name.

Apart from compiler bugs (pending of course LLVM project's acknowledgement), why is that not a good idea?
The CL syntax is clear enough and does reduce clutter, I do not consider it obscure in any way - after all, CLs have been with us for more than 20 years.

...
Anyway, these samples can be easily converted into standard C-89
...

To have CLs at least C99 needs to be considered.
I really do not understand the connection: does that dialect (your invention?) support compound literals?
If so, how are they related to the C ones?
It looks more like a pared down version of C++ (which does not have CLs) but:
Is get_address() a constexpr function in the C++ meaning (so it can be used as an initializer) or a keyword/operator (like &)?
Are the various types defined somewhere or implicitly derived?

[DiTBho]

That is no reason to avoid writing valid code. IDEs are notoriusly buggy in parsing / recognizing code. You just have to live with that.
Now I can accept that compiler bug is a much more valid reason to avoid some constructs. You can ignore IDE, but you can't ignore compiler.

If your colleagues are confused, well ... and if your IDE and ICE are confused, too, well that's is a serious problem when you have to debug and analyze things.
For example, here both the dynamic coverage (performed by the ICE) and the static coverage (performed by the IDE) don't work correctly.

[DiTBho]

I really do not understand the connection: does that dialect (your invention?) support compound literals?

Yes. I haven't touched the Clang/C99 internal structure, only a piece of the grammar before the AST, this in order to force people to write clean code, because I am frankly tired to waste my unpaid time refactoring other people's code inside my team.

get_address()

I hacked Clang, and I introduced something that works similarly to sizeof(), but you can translate it with the common "&" operator. There are a variety of reasons for this, including that the trick is also massively used to force the ICE to look at the map file and to pre-load the variable address, so when you debug something you already have things preloaded and ready to be inspected. It saves a lot of time during a debug session, and it also help during the dynamic coverage.
 

Are the various types defined somewhere or implicitly derived?

"p_" automatically defines a typedef pointer, e.g. "p_uint32_t" is equivalent to "typedef uint32_t* p_uint32_t;".
Nothing special, but it saves time, and you can forget to think about it since it automatically generates these things in an header file for you.

Anyway, here *the point* is that even if I translate your source into pure C99, both the unpatched-IDE and ICE are confused, as well as the most of my colleagues.

That's not good for me.

[Siwastaja]

If your colleagues are confused, well ...

... discuss usage of new language features before using them, so that everyone is familiar with them.

Unless your communication is broken, of course, in which case your project is doomed.

and if your IDE and ICE are confused, too,

If the IDE and ICE claim to understand that standard, then well what can you do? Work around their bugs? At least issue tickets in their bug trackers.

If they honestly do not support that standard, then well, that's a good reason to adhere to older standards. C89 is fine. It just results in more work and possibly worse code.

But you are just so utterly wrong in saying that it is not "good idea" to write modern, normal, standard-compliant code, because you happen to have an old or broken tool that does not support modern, standard-compliant C code.

C is a slowly moving target anyway, and the new standards have mostly evolved for the better. Generally, using the new C standards is good advice; your advice of doing the exact opposite may work in certain niche, like your not-invented-here "#define _IF_ if" style language, but again not as a generic advice.

"p_" automatically defines a typedef pointer, e.g. "p_uint32_t" is equivalent to "typedef uint32_t* p_uint32_t;".
Nothing special, but it saves time, and you can forget to think about it since it automatically generates these things in an header file for you.

Anyway, here *the point* is that even if I translate your source into pure C99, both the unpatched-IDE and ICE are confused, as well as the most of my colleagues.

A very sure way of getting colleagues confused is reinventing standard language constructs like pointers and replace them with your NIH syntax (* -> p_, etc.). The fact that C is powerful enough to come with typedef and define metaprogramming, does not mean you actually have to do this. What a horrible mess! I'm so glad I don't have to work with your code.

But I guess, if your colleagues are using your custom syntax replacing the most basic * and & operators, then showing them any standard C language construct is pretty mind-blowing.

[DiTBho]

A very sure way of getting colleagues confused is reinventing standard language constructs like pointers and replace them with your NIH syntax. What a horrible mess! I'm so glad I don't have to work with your code.

Indeed, it was so "clear" that even people in this topic are confused, and I was the first in this topic telling to "remove" the "const" declaration 

[DiTBho]

The fact that C is powerful enough to come with typedef and define metaprogramming, does not mean you actually have to do this. What a horrible mess! I'm so glad I don't have to work with your code.

Your opinion, facts are "my code" is back-compatible due to an automatic generator, and the final code usually passes the MISRA-checkers at the first try, and it's usually also well understood by the ICE.

Do you know what it means? Less effort, more good results.

[Siwastaja]

So what you have is a highly sophisticated, partially custom tooling which makes the language you write not actual C, but more like another language based on C. One with more error checking, better visibility, and so on. Is it worth it?Maybe it is. Is it more reliable, better, safer? Maybe it saves development time? Maybe it's the best thing since sliced bread?

I don't deny any of that. I only question your recommendations of avoiding normal, modern, standard C language only because it does not work in your workflow. Your commentary is more about bragging about your own special unique snowflake (which, to be fair, might be really good), than giving useful or correct advice to others, who just discuss standard C and standard, up-to-date tools that claim full compatibility with the modern standard.

Not interested in dick length comparison. I just use standard C, and I don't have any of that fancy tooling you have. We work fundamentally differently. To me, MISRA means bureaucracy which compromises code quality. To you, it probably means the opposite. That is fine, we can disagree.

But the fact is, no one here except you has your tooling in use.

[Nominal Animal]

Why not use the much more common pattern,

Code: [Select]

static Block block[] = {
    { .flag = 0, .counter = 0, },
};
instead?  No other changes needed.

[newbrain]

If your colleagues are confused, well ...

...then I'd like to know their opinion on  _Complex or _Alignas and, god forbid, _Static_assert or _Generic.

C is not evolving blindingly fast (C++, OTOH did make some giant leaps ten years ago and modern C++ is a completely different beast from the one I learned)
All of the above are features that have been there for ten years, compound literals and designated initializers are old enough to drive, drink and vote almost everywhere (not all at the same time, hopefully ).

They make programs more expressive while not betraying the C "philosophy" and being evolutionary rather than revolutionary.
Some, like _Generic, make it possible to express new concepts or old concetps that required non standard constructs.
Others, like _Static_assert, can make your program safer.
Some others, like _Atomic, do both.

Though I'll admit I have some antipathy for VLAs, and never use them - but I mostly program for small MCUs, so I think it's justified.

I won't comment on the IDE. VS Code (rather, the C/C++ extension) has no problems with these constructs, but I remember getting squiggles when it was in its infancy for designated initializers. Visual Studio understands them no problems.

Resorting to a training wheels language instead of honing skills might work, but does not seem a viable long term strategy.

EtA: a little example of what I'm saying. Look at this (heavily abridged*) code from NXP SDK, and the hoops it needs to jump through for not being able to use _ALignas (I think they go for C99 compliance, which is a reasonable goal for a generic SDK):

Code: [Select]

struct _lpspi_master_edma_handle
{
...
    edma_tcd_t lpspiSoftwareTCD[3]; /*!<SoftwareTCD, internal used*/
};

status_t LPSPI_MasterTransferEDMA(LPSPI_Type *base, lpspi_master_edma_handle_t *handle, lpspi_transfer_t *transfer)
{
    ...
    edma_tcd_t *softwareTCD_extraBytes    = (edma_tcd_t *)((uint32_t)(&handle->lpspiSoftwareTCD[1]) & (~0x1FU));
    edma_tcd_t *softwareTCD_pcsContinuous = (edma_tcd_t *)((uint32_t)(&handle->lpspiSoftwareTCD[2]) & (~0x1FU));
   ...
}
Why this opprobrium?
Because Transfer Control Descriptors for the iMX.RT DMA must be 32 byte aligned.
Slightly memory wasteful, non portable, non standard and UB as a cherry on top.

*ST: "My HAL is bloated"
NXP: "Hold my beer"

[newbrain]

Code: [Select]

static Block block[] = {
    { .flag = 0, .counter = 0, },
};


This is nice, thanks for the suggestion, I admit I never used this pattern, and did not think of it!

As good as it is, it's still a slightly obfuscated way to convey the meaning.
Constness is replaced with not being an lvalue, and the array is not used as a collection but as a single element.
It is not possible to use in all cases but it's perfect for many.

In any case, compliant code should work.

[Nominal Animal]

Constness is replaced with not being an lvalue, and the array is not used as a collection but as a single element.
It is not possible to use in all cases but it's perfect for many.

True.  I did not mean my question (Why not use...) as a rhetorical one or as a suggestion; I was genuinely wondering why it wasn't used in this case.  I see it constantly in Linux systems programming, you see.

A variant of this – that works on systems that use ELF binaries, including many microcontroller development environments – is to make sure the size of Block is a multiple of its alignment (as it is for all base types), and then use __attribute__((used, section ("name")) to gather all entries (in a pseudorandom order) into a single array, even if compiled in separate compilation units or dynamically linked at run time.  The first element is the address of the __start_name symbol, and the lowest address beyond its end is the address of the __stop_name symbol (whose type does not matter).  See e.g. here for an example I posted some time ago.

[SiliconWizard]

Thanks everyone for the comments!

First, a side point:

It's a pretty odd use of literals. I would have never thought of doing that.

I see that many, as SiliconWizard, did not like the style.
No problem with that, it's to a large extent personal - to me it's more readable than having a uselessly declared variable

It's not that I don't like the style per se - I can understand your point, considering it more elegant.
It's just that, compound or not, I personally think that literals should never be modifiable objects. That's a general consideration from a language standpoint, so I think the C std made a mess here. They probably did for the same reason as you use that: because it can be elegant and useful, but IMHO it introduces an inconsistency that can only confuse both people using C and people writing compilers for C. For which we have evidence here. =)

As to the literal being modifiable in the context you used it here - I re-read the std, and am still not fully convinced, but you have convinced me that the opposite was also not a given, so I consider it being clear as mud at this point.

Now whether it should be modifiable or not, in both cases, Clang's behavior is bogus. So this is clearly a bug.

[DiTBho]

I don't deny any of that. I only question your recommendations of avoiding normal, modern, standard C language only because it does not work in your workflow.

No, I don't recommend it because it makes tools and people confused and it happens even with the standard C99.