p3p / pio-framework-arduino-lpc176x

10 stars 20 forks source link

Rename Sense_ASC::NONE to Sense_ASC::DISABLED to fix ODR warning #51

Closed mh-dm closed 1 month ago

mh-dm commented 2 months ago

Rename Sense_ASC::NONE to Sense_ASC::DISABLED to fix ODR warning

When compiling Marlin with -flto, gcc outputs some ODR warnings:

Linking .pio/build/LPC1769/firmware.elf
.platformio/packages/framework-arduino-lpc176x/system/CMSIS/lib/usb/mscuser.h:55:13: warning: type 'Sense_ASC' violates the C++ One Definition Rule [-Wodr]
   55 | enum struct Sense_ASC : uint8_t {
      |             ^
.platformio/packages/framework-arduino-lpc176x/system/CMSIS/lib/usb/mscuser.h:55:13: note: an enum with different value name is defined in another translation unit
   55 | enum struct Sense_ASC : uint8_t {
      |             ^
.platformio/packages/framework-arduino-lpc176x/system/CMSIS/lib/usb/mscuser.h:56:3: note: name 'DISABLED' differs from name 'NONE' defined in another translation unit
   56 |   NONE = 0x0,
      |   ^
.platformio/packages/framework-arduino-lpc176x/system/CMSIS/lib/usb/mscuser.h:56:3: note: mismatching definition
   56 |   NONE = 0x0,
      |   ^

This is a very confusing warning since gcc doesn't say anything else about the "another translation unit". It at least it says it's expecting DISABLED instead of NONE but I can't find any other Sense_ASC defined anywhere.

rgrep DISABLED
Binary file system/CMSIS/bin/libusbd_175x_6x_lib.a matches

rgrep Sense
Binary file system/CMSIS/bin/libusbd_175x_6x_lib.a matches

Binary files? In my open source?

Let's just rm system/CMSIS/bin/libusbd_175x_6x_lib.a. The rebuild notices something changed but there's still the ODR error and resulting final binary output does not change (same sha256sum .pio/build/LPC1769/firmware.elf). I guess system/CMSIS/bin/libusbd_175x_6x_lib.a is unnecessary, at least in (my?) Marlin build. But this doesn't help the ODR warning.

If you know how to find where the other definition is coming from I'd be quite interested. That said, this rename is simple enough to do (especially since there are no actual users of NONE from Sense_ASC I could find incl. through web searches) so I might as well offer it up as a PR.

p3p commented 1 month ago

You are correct system/CMSIS/bin/libusbd_175x_6x_lib.a was never used, it is just a left over from when I was trying to get the usbd working initially, I will have to have a look at where this conflict is coming from, I haven't seen the error myself.

mh-dm commented 1 month ago

The warning only shows up with -flto. But a word of caution: -flto does not work (Marlin breaks completely). Took a while but I finally figured out why -flto doesn't work (random missing volatile). I'll send a PR but I'd like to get to a solution that doesn't end up making incremental builds 10s slower (flto significantly slows down the link step). A solution with -flto partly enabled that results in some measurable benefits.

p3p commented 1 month ago

fun times NONE is defined as DISABLED in Marlin

macros.h:200 #define NONE DISABLED

mh-dm commented 1 month ago

Wow 😮, that was the issue. Closing this wrong PR then.

I'll make a PR in marlin to do this instead: #define NONE(V...) DISABLED(V)