Memory load reordering optimization (-O2) bug

• Informador: nik-ponomarev
• Propietario: (Ninguno)
• Tipo: Issues
• Estado: Cerrado
• Componente: OTHER
• Hito: (Ninguno)
• Prioridad: 5 - Medium
• Gravedad: 5 - Medium
• Resolución: Invalid
• Fichero: Ninguno

Details

Description

The bug is related to some internal code structure, so tried to reproduce the situation as close as possible. mingw with -O2 optimization flag loads memset-ed value on the first load of atomic (just struct name) value read and initialized value (right value) at the second load call.

Fixes:
* Removing volatile keyword
* Changing int32_t to unsigned long
* Other reduction of code complexity

What is wrong with this code? Is it a bug? GCC7 does not have this issue.

Reproducer

 #include <new>
 #include <iostream>
 #include <cstring>
 
 struct atomic {
     unsigned long my_value;
 public:
     atomic() = default ;
     atomic(unsigned long value) : my_value(value) {}
     unsigned long load() {
         int32_t* v = reinterpret_cast<int32_t*>(&my_value);
         return (volatile int32_t&)(*v);
     }
 };
 
 //--------------------------------------------------------------------------------------------------------
 
 struct TestStruct {
     atomic storage;
     
     TestStruct(){
         memset(static_cast<void*>(&storage),-1,sizeof(storage));
     }
 
     void operator()(unsigned long i){
         atomic* ptr2 = new (&storage) atomic(i);
         unsigned long val1 = ptr2->load();
         std::cout << "Expected:" << i << " Value first: " << val1 << std::endl;
         unsigned long val2 = ptr2->load();
         std::cout << "Expected:" << i << " Value second: " << val2 << std::endl;
     }
 };
 
 int main () {
     TestStruct()(0L);
     //just long also should fail;
     return 0;
 }

Output:

Expected:0 Value first: 4294967295
Expected:0 Value second: 0

Environment

Windows Server 2016

gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=d:/mingwcompilers/mingw_15.4_gcc_7.3.0/bin/../libexec/gcc/x86_64-w64-mingw32/7.3.0/lto-wrapper.exe
Target: x86_64-w64-mingw32
Configured with: ../src/configure --enable-languages=c,c++ --build=x86_64-w64-mingw32 --host=x86_64-w64-mingw32 --target=x86_64-w64-mingw32 --disable-multilib --prefix=/c/temp/gcc/dest --with-sysroot=/c/temp/gcc/dest --disable-libstdcxx-pch --disable-libstdcxx-verbose --disable-nls --disable-shared --disable-win32-registry --with-tune=haswell --enable-threads=posix --enable-libgomp
Thread model: posix
gcc version 7.3.0 (GCC)

ld -v
GNU ld (GNU Binutils) 2.29.1

Ticket History (3/8 Histories)

2018-07-26 22:39 Updated by: nik-ponomarev

• New Ticket "Memory load reordering optimization (-O2) bug" created

2018-07-27 03:16 Updated by: earnie

• Componente Update from (Ninguno) to OTHER
• Resolución Update from Ninguno to Invalid
• Estado Update from Open to Cerrado

Comentario

Invalid Report

• This is not a mingw.org issue.
• You should file your report with GCC at https://gcc.gnu.org/bugs/.
• Your report indicates that you're using a different distribution of GCC than the one we provide.

2018-07-27 19:47 Updated by: nik-ponomarev

Comentario

Same GCC version does not have this bug

2018-07-27 20:40 Updated by: keith

Comentario

Reply To nik-ponomarev

Same GCC version does not have this bug

With respect, that's an asinine, and utterly meaningless statement; you are reporting an issue with a specific version of GCC so obviously that particular version of GCC does exhibit the issue.

I'm sorry to say this, but while we thank you for the heads-up, (which will enable us to check whether, or not, our distribution of GCC-7.3 exhibits this issue), we simply cannot help you to resolve your issue, because your report clearly demonstrates that you are not using our GCC distribution; you must seek assistance from the distributor of the compiler you are actually using, (which is not ours), or escalate it upstream to gcc.org. Furthermore, just because some GCC-7.3 build, presumably for some other platform, doesn't exhibit the issue, that does not preclude a bug in target specific gcc.org code, in which case reporting your issue upstream would be entirely appropriate.

2018-07-27 21:15 Updated by: keith

• Details Updated

2018-07-27 21:19 Updated by: nik-ponomarev

Comentario

Ok, many thanks for the detailed and informative instructions! I will submit the issue to GCC bug-tracker.

2018-07-28 04:59 Updated by: keith

Comentario

I don't know exactly what you are trying to demonstrate, with your test case, but if I understand you correctly, you tripped over some unexpected behaviour with std::atomic. FWIW, if I substitute std::atomic<int32_t>, (having also added #include <cstdint>), for your home-brewed alternative struct atomic:

 #include <cstdint>
 #include <iostream>
 #include <cstring>
 #include <atomic>
 
 struct TestStruct
 { std::atomic<int32_t> storage;
 
   TestStruct(){ memset(static_cast<void *>(&storage),-1,sizeof(storage)); }
   void operator()(unsigned long i)
   { std::atomic<int32_t>* ptr2 = new (&storage) std::atomic<int32_t>(i);
     unsigned long val1 = ptr2->load();
     std::cout << "Expected: " << i << "; First load value: " << val1 << std::endl;
     unsigned long val2 = ptr2->load();
     std::cout << "Expected: " << i << "; Second load value: " << val2 << std::endl;
   }
 };
 
 int main()
 { TestStruct()(0UL);
   return 0;
 }

Then cross-compile and run it (under wine), with the Linux hosted build of our GCC-7.3.0:

$ mingw32-g++ -O2 --std=gnu++11 t_atomic.cpp
$ ./a.exe
Expected: 0; First load value: 0
Expected: 0; Second load value: 0

Isn't this the behaviour you expect?

BTW, I'm no expert in C++, (in fact, I find that it all too frequently degenerates into inscrutable black magic), doesn't that new operator in my line 11, (corresponding to your line 26), create a memory leak?

2018-07-29 21:08 Updated by: keith

Comentario

Reply To keith

BTW, I'm no expert in C++, (in fact, I find that it all too frequently degenerates into inscrutable black magic), doesn't that new operator in my line 11, (corresponding to your line 26), create a memory leak?

I guess it doesn't. As I said, I'm no expert in C++, and I'd never seen the "placement" overload of new before. Some research tells me that this usage is simply creating a new pointer, referring to an existing memory "allocation"; there is no newly allocated memory to leak.