From e6ed3d9f7f844cbcef872796df33ef3e41e15db6 Mon Sep 17 00:00:00 2001 From: Mike Date: Mon, 23 Sep 2024 08:40:33 +0100 Subject: [PATCH] Fix build warnings/errors in C++20 (#2883) **FlashString static initialisation** The `Object` copy and move constructors are deleted as a precaution as these objects should not be copied. C++20 doesn't like this so they're only compiled for C++17 and earlier. **std:is_pod deprecated** Use std::is_standard_layout instead (only one instance to correct). **Volatile** C++20 deprecates many uses of `volatile` which may be implemented in an ambiguous (non-deterministic) way. Largely these uses are concerned with threaded applications. A general web search turns up plenty of articles on this. One argument is related to code like this: ```c++ volatile int num; ++num; // Is this atomic? Or a read, followed by load? ``` So we should identify all such cases and code them more explicitly. The FIFO/FILO classes define `numberOfElements` as volatile but I can see no reason why this needs to be the case. **Volatile bitwise operations** C++23 will, it seems, de-deprecate use of volatile for bitwise operations. This is discussed here https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2327r1.pdf. See also https://github.com/earlephilhower/arduino-pico/issues/1379 These are genuine uses of volatile as values are used both in interrupt and task context. Some of these actually need to be atomic, and the assumption is probably that `x |= n` does that. Hence the ambiguity. These are the core Sming files affected: * Sming/Arch/Esp8266/Components/driver/i2s.cpp * Sming/Arch/Esp8266/Components/spi_flash/iram_precache.cpp * Sming/Arch/Esp8266/Components/gdbstub/appcode/gdb_hooks.cpp * Sming/Arch/Esp8266/Components/gdbstub/gdbstub.cpp * Sming/Arch/Esp8266/Components/gdbstub/gdbuart.cpp * Sming/Arch/Esp8266/Core/Digital.cpp There will be many others in libraries. Solution: Rather than attempting to 'correct' the code and introduce bugs - risks identified in the above paper - it is safer to just silence this particular warning. This has been done in the main Sming `build.mk` file so it's easy to revert if required. This is the `volatile` warning which covers other uses as well - future GCC revisions may add some granularity to this. **FIFO / FILO uses int instead of unsigned** No warnings but using `int` for the count makes no sense. Not related to C++20 but since we're modifying the classes anyway may as well do it here. --- Sming/Components/FlashString | 2 +- .../Storage/src/include/Storage/Partition.h | 2 +- Sming/Wiring/FIFO.h | 20 +++++++++---------- Sming/Wiring/FILO.h | 20 +++++++++---------- Sming/build.mk | 5 +++++ 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/Sming/Components/FlashString b/Sming/Components/FlashString index 79eb493406..ee011c39a8 160000 --- a/Sming/Components/FlashString +++ b/Sming/Components/FlashString @@ -1 +1 @@ -Subproject commit 79eb493406c3138cb20abf92d8afd5e357f4f715 +Subproject commit ee011c39a8aa75fefd6dd6f5eb48deb8d7feda4b diff --git a/Sming/Components/Storage/src/include/Storage/Partition.h b/Sming/Components/Storage/src/include/Storage/Partition.h index 7069c55c78..98c6b9cf5a 100644 --- a/Sming/Components/Storage/src/include/Storage/Partition.h +++ b/Sming/Components/Storage/src/include/Storage/Partition.h @@ -281,7 +281,7 @@ class Partition bool read(storage_size_t offset, void* dst, size_t size); template - typename std::enable_if::value, bool>::type read(storage_size_t offset, T& value) + typename std::enable_if::value, bool>::type read(storage_size_t offset, T& value) { return read(offset, &value, sizeof(value)); } diff --git a/Sming/Wiring/FIFO.h b/Sming/Wiring/FIFO.h index ffd6df5728..8a2d6e53be 100644 --- a/Sming/Wiring/FIFO.h +++ b/Sming/Wiring/FIFO.h @@ -21,10 +21,10 @@ #include "Countable.h" -template class FIFO : public Countable +template class FIFO : public Countable { public: - const int size; // speculative feature, in case it's needed + const unsigned size; // speculative feature, in case it's needed FIFO(); @@ -60,18 +60,18 @@ template class FIFO : public Countable } protected: - volatile int numberOfElements; - int nextIn; - int nextOut; + unsigned numberOfElements; + unsigned nextIn; + unsigned nextOut; T raw[rawSize]; }; -template FIFO::FIFO() : size(rawSize) +template FIFO::FIFO() : size(rawSize) { flush(); } -template bool FIFO::enqueue(T element) +template bool FIFO::enqueue(T element) { if(full()) { return false; @@ -85,7 +85,7 @@ template bool FIFO::enqueue(T element) return true; } -template T FIFO::dequeue() +template T FIFO::dequeue() { T item; numberOfElements--; @@ -95,12 +95,12 @@ template T FIFO::dequeue() return item; } -template T FIFO::peek() const +template T FIFO::peek() const { return raw[nextOut]; } -template void FIFO::flush() +template void FIFO::flush() { nextIn = nextOut = numberOfElements = 0; } diff --git a/Sming/Wiring/FILO.h b/Sming/Wiring/FILO.h index 15253633a8..88d9e800a8 100644 --- a/Sming/Wiring/FILO.h +++ b/Sming/Wiring/FILO.h @@ -20,10 +20,10 @@ #include "Countable.h" -template class FILO : public Countable +template class FILO : public Countable { public: - const int size; // speculative feature, in case it's needed + const unsigned size; // speculative feature, in case it's needed FILO(); @@ -59,18 +59,18 @@ template class FILO : public Countable } private: - volatile int numberOfElements; - int nextIn; - int nextOut; + unsigned numberOfElements; + unsigned nextIn; + unsigned nextOut; T raw[rawSize]; }; -template FILO::FILO() : size(rawSize) +template FILO::FILO() : size(rawSize) { flush(); } -template bool FILO::push(T element) +template bool FILO::push(T element) { if(count() >= rawSize) { return false; @@ -79,7 +79,7 @@ template bool FILO::push(T element) return true; } -template T FILO::pop() +template T FILO::pop() { if(numberOfElements > 0) { return raw[--numberOfElements]; @@ -87,7 +87,7 @@ template T FILO::pop() return raw[0]; } -template T FILO::peek() const +template T FILO::peek() const { if(numberOfElements > 0) { return raw[numberOfElements - 1]; @@ -95,7 +95,7 @@ template T FILO::peek() const return raw[0]; } -template void FILO::flush() +template void FILO::flush() { nextIn = nextOut = numberOfElements = 0; } diff --git a/Sming/build.mk b/Sming/build.mk index 9b8d4c7c37..b4d50a34f9 100644 --- a/Sming/build.mk +++ b/Sming/build.mk @@ -239,6 +239,11 @@ COMPILER_VERSION_FULL := $(shell LANG=C $(CC) -v 2>&1 | $(AWK) -F " version " '/ COMPILER_NAME := $(word 1,$(COMPILER_VERSION_FULL)) COMPILER_VERSION := $(word 2,$(COMPILER_VERSION_FULL)) +# Use of bitwise assignment for volatile registers is deprecated in C++20 but de-deprecated in C++23 +ifeq ($(COMPILER_NAME)-$(SMING_CXX_STD),gcc-c++20) +CXXFLAGS += -Wno-volatile +endif + ifndef USE_CLANG # Required to access peripheral registers using structs # e.g. `uint32_t value: 8` sitting at a byte or word boundary will be 'optimised' to