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

Enable C++20 #521

Merged
merged 16 commits into from
Jan 10, 2021
Merged

Enable C++20 #521

merged 16 commits into from
Jan 10, 2021

Conversation

rleh
Copy link
Member

@rleh rleh commented Dec 19, 2020

  • Update ARM GCC to version 10
  • Use new Docker images (hosted on Github Container Registry ghcr.io)
  • Update installation guide
  • Enable C++20

@rleh rleh requested a review from salkinium December 19, 2020 00:05
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MacOS CI may fail until homebrew is updated.

docs/src/guide/installation.md Show resolved Hide resolved
@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build/release/modm/libmodm.a(event_groups.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: multiple definition of `modm::platform::fcpu'; build/release/modm/libmodm.a(croutine.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: first defined here
/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build/release/modm/libmodm.a(list.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: multiple definition of `modm::platform::fcpu'; build/release/modm/libmodm.a(croutine.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: first defined here
/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build/release/modm/libmodm.a(port.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: multiple definition of `modm::platform::fcpu'; build/release/modm/libmodm.a(croutine.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: first defined here
/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build/release/modm/libmodm.a(queue.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: multiple definition of `modm::platform::fcpu'; build/release/modm/libmodm.a(croutine.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: first defined here
/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build/release/modm/libmodm.a(stream_buffer.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: multiple definition of `modm::platform::fcpu'; build/release/modm/libmodm.a(croutine.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: first defined here
/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build/release/modm/libmodm.a(tasks.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: multiple definition of `modm::platform::fcpu'; build/release/modm/libmodm.a(croutine.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: first defined here
/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build/release/modm/libmodm.a(timers.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: multiple definition of `modm::platform::fcpu'; build/release/modm/libmodm.a(croutine.o):/root/project/examples/nucleo_f103rb/rtos/modm/ext/freertos/inc/freertos/FreeRTOSConfig.h:39: first defined here
collect2: error: ld returned 1 exit status
scons: *** [build/release/rtos.elf] Error 1

@salkinium
Copy link
Member

Oh yeah, I forgot C++20 deprecated volatile. Not entirely sure if we can fix this at all (considering the volatile comes from extern "C" headers) so we have to just ignore the warning?

@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

Errors

  • Cortex-M on WIndows: Zip folder structure changed. Fixed!
  • Linux running on Aarch64 (Travis): GCC10 (hosted) has to be installed. Fixed!
  • MacOS: Waiting for Update to 10-2020-q4-major osx-cross/homebrew-arm#16...
  • Cortex-M on Linux: see comment above
  • Windows: WTF?!?
The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "../tools/scripts/examples_compile.py", line 113, in <module>
    exit(compile_examples(sys.argv[1:]))
  File "../tools/scripts/examples_compile.py", line 99, in compile_examples
    projects = pool.map(build, projects)
  File "C:\hostedtoolcache\windows\Python\3.8.6\x64\lib\multiprocessing\pool.py", line 364, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
  File "C:\hostedtoolcache\windows\Python\3.8.6\x64\lib\multiprocessing\pool.py", line 771, in get
    raise self._value
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xb7 in position 118: invalid start byte
Error: Process completed with exit code 1.

@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

STM32 Unittests

FLASH' overflowed by 10424 bytes

/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: release/test.elf section `.text' will not fit in region `FLASH'
/opt/gcc-arm-none-eabi-10-2020-q4-major/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: region `FLASH' overflowed by 10424 bytes

@salkinium
Copy link
Member

FLASH' overflowed by 10424 bytes

Yes, the F103 was like 98% full before… so I guess there's been a little regression in GCC code size?

@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

There is already a lot disabled for STM32F1: https://github.com/modm-io/modm/blob/develop/test/config/nucleo-f103.xml

  <modules>
    <module>modm:platform:heap</module>
    <!-- Not everything fits -->
    <!-- <module>modm-test:test:architecture</module>
    <module>modm-test:test:communication</module>
    <module>modm-test:test:container</module>
    <module>modm-test:test:driver</module>
    <module>modm-test:test:io</module> -->

    <module>modm-test:test:platform:**</module>
    <module>modm-test:test:processing</module>
    <module>modm-test:test:ui</module>
    <module>modm-test:test:math</module>
  </modules>

@chris-durand
Copy link
Member

Oh yeah, I forgot C++20 deprecated volatile. Not entirely sure if we can fix this at all (considering the volatile comes from extern "C" headers) so we have to just ignore the warning?

Only some parts are deprecated of course. Otherwise the compiler would optimize away all hardware register accesses. There is still no other way to tell the compiler not to mess with your memory mapped IO. They deprecated all the insane nonsensical parts no-one should ever use, like volatile qualified member functions, volatile return values, volatile function parameters, etc.

The annoying part is they also deprecated some parts which are ubiquitously used in vendor provided code. Compound operations on volatiles like SOME_REGISTER |= (1 << 4) are deprecated. There are a lot of embedded people who consider that part a mistake.

@chris-durand
Copy link
Member

From P1152R4 "Deprecating volatile"

We propose deprecating most of volatile. See § 3 Wording for the details.

The proposed deprecation preserves the useful parts of volatile, and removes the dubious / already broken ones.

@chris-durand
Copy link
Member

Reddit flamewar discussion on that topic: Compound assignment to volatile must be un-deprecated

@salkinium
Copy link
Member

Summary of the comments:

  • "I write userspace applications with the standard library and know about std::atomic<>, you know about std::atomic, right?"
  • "You, an embedded developer, don't know how stores/loads interact with interrupts - you clearly don't know how invalid your code is"
  • "Why not write wrapper functions for the entire chip API, in your heavily-constrained embedded development environment. You're using gcc/llvm, right? It'll probably all be inlined and elilded, probably."
  • "Here's a bit-twiddling refactor of your version. I did this myself. It doesn't address the complaint, but it does save a load. Not enough loads to actually avoid a race with an interrupt, but it does save one."
  • "The chip manufacturer should just make their C headers C++20 compatible - unless the chip manufacturer is anything like 90 % of chip manufacturers, which typically only explicitly support an approved dialect of C"
  • "God, these embedded manufacturers should just get with the times and write modern C++. These potentially breaking changes are great for C++" ... "but don't move to an entirely different language like Rust because you'd be losing out on the backwards-compat benefits you want from C++"
  • "The C++ standards committee should implement a new language feature that enables C++ programmers to do stuff that's possible in C89. Also, they should deprecate bitfields, I don't like bitfields."

Scathing…

@chris-durand
Copy link
Member

We might have a few [[nodiscard]] in the wrong place:

In file included from modm/src/modm/platform.hpp:15,
                 from modm/src/modm/board/board.hpp:17,
                 from modm/src/modm/board.hpp:12,
                 from main.cpp:15:
modm/src/modm/platform/can/can_1.hpp:111:49: warning: 'nodiscard' attribute can only be applied to functions or to class or enumeration types [-Wattributes]
  111 |     bool overwriteOnOverrun = true) [[nodiscard]]

It has to go to the front. I am quite sure written like above it does nothing despite giving the warning about misuse of attributes.

@chris-durand
Copy link
Member

I'll have a look at the [[nodiscard]]s

@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

Now another error on Windows hosted:

 modm\src\modm\platform\core\assert.cpp: In function 'void modm_abandon(const modm::AssertionInfo&)':
modm\src\modm\platform\core\assert.cpp:60:61: error: unknown conversion type character 'l' in format [-Werror=format=]
   60 |  if (info.context != uintptr_t(-1)) { MODM_LOG_ERROR.printf(" @ %p (%" PRIuPTR ")", (void *)info.context, info.context); }
      |                                                             ^~~~~~~~~~~~~~~~~~~~~~
modm\src\modm\platform\core\assert.cpp:60:61: error: too many arguments for format [-Werror=format-extra-args]
cc1plus.exe: some warnings being treated as errors

Has anyone seen character 'l'?

@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

Probably because I exchanged MinGW32 with a gcc-10 based Mingw64...

@chris-durand
Copy link
Member

chris-durand commented Dec 19, 2020

Has anyone seen character 'l'?

Don't you see the l in PRIuPTR 😉 ?

PRIuPTR is for uintptr_t, probably lu, format for unsigned long.

@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

https://stackoverflow.com/a/55099671

Use %I64u on Windows, or just use inttypes.h PRIuMAX.

Or: dosbox-staging/dosbox-staging#64 (comment)

@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

😒

 modm\src/modm/platform/can/can_bit_timings.hpp:80:22: error: call to non-'constexpr' function 'double fabs(double)'
   80 |    float error = fabs(1 - intPrescaler/idealPrescaler);
...
c:\winlibs-gcc\mingw64\x86_64-w64-mingw32\include\math.h:220:31: note: 'double fabs(double)' declared here
  220 |   __CRT_INLINE double __cdecl fabs (double x)

@chris-durand
Copy link
Member

fabs is not required to be constexpr, not even in C++20 but in libstdc++ it actually is. Should I fix it by putting a constexpr version in there?
Like:

template <typename Float>
    requires std::is_floating_point_v<Float>
constexpr Float constexpr_fabs(Float number)
{
    if (number >= 0) {
        return number;
    } else 
        return -number;
    }
}

@rleh
Copy link
Member Author

rleh commented Dec 19, 2020

Sehr gerne!

@chris-durand
Copy link
Member

Ok, I'll do it.

@chris-durand
Copy link
Member

Could you have a look at it and cherry-pick the commit? chris-durand@6578b24

@rleh rleh requested a review from salkinium December 19, 2020 16:29
@chris-durand
Copy link
Member

I am fixing AVR time_test. It dereferences a pointer to a value in flash without accessor::Flash. Furthermore, the date implementation has some integer overflows on AVR.

We should consider to move to llvm for AVR at some point because they have solved the problem with the separated flash address space in a beautiful way, no need for accessor::Flash anymore. With llvm the address space attribute is part of the type system. Thus, you can even run STL algorithms on flash arrays. See here: https://gcc.godbolt.org/z/cTjWYn

@chris-durand
Copy link
Member

chris-durand commented Jan 5, 2021

Fix for Date classes on AVR: #530
Note to self: this Date stuff needs refactoring, let's fix the bugs first ...

@rleh rleh removed the stale ♾ label Jan 5, 2021
@rleh
Copy link
Member Author

rleh commented Jan 5, 2021

@salkinium Are you ok with merging this PR despite of 169dfab?

@salkinium
Copy link
Member

@salkinium Are you ok with merging this PR despite of 169dfab?

Yes.

@chris-durand
Copy link
Member

Apart from the communication tests everything passes on AVR with #531 and the branch rebased to develop.

Unittests (Jan  6 2021, 23:29:30)
>>> LUDecomposition_test
>>> ad7280a_test
>>> angle_test
>>> arithmetic_traits_test
>>> atomic_queue_test
>>> bit_operation_test
>>> block_allocator_test
>>> bounded_deque_test
>>> bounded_queue_test
>>> bounded_stack_test
>>> button_group_test
>>> button_test
>>> can_bit_timings_test
>>> can_lawicel_formatter_test
>>> can_message_test
>>> circle_2d_test
>>> clock_test
>>> debounce_test
>>> doubly_linked_list_test
>>> drv832x_spi_test
>>> dynamic_array_test
>>> endianness_test
>>> fir_test
>>> flash_test
>>> i2c_test
>>> io_stream_test
>>> lagrange_interpolation_test
>>> line_2d_test
>>> line_segment_2d_test
>>> linear_interpolation_test
>>> linked_list_test
>>> location_2d_test
>>> ltc2984_test
>>> matrix_test
>>> matrix_vector_test
>>> mcp2515_can_bit_timings_test
>>> median_test
>>> moving_average_test
>>> operator_test
>>> pair_test
>>> periodic_timer_test
>>> pid_test
>>> point_set_2d_test
>>> polygon_2d_test
>>> prescaler_test
>>> protothread_test
>>> quaternion_test
>>> ramp_test
>>> range_test
>>> ray_2d_test
>>> register_test
>>> resumable_test
>>> s_curve_controller_test
>>> s_curve_generator_test
>>> saturated_test
>>> scheduler_test
>>> spi_device_test
>>> time_test
>>> timeout_test
>>> timestamp_test
>>> vector1_test
>>> vector2_test
>>> vector3_test
>>> vector4_test
>>> vector_test

Passed 3978 tests
OK!

@chris-durand
Copy link
Member

The xpcc AVR tests succeed whereas sab fails.

Unittests (Jan  7 2021, 00:11:55)
>>> can_connector_base_test
>>> can_connector_test
>>> dispatcher_test
>>> interface_test
>>> slave_test
FAIL: slave_test:118 : 0 == 1
FAIL: slave_test:140 : 0 == 2
FAIL: slave_test:148 : true == false
FAIL: slave_test:151 : 1 == 2
FAIL: slave_test:154 : 
[1, 163]
[205, 171]
FAIL: slave_test:166 : 0 == 3
FAIL: slave_test:190 : 0 == 4
FAIL: slave_test:191 : 0 == 39030
FAIL: slave_test:199 : true == false
FAIL: slave_test:202 : 1 == 4
FAIL: slave_test:204 : 
[1, 9, 0, 222]
[120, 86, 52, 18]

Failed 11 of 320 tests
FAIL!

@chris-durand
Copy link
Member

I don't intend to fix the SAB implementation because it's broken unrelated to this PR. The interface of sab::Slave requires the action array to be stored in flash and reads it using modm::accessor::Flash. The array content can't have constant initialization due to some undefined behavior reinterpret_casts of member function pointers in the SAB_ACTION macro and thus, can't be declared as PROGMEM. Therefore, the code reads the data from flash although it actually is in RAM and fails.

@rleh rleh force-pushed the update_c++20_ci branch from edd77af to 8c322a2 Compare January 8, 2021 23:28
@rleh
Copy link
Member Author

rleh commented Jan 8, 2021

Thanks @chris-durand for the great help with the tests!

I suggest to merge the pull request soon and then see which further improvements or bug fixes come up in the future.

@rleh rleh merged commit 8c322a2 into modm-io:develop Jan 10, 2021
@rleh rleh deleted the update_c++20_ci branch January 10, 2021 00:24
@rleh rleh restored the update_c++20_ci branch January 10, 2021 00:24
@@ -260,7 +260,8 @@ def common_compiler_flags(compiler, target):
# "-Wnon-virtual-dtor",
# "-Wold-style-cast",
"-fstrict-enums",
"-std=c++17",
"-std=c++20",
"-Wno-volatile", # volatile is deprecated in C++20 but lots of our external code uses it...
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that not every thing volatile is deprecated, but nearly every peace of exiting embedded vendor code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in theory a good idea, just not very practical: #521 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants