hal: nxp: ARRAY_SIZE collision

[gmarull]

Describe the bug

It looks like hal_nxp (mcux-sdk code) defines its own ARRAY_SIZE in a public scope, thus polluting the namespace. While it doesn't re-define if already defined, it is still problematic if included before zephyr/sys/util.h, and we should not rely on include order. In some situations, one can observe now:

In file included from ../../../../../../../include/zephyr/devicetree.h:21,
                 from ../../../../../../../include/zephyr/device.h:12,
                 from ../../../../../../../include/zephyr/drivers/usb/usb_dc.h:20,
                 from /__w/zephyr/zephyr/drivers/usb/device/usb_dc_mcux.c:12:
../../../../../../../include/zephyr/sys/util.h:108: error: "ARRAY_SIZE" redefined [-Werror]
  108 | #define ARRAY_SIZE(array) \
      | 
In file included from ../../../../../../../soc/arm/nxp_lpc/lpc55xxx/soc.h:19,
                 from /__w/zephyr/zephyr/drivers/usb/device/usb_dc_mcux.c:10:
/__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/drivers/common/fsl_common.h:236: note: this is the location of the previous definition
  236 | #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
      | 
cc1: all warnings being treated as errors
[168/181] Building C object zephyr/kernel/CMakeFiles/kernel.dir/sched.c.obj
ninja: build stopped: subcommand failed.

To Reproduce

west build -b lpcxpresso55s69_cpu0 samples/subsys/usb/hid -p

Expected behavior

HAL respects Zephyr ARRAY_SIZE. There are multiple ways to solve this:

1. Avoid namespace pollution, e.g. use FSL_ARRAY_SIZE
2. Do not expose such method in a "common" public header (possible if only used internally)
3. HAL uses Zephyr version when being compiled for Zephyr instead, e.g.

#ifdef __ZEPHYR__
#include <zephyr/sys/util.h>
#else
#define ARRAY_SIZE(x) ...
#endif

Impact

Build warnings, CI failures.

Logs and console output

See above.

Environment (please complete the following information):

• OS: Linux
• Toolchain Zephyr SDK 0.15.0
• Commit SHA or Version used 6755999

Additional context
https://github.com/zephyrproject-rtos/zephyr/actions/runs/3272340917/jobs/5383270655

[gmarull]

It turns out we have yet another case:

/__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/drivers/iuart/fsl_uart.c
In file included from /__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/drivers/common/./fsl_common.h:222,
                 from /__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/drivers/iuart/fsl_uart.h:12,
                 from /__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/drivers/iuart/fsl_uart.c:9:
/__w/zephyr/zephyr/include/zephyr/sys/util.h:512: error: "KB" redefined [-Werror]
  512 | #define KB(x) (((size_t)x) << 10)
      | 
In file included from /__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/CMSIS/Core_A/Include/core_ca53.h:157,
                 from /__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/devices/MIMX8MM6/./MIMX8MM6_ca53.h:265,
                 from /__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/devices/MIMX8MM6/./fsl_device_registers.h:21,
                 from /__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/drivers/common/./fsl_common.h:22:
/__w/zephyr/modules/hal/nxp/mcux/mcux-sdk/CMSIS/Core_A/Include/mmu_armv8a.h:45: note: this is the location of the previous definition
   45 | #define KB(x)                  ((x) << 10)

[dleach02]

One could argue that Zephyr should consider namespace protection as well.

While I agree that we shouldn't rely on include order the reason we have to is because for some reason the Zephyr project decided not to protect these generic definitions with "#ifndef" type of constructs like the NXP SDK does. So when we create our NXP shim drivers we ensure the order of includes to protect against this problem.

The logistics of actually changing this in our HAL is much more problematic due to how the NXP HAL is sourced. It isn't clear to me why we can't just put the name space collision check in the Zephyr code?

[gmarull]

Using #ifndef in Zephyr doesn't solve the include order problem. You can end up using HAL implementation in unexpected places if HAL header included first (e.g. with soc.h, included via many unexpected paths). The current proposal, zephyrproject-rtos/hal_nxp#197, is a solution that keeps HAL patch small (easy to re-apply if needed) and makes sure Zephyr code will always use Zephyr definitions.

[dleach02]

I understand you don't like ordering include files and you have a concern that the definition doesn't align if you happen to do the wrong order. So this problem is going to continue to happen since Zephyr doesn't protect this commonly defined macro.

If we are that concerned that a common macro is not going to be defined correctly by these included modules then we should make a change to the Zephyr definition. Either of these would work (one seems less annoying):

