Possible bug in xtensa-esp32-elf-g++ (crosstool-NG esp-2021r2-patch5) 8.4.0

[calint]

I have a case where I do a "new in place" of an object with one uninitialized field.

That field is set before the memory is used in "new in place" operation.

When the function that allocates the memory for the object and initializes that field gets in-lined, the compiler chooses to skip that store falsely assuming that it will be overwritten by the constructor (?)

The solution is compiler flag "-flifetime-dse=1".

Am I doing UB or is it a bug?

Kind regards

[igrr]

@calint I would recommend creating a small reproducer for this issue and uploading it to the Compiler Explorer (godbolt.org). There you can switch between different compilers and architectures and see if this behavior is unique to GCC 8.4 or not.

[calint]

I cannot reproduce it in godbolt. In the actual code it does optimize away a store. I can produce generated assembler with source annotations.

Boiled down to a description in code is:

#include <cstdio>
#include <cstdlib>
#include <iostream>

class object {
   public:
    object **alloc_ptr;
    int x = 1;
    int y = 2;
};

inline __attribute__((always_inline)) object *alloc() {
    object *mem = (object *)malloc(sizeof(object));
    mem->alloc_ptr = (object **)(0xf00ba7); // this store gets optimized away
    return mem;
}

int main() {
    object *o = new (alloc()) object;
    printf("%p\n", o->alloc_ptr); // UB
}

I am probably doing UB... It is solved with the compiler flag.

Thanks

[Lapshin]

@calint , seems it is a common issue for GCC with -O2 optimization. You could workaround this by adding a volatile keyword on your mem variable. (See https://godbolt.org/z/afjo67KMf)

[calint]

Knowing about the optimization then the flag makes sense:

-flifetime-dse=1: Enables DSE but preserves stores before the constructor starts. This means that any memory initialization (e.g., zeroing out memory) done before the constructor is preserved, but stores after the destructor are treated as dead.

[Lapshin]

@calint , maybe it works in your particular case. But seems not in case that you posted here - see https://godbolt.org/z/8qcnWcfaP

[calint]

Hmm. I tried it and it prints the correct value with and without the flag.

Am I missing something?

I tried to reproduce the issue, but could not in godbolt.

[Lapshin]

@calint , if -O2 optimization is enabled then your code:

#include <cstdio>
#include <cstdlib>
#include <iostream>

class object {
   public:
    object **alloc_ptr;
    int x = 1;
    int y = 2;
};

inline __attribute__((always_inline)) object *alloc() {
    object *mem = (object *)malloc(sizeof(object));
    mem->alloc_ptr = (object **)(0xf00ba7); // this store gets optimized away
    return mem;
}

int main() {
    object *o = new (alloc()) object;
    printf("%p\n", o->alloc_ptr); // UB
}

Will be optimized to something like this:

#include <cstdio>
#include <cstdlib>
#include <iostream>

int main() {
    printf("%p\n", 0xf00ba7);
}

So, if you want to allocate some memory in your code to use it, you should tell the compiler to not optimize it. For example, using the volatile keyword

[calint]

Of course :)

Now I remember why I could not reproduce the issue. Could not create the complexity for the compiler to not optimize it to nothing.

The compiler flag does do exactly what I want to avoid: the compiler to assume that a constructor would overwrite all fields.

Declaring it volatile would inhibit other optimizations also. No?

Other thoughts are that it happens when it gets in-lined. The compiler should see 'oh, it is a new in place' don't assume anything about the memory. Further more it gives a warning about the uninitialized member field so that would be another clue that the member field might be initialized before the constructor.

Anyway, it took a reading of all the optimization options in man page and as soon as I saw that flag I was confident the issue would be gone.

Kind regards

[Lapshin]

@calint , it seems your approach with -flifetime-dse=1 makes sense in c++ optimizations perspective then.

Declaring it volatile would inhibit other optimizations also. No?

It depends on the optimization type but it looks like -flifetime-dse=1 is still required.

I'm closing the issue since you found the solution. Please feel free to open issues in case you found something!