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.