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

[Alistair Francis]

On Sun, Aug 20, 2017 at 9:38 PM, Nathan Rossi <nathan at nathanrossi.com> wrote:
> 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

Ah, that explains it. I just took the commit message at face value and
assumed it should have fixed it.

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

This fixes it for me. I won't bother with any Makefile changes as I
think a code fix is better then disabling error checking.

I'm going to send this patch internally and upstream to fix the issue.
I'll add your SOB line to the patch. Let me know if that's an issue.

>
>>
>> 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
> --
> _______________________________________________
> meta-xilinx mailing list
> meta-xilinx at yoctoproject.org
> https://lists.yoctoproject.org/listinfo/meta-xilinx