• change the name of the Zephyr macro to include some sort of zephyr identifier: Z_ARRAY_SIZE() for example
• use #UNDEF ARRAY_SIZE in the zephyr include to force the definition to the Zephyr version.

See the following as a separate example
#44730

Also, quick search of the tree finds the following:

\bootloader\mcuboot\boot\boot_serial\src\zcbor_common.h:#333
\modules\audio\sof\tools\fuzzer\fuzzer.h:#33
\modules\hal\espressif\components\bt\esp_ble_mesh\mesh_common\include\mesh_util.h:#59
\modules\hal\espressif\components\wpa_supplicant\src\utils\common.h:#443
\modules\hal\telink\tlsr9\ble\common\utility.h:#104
\modules\lib\chre\chpp\include\chpp\macros.h:#28
\modules\lib\loramac-node\src\boards\mcu\sam121\hal\utils\include\utils.h:#65
\modules\lib\zcbor\include\zcbor_common.h:#74
\modules\tee\tf-a\trusted-firmware-a\include\lib\utils_def.h:#14
\modules\tee\tf-m\trusted-firmware-m\secure_fw\include\array.h:#15 (a number of different files in TF-M define this)

Every time we include a new module or HAL with this particular approach we have to confirm that this new module isn't trying to define ARRAY_SIZE and if it does then we have to change the module code instead of changing things once in the Zephyr code base.

[gmarull]

I understand you don't like ordering include files and you have a concern that the definition doesn't align if you happen to do the wrong order. So this problem is going to continue to happen since Zephyr doesn't protect this commonly defined macro.

I'm not sure how you reached this conclusion, I've precisely proposed to follow Google include guidelines a few times, https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes. Zephyr is now a mess in this area, just see the issues triggered by this single commit: a6a4400. Namespacing the Zephyr one is definitely an option that we should consider, specially because we integrate with many 3rd party code that doesn't necessarily follow good practices. I wish Zephyr had a namespace for everything, actually. Lacking formal namespaces, this is something every C project should do, specially libraries or code integrating other libraries like Zephyr.

If we are that concerned that a common macro is not going to be defined correctly by these included modules then we should make a change to the Zephyr definition. Either of these would work (one seems less annoying):

• change the name of the Zephyr macro to include some sort of zephyr identifier: Z_ARRAY_SIZE() for example

Even if Zephyr adds a namespace, one could still see clashes if using 2 3rd party libraries in Zephyr. The best solution in the end is that every library does proper namespacing.

• use #UNDEF ARRAY_SIZE in the zephyr include to force the definition to the Zephyr version.

#undef is discouraged by e.g. MISRA, so if we care about safety we should likely follow the advice. #undef also makes 3rd party code use another implementation without any warning or sign, current patch makes it clear that hey, we've done this conscious choice in the context of Zephyr.

See the following as a separate example #44730

Also, quick search of the tree finds the following:

\bootloader\mcuboot\boot\boot_serial\src\zcbor_common.h:#333 \modules\audio\sof\tools\fuzzer\fuzzer.h:#33 \modules\hal\espressif\components\bt\esp_ble_mesh\mesh_common\include\mesh_util.h:#59 \modules\hal\espressif\components\wpa_supplicant\src\utils\common.h:#443 \modules\hal\telink\tlsr9\ble\common\utility.h:#104 \modules\lib\chre\chpp\include\chpp\macros.h:#28 \modules\lib\loramac-node\src\boards\mcu\sam121\hal\utils\include\utils.h:#65 \modules\lib\zcbor\include\zcbor_common.h:#74 \modules\tee\tf-a\trusted-firmware-a\include\lib\utils_def.h:#14 \modules\tee\tf-m\trusted-firmware-m\secure_fw\include\array.h:#15 (a number of different files in TF-M define this)

Every time we include a new module or HAL with this particular approach we have to confirm that this new module isn't trying to define ARRAY_SIZE and if it does then we have to change the module code instead of changing things once in the Zephyr code base.

We should really pay more attention to what gets in as modules and not accept everything. You could have security issues because of module bugs, for example. And... let's not open the debate about the size of modules and what's in there...

[dleach02]

Again, in this particular case with ARRAY_SIZE, I think it is arguable that the issue should be fixed on the Zephyr side and not on the modules we include.

Where there are collisions between modules/libraries we can fix that but in most all of those cases, the modules have the #ifndef framing around the define.

[carlescufi]

Note: see full discussion in https://discord.com/channels/720317445772017664/1014241011989487716/1032623375228608522