micropython / stm32lib

STM32 Cube library - CMSIS and HAL for F4, F7 and L4 microcontrollers
63 stars 71 forks source link

Remove packed attribute for uint32_t (new GCC-8 warning) #6

Closed prusnak closed 5 years ago

prusnak commented 5 years ago

New GCC-8 complains about using packed attribute for uint32_t. This patch removes this extraneous usage.

dpgeorge commented 5 years ago

Thanks for the patch. We need to be careful with things like this because the HAL can be picky when it comes to these kinds of things (see eg 1fe30d1446f2eba3730dc4b05e9ac0cf03bcf9bf).

I think the __packed attribute here is intended to mean that the uint32_t* pointer may be unaligned, and that's important to keep, so I'd be hesitant about removing the attribute.

With arm-none-eabi-gcc version 8.2.0 I don't get any warnings. Which version do you use?

prusnak commented 5 years ago

arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 8-2018-q4-major) 8.2.1 20181213 (release) [gcc-8-branch revision 267074]

downloaded from: https://developer.arm.com/open-source/gnu-toolchain/gnu-rm/downloads

dpgeorge commented 5 years ago

Doing some research, it seems that ARM CC accepts the __packed attribute and takes it to mean "this pointer may be unaligned", see http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0472m/chr1359124968737.html

Then for gcc there is a compatibility macro in the HAL which defines __packed to attribute((packed)). But gcc doesn't support this usage, since to it packed means don't add padding in a struct. So I guess that gcc 8.2.1 is now warning about this bad usage.

https://answers.launchpad.net/gcc-arm-embedded/+question/452880 contains some more info about gcc in such a case, and the last post on that page provides a way to get it working in gcc by defining a packet struct with a single member (in this case it would be a uint32_t).


I'm assuming that this packed qualifier never did anything on gcc (although that remains to be checked) and so I think the best way to handle it is to #define __packed to nothing for the case of gcc.

prusnak commented 5 years ago

It seems the __packed is indeed used only for uint32_t in the whole stm32lib codebase, so it might make sense to undefine it for GCC in this lib.

dpgeorge commented 5 years ago

It seems the __packed is indeed used only for uint32_t in the whole stm32lib codebase, so it might make sense to undefine it for GCC in this lib.

How about just undefining it in these ll_usb.c files? I.e. at the top of these .c files: #if GCC, #undef __packed. That is the smallest change (so least unforeseen consequences) that solves the problem.

prusnak commented 5 years ago

I agree.

prusnak commented 5 years ago

Force pushed to my branch

dpgeorge commented 5 years ago

Sorry to be a bother, but I just realised this will need to be split into 4 separate commits (which can be all put in this same PR), one for each of the MCU variants. That makes it easier to maintain in the future, when the Cube HAL is updated. (Existing commits do this.)

It'll also be good to add a C comment to this change, something like: In this file __packed is used to signify an unaligned pointer, which GCC doesn't support, so disable it.

Thanks!

prusnak commented 5 years ago

Updated the PR

prusnak commented 5 years ago

I realised this should not go to the vendor branch but rather to the latest active work-* branch. Superseded by https://github.com/micropython/stm32lib/pull/7