sys: util: Prevent multiple definitions of ARRAY_SIZE

[mfischer]

Wrap ARRAY_SIZE macro with an #ifndef ARRAY_SIZE to prevent multiple definition issues when dealing with third party libraries that have their own definitions.

Fixes #51371

Signed-off-by: Moritz Fischer moritzf@google.com

[dcpleung]

Similar PR was closed without merging: #49831

[mfischer]

Similar PR was closed without merging: #49831

What is the reasoning to not allow this, but do the same thing for other macros? Every single implementation of ARRAY_SIZE I've seen in the wild looked exactly the same, and expecting every library out there to carry patches for Zephyr is pretty unreasonable in my book.

[dcpleung]

Please read the comments of the linked PR and see if you can satisfy what has been voiced there.

[cfriedt]

Add tests here, please.

https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/unit/util/test.inc

Unless it can be proven that there is some sort of security threat by doing so, I see absolutely no reason not to ack here.

On a side note, we probably do want to have a non-unit test that explicitly causes a crash to validate failure modes. IIRC, it is not possible to do that with a zephyr "unit test" (because they run as native_posix).

[mfischer]

main/tests/unit/util/test.inc

There are tests in there that use ARRAY_SIZE. I can add a

void run_ARRAY_SIZE(void)
{
  size_t array[] = {1,2,3,4};
  zassert_equal(4, ARRAY_SIZE(array));
}

If you want something explicit.

[de-nordic]

This has been discussed here #49831, as @dcpleung has noted, and abandoned by @nordicjm, as a result of the discussion.

Making this conditional may make the macro behave differently across compilation units, when different definitions are provided.
Do we really want to allow that?

[cfriedt]

This has been discussed here #49831, as @dcpleung has noted, and abandoned by @nordicjm, as a result of the discussion.

It's been discussed in a few places a number of times.

Making this conditional may make the macro behave differently across compilation units, when different definitions are provided. Do we really want to allow that?

This is for 3rd-party code compatibility, so simply avoid a compiler warning that will be promoted to error in twister.

We all know the correct fix for this - i.e. use a namespaced Z_ARRAY_SIZE(). However, there is constant pushback against that.

As an intermediate step to downstream projects dropping ARRAY_SIZE()

1. Replace it everywhere in Zephyr with Z_ARRAY_SIZE
2. create a CONFIG_DEFINE_ARRAY_SIZE that defaults to y, such that

#ifdef CONFIG_DEFINE_ARRAY_SIZE
#define ARRAY_SIZE(x) Z_ARRAY_SIZE(x)
#endif

3. Add deprecation for ARRAY_SIZE
4. Next release, remove the CONFIG / define.

[de-nordic]

This is for 3rd-party code compatibility, so simply avoid a compiler warning that will be promoted to error in twister.

Avoiding warning does not fix problem it makes it just creep in the shadows.
Avoiding warnings can be done by running sed/awk in pipeline and filtering them out: works the same, does not fix the problem.
I have already spent some time in my life on debugging things that could have been found in warnings, warnings suppressed because they would cause warning count go above level accepted by quality - no difference from here.

I have also gone through something like

#undef SUCCESS
#define SUCCESS 0

which is basically "happy debugging, suckers" case from bash.org, that makes me reluctant to allow different definitions depending on the order of header inclusions.

We all know the correct fix for this - i.e. use a namespaced Z_ARRAY_SIZE(). However, there is constant pushback against that.

Then, the warning is the argument in the discussion.

As an intermediate step to replacing ARRAY_SIZE() everywhere, deprecate it, create a CONFIG_DEFINE_ARRAY_SIZE that defaults to y, such that

#ifdef CONFIG_DEFINE_ARRAY_SIZE
#define ARRAY_SIZE(x) Z_ARRAY_SIZE(x)
#endif

Eventually, removing the CONFIG / define.

That would be great.

[gmarull]

We all know the correct fix for this - i.e. use a namespaced Z_ARRAY_SIZE(). However, there is constant pushback against that.

I wish Zephyr had a namespace. IMO, it's a mistake not to do that when using lots of 3rd party code in-tree (HALs et al.).

[de-nordic]

I wish Zephyr had a namespace. IMO

Me too.

it's a mistake not to do that when using lots of 3rd party code in-tree (HALs et al.).

Yes. Having our own namespace kinda lets us move out of the way, in most of cases as ARRAY_SIZE(TM)(R)(C) is very generic name.

I must admit that the 3rd party code is not without blame, because using generic names for things is just asking for collisions.
It is obvious that user needs to include headers of some lib to use its API, but if these bring redefinitions of things like ARRAY_SIZE, OK, FALSE, SUCCESS or whatever you thing about now, interface of the library probably exposes something that should be internal, is missing prefixes, or contains inclusions of headers it should not take.

[cfriedt]

