Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++20 volatile warnings #1379

Closed
GFoxPM opened this issue Apr 17, 2023 · 7 comments
Closed

C++20 volatile warnings #1379

GFoxPM opened this issue Apr 17, 2023 · 7 comments
Labels
enhancement New feature or request

Comments

@GFoxPM
Copy link

GFoxPM commented Apr 17, 2023

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
Copy link
Contributor

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

@maxgerhardt
Copy link
Contributor

maxgerhardt commented Apr 17, 2023

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 raspberrypi/pico-sdk#1017 (comment) 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
Copy link
Owner

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.

@earlephilhower earlephilhower added the enhancement New feature or request label Apr 17, 2023
@earlephilhower earlephilhower changed the title c++20 warnings fix C++20 volatile warnings Apr 17, 2023
@GFoxPM
Copy link
Author

GFoxPM commented Apr 17, 2023

@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
Copy link
Owner

@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
Copy link
Author

GFoxPM commented Apr 19, 2023

@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
Copy link
Owner

Per esp8266/Arduino#8916 (comment) it seems these volatile warnings are de-deprecated in C++23. So, closing this as a won't-fix.

@earlephilhower earlephilhower closed this as not planned Won't fix, can't repro, duplicate, stale Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants