earlephilhower / arduino-pico

Raspberry Pi Pico Arduino core, for all RP2040 and RP2350 boards
GNU Lesser General Public License v2.1
2.03k stars 422 forks source link

C++20 volatile warnings #1379

Closed GFoxPM closed 1 year ago

GFoxPM commented 1 year ago

Lots of warnings: compound assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile] Fixes like: /cores/rp2040/RP2040Support.h#L361

-  rp2040._epoch += 1LL << 24;
+  rp2040._epoch = rp2040._epoch + (1LL << 24);

/cores/rp2040/Bootsel.cpp#L38

-  for (volatile int i = 0; i < 1000; ++i);
+  for (int i = 0; i < 1000; ++i); // ?

/cores/rp2040/SerialPIO.cpp#L237

-  _rxPIO->sm[_rxSM].shiftctrl |= 0x80000000;
+  _rxPIO->sm[_rxSM].shiftctrl = _rxPIO->sm[_rxSM].shiftctrl | 0x80000000;
maxgerhardt commented 1 year ago

The second change will optimize the loop away completeley, so its intended function of providing a delay is destroyed.

maxgerhardt commented 1 year ago

Hm, using i = i + 1 instead of ++1 generates the warning: using value of simple assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile]ARM gcc 11.2.1 (none) #1 warning but then https://github.com/raspberrypi/pico-sdk/issues/1017#issuecomment-1242814904 says that this has already been deprecated as warning.

Other alternatives generated different microcode, so it would change the timing. Not sure if it's enough to break BOOTSEL functionality.

See https://godbolt.org/z/77TxzWM87

earlephilhower commented 1 year ago

For the 2nd one I think the following is the safest:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wno-volatile"
<for loop>
#pragma GCC diagnostic pop

That said, this is something that wouldn't be looked at until the move from GCC-10 to GCC-12, whenever it comes around. Also, per @maxgerhardt 's linked issue the C++ guys seem to have dropped this as a warning going forward so maybe by then it will be a non-issue.

GFoxPM commented 1 year ago

@earlephilhower

pragma GCC diagnostic ignored "-Wno-volatile"

pragma GCC diagnostic ignored "-Wvolatile"

Is there more information about the lack of warnings in GCC12? Because in ARM GCC 12.2 (Linux) the warnings are still there, and it's unlikely to be platform dependent... C++20 https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1152r3.html C++23 https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2327r1.pdf https://gcc.gnu.org/projects/cxx-status.html ARM GCC Trunk (13) with -std=c++2b -std=gnu++2b still generates warnings.

Doesn't generate a warning: /cores/rp2040/Bootsel.cpp#L38

-  for (volatile int i = 0; i < 1000; ++i);
+  volatile int i = 1000;
+  while(i) { i = i - 1; }
earlephilhower commented 1 year ago

@GFoxPM are you using --std=gnu++20 now? Would you like to put in a PR with all your suggested changes? We can pull them in now if there is no issue w/our current C++ level, although we may accidentally introduce other problems between now and when we go to C++20...

GFoxPM commented 1 year ago

@earlephilhower Yes, I'm currently using --std=gnu++20 But I'm not sure about the completeness of my edits, because this is only what the compiler warned about.

earlephilhower commented 1 year ago

Per https://github.com/esp8266/Arduino/issues/8916#issuecomment-1519095142 it seems these volatilewarnings are de-deprecated in C++23. So, closing this as a won't-fix.