@mfischer - well, how would you feel ripping the bandaid off?

[mfischer]

@mfischer - well, how would you feel ripping the bandaid off?

Sure, this week is busy. I'll see when I get around to it. During -rc nothing will be merged anyways :)

[stephanosio]

@andyross What do you mean by collision? Current version of this PR undefs any preceding definitions of macros and then defines them. Zephyr wins.

What if you include another (third-party) header that does the same thing after the Zephyr header?

These re-definitions only fail because the re-definition is different from the original definition (compiler will not complain for the identical re-definition) -- that IS a problem.

It would be best to fix this in the other code

Agreed.

But if we have to do it here, pick one of the Zephyr prefixes (SYS_ would probably be best) and use that.

I am no longer sure we need that. sys/util.h is, IMHO, internal util header; other systems to not bother add prefixes to their definitions.

I think SYS_ is still useful. By prefixing all these macros and "claiming" the SYS_ namespace (reasonable since Zephyr is an operating system), we can then clearly say any third-party code defining anything that starts with SYS_ are wrong.

[cfriedt]

But if we have to do it here, pick one of the Zephyr prefixes (SYS_ would probably be best) and use that.

I kind of like Z_ for a namespace prefix and we seem to be using that all over the place already.

[gmarull]

In practice this means ARRAY_SIZE implementation will depend on the including order: BAD. Note that since Zephyr seems to be worried about safety, this violates MISRA 20.5 (advisory) as well.

I think we should either:

• Fix offending libraries/3rd party code
• Use a namespace in Zephyr, to protect us from 3rd party code.

[de-nordic]

In practice this means ARRAY_SIZE implementation will depend on the including order: BAD. Note that since Zephyr seems to be worried about safety, this violates MISRA 20.5 (advisory) as well.

Valid arguments.

I think we should either:

* Fix offending libraries/3rd party code

Yes.

* Use a namespace in Zephyr, to protect us from 3rd party code.

There is always chance for collision, and system should not move out of the way.
Choosing SYS_ or Z_ prefix will fix a problem until other system gets the same idea and some interface header for some library, hal or other thing does the #ifndef/#define thing in interface header again.

Only way to fix header order problem is to avoid having definitions, in headers, that should not be there.

sys/util.h is included specifically for the definitions it provides, so it is not guilty of redefinitions.

Question: how many Zephyr headers include sys/util.h for no reason?

[cfriedt]

I will also +1 this PR if it evolves into a tree-wide change to Z_ARRAY_SIZE() (i.e. if we namespace it) as long as we deprecate ARRAY_SIZE() and activate it with a y-defaulted Kconfig. That way, if a particular build sees a conflict in the definition of ARRAY_SIZE(), it can just be deactivated with a Kconfig. Also does not break modules, since it is y-defaulted.

This above work is practically as simple as running a grep, awk, and sed one-liner.

[dleach02]

I'm stepping into this thread since the NXP HAL is one of the modules/libraries that @gmarull is trying to change. While the change proposed is not that big, the problem I have is that I have to propagate this change back to the source or I'm burdened with the technical debt of maintaining a patch that has to be applied.

Previously, it was header include order that prevented this problem from happening and partially because the NXP HAL definition wrapped the ARRAY_SIZE with an #ifndef. We don't seem to want to do that with the Zephyr version to, I assume, ensure that the system build gets the functionality being defined by our version.

The other solution being discussed is to fix this via a namespace prefix.

My vote is for a namespace fix. ARRAY_SIZE is a super common define in the embedded space. Below is a quick scan of our current modules/libraries on the frequency of its definition so rather then forcing changes on every HAL/Module/Lib we pull into Zephyr we should just ensure a separate name space for Zephyr's implementation of these various common defines (or just use the #ifndef wrapper).

\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)

[de-nordic]

The zcbor has already been updated with replacement of ARRAY_SIZE by ZCBOR_ARRAY_SIZE
https://github.com/zephyrproject-rtos/zcbor/blob/main/include/zcbor_common.h#L85-L87
The mcuboot has not yet been updated with the new code.
The zcbor has a good reason to export the ZCBOR_ARRAY_SIZE because it is used by macro that is part of an API here:
https://github.com/zephyrproject-rtos/zcbor/blob/main/include/zcbor_encode.h#L278-L281

Unless there is good reason to make such macro public, as in case of zcbor, it should not be visible via hal/lib/module interface and should be guarded by #ifndef.

[carlescufi]

See conversation here: https://discord.com/channels/720317445772017664/1014241011989487716/1032623375228608522

[carlescufi]

The approach in this PR has been rejected in the different forums where this has been discussed, and instead a completely different strategy is under review:
#51963

[yperess]

LGTM on the tests/ end

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[de-nordic]

@mfischer I think this can be closed since #51963 went through.

[de-nordic]

Can we close this now?