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

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

Closed
wants to merge 4 commits into from

Conversation

prusnak
Copy link

@prusnak prusnak commented Jan 10, 2019

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

@dpgeorge
Copy link
Member

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 1fe30d1).

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

prusnak commented Jan 11, 2019

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

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

prusnak commented Jan 14, 2019

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

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

prusnak commented Jan 15, 2019

I agree.

@prusnak
Copy link
Author

prusnak commented Jan 15, 2019

Force pushed to my branch

@dpgeorge
Copy link
Member

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

prusnak commented Jan 23, 2019

Updated the PR

@prusnak
Copy link
Author

prusnak commented Jan 24, 2019

I realised this should not go to the vendor branch but rather to the latest active work-* branch. Superseded by #7

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

Successfully merging this pull request may close these issues.

2 participants