search

STMicroelectronics / stm32h7xx_hal_driver

Provides the STM32Cube MCU Component "hal_driver" of the STM32H7 series.
BSD 3-Clause "New" or "Revised" License
86 stars 36 forks source link

Padding warnings that need fix #27

Closed escherstair closed 3 weeks ago

escherstair commented 2 years ago

When the source code is comèpiled with armclang with all the warnbings enabled, some padding warnings are shown. I think that "zero compiler warnings" with very strict compilation flags is an ambitious, but really appreciated target from the user point of view.

Here are some of these needed paddings: .../STM32H7xx_HAL_Driver/Inc\stm32h7xx_hal_uart.h(234): warning: padding struct 'struct __UART_HandleTypeDef' with 2 bytes to align 'FifoMode' [-Wpadded] So uint16_t padding; is needed just before https://github.com/STMicroelectronics/stm32h7xx_hal_driver/blob/8e7eb8dba8bee5e4c260c6fc724437502e98fc76/Inc/stm32h7xx_hal_uart.h#L233-L234

.../STM32H7xx_HAL_Driver/Inc\stm32h7xx_hal_uart.h(253): warning: padding struct 'struct __UART_HandleTypeDef' with 3 bytes to align 'gState' [-Wpadded] So uint8_t padding_bytes[3]; should be needed just before https://github.com/STMicroelectronics/stm32h7xx_hal_driver/blob/8e7eb8dba8bee5e4c260c6fc724437502e98fc76/Inc/stm32h7xx_hal_uart.h#L252-L254 But I think that this depends on the behavior of the compiler, because HAL_LockTypeDef is an enum and so every compiler can set its size to different values. Maybe an union to a fixed size value is the best option?

wovano commented 12 months ago

I generally agree with the wish to support warning-free compilation, but in this case it simply means that you've turned on too many warnings (see also here). This is not a compiler warning that indicates a (possible) problem with the code, but simply an extra diagnostic that is disabled by default (and even when using -Wall).

Manually adding padding bytes will only obfuscate the code and does not solve any problem (other than suppressing this warning that you specifically enabled).

The point of this compiler warning is to be aware of memory wasted due to struct padding. Manually added dummy variables will waste the same amount of memory but hide it from the compiler. The real solution would be to reorder the struct members from large to small so the amount of padding bytes is actually minimized. This would reduce the struct size from 144 to 140 bytes (or from 200 to 196 bytes if callbacks are enabled), which is quite insignificant. If memory constraints are that important, the low-level HAL would probably be a better match. For the HAL, readability/understandability and maintainability are more important and I doubt this proposal would really benefit anyone.

My 2 cents.

KRASTM commented 4 weeks ago

Hello @escherstair,

Sorry for the very late response, thank you for the report.

About your issue, this is a known limitation mentioned in the release note for the firmware package: Only template projects migrated to Arm Compiler 6 with MDK-ARM 5.29 or upper version (“AC5-like Warnings” mode). Currently we don't support the option "All warning enabled", just “AC5-like Warnings” as discussed.

With regards,

KRASTM commented 3 weeks ago

Hello @escherstair,

Please, allow me to close this thread, thank you for your comprehension.

With regards.