[meta-xilinx] [PATCH 4/4] arm-trusted-firmware: Fix a false positive out of bounds

[Nathan Rossi]

On 19 August 2017 at 02:29, Alistair Francis
<alistair.francis at xilinx.com> wrote:
> This error is received while building
> services/std_svc/psci/psci_common.c: In function 'psci_do_state_coordination':
> services/std_svc/psci/psci_common.c:220:27: error: array subscript is above
> array bounds [-Werror=array-bounds]
>   psci_req_local_pwr_states[pwrlvl - 1][cpu_idx] = req_pwr_state;
>
> Patch 555ebb34db8f3424c1b394df2f10ecf9c1f70901 explains why the error
> is seen and supposibly includes a fix. As the fix appears not to work,
> let's disable the error checking.

Not sure how that change was ever a fix? Since assert() will always be
"(void)0" when not debugging. Essentially making the assert a empty
statement.

https://github.com/ARM-software/arm-trusted-firmware/blob/555ebb34db8f3424c1b394df2f10ecf9c1f70901/include/stdlib/assert.h#L49

NDEBUG is defined here for release builds.
https://github.com/ARM-software/arm-trusted-firmware/blob/555ebb34db8f3424c1b394df2f10ecf9c1f70901/Makefile#L142


And master still has the same issue since ENABLE_ASSERTIONS is default
to "DEBUG", except for specific boards

https://github.com/ARM-software/arm-trusted-firmware/blob/master/include/lib/stdlib/assert.h
https://github.com/ARM-software/arm-trusted-firmware/blob/master/Makefile#L36

e.g.
https://github.com/ARM-software/arm-trusted-firmware/blob/38fe380a9a04d0b4356123ed202abf064ec69cbf/plat/nvidia/tegra/platform.mk#L14


But something like this works.

diff --git a/lib/psci/psci_common.c b/lib/psci/psci_common.c
index 68cdd6eb12..a8c94215c7 100644
--- a/lib/psci/psci_common.c
+++ b/lib/psci/psci_common.c
@@ -394,6 +394,8 @@ void psci_do_state_coordination(unsigned int end_pwrlvl,
        plat_local_state_t target_state, *req_states;

        assert(end_pwrlvl <= PLAT_MAX_PWR_LVL);
+       if (end_pwrlvl > PLAT_MAX_PWR_LVL)
+               return;
        parent_idx = psci_cpu_pd_nodes[cpu_idx].parent_node;

        /* For level 0, the requested state will be equivalent

>
> Signed-off-by: Alistair Francis <alistair.francis at xilinx.com>
> ---
>  recipes-bsp/arm-trusted-firmware/arm-trusted-firmware.inc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/recipes-bsp/arm-trusted-firmware/arm-trusted-firmware.inc b/recipes-bsp/arm-trusted-firmware/arm-trusted-firmware.inc
> index 759cc10..747874d 100644
> --- a/recipes-bsp/arm-trusted-firmware/arm-trusted-firmware.inc
> +++ b/recipes-bsp/arm-trusted-firmware/arm-trusted-firmware.inc
> @@ -24,11 +24,12 @@ ATF_BASE_NAME[vardepsexclude] = "DATETIME"
>  COMPATIBLE_MACHINE = "zynqmp"
>  PLATFORM_zynqmp = "zynqmp"
>
> +CFLAGS_append = " -Wno-error=array-bounds"
> +
>  # requires CROSS_COMPILE set by hand as there is no configure script
>  export CROSS_COMPILE="${TARGET_PREFIX}"
>
>  # Let the Makefile handle setting up the CFLAGS and LDFLAGS as it is a standalone application
> -CFLAGS[unexport] = "1"

Changing this might cause some issues (the most obvious is -O2 is
passed via CFLAGS where as ATF uses -Os by default). Have you tested
the resulting build on a hardware target? It does seem to work fine in
QEMU.

I think this might work better as a patch to the atf Makefile, setting
-Wno-error=array-bounds on TF_CFLAGS or similar.

Regards,
Nathan