ARM-software / tf-issues

Issue tracking for the ARM Trusted Firmware project
37 stars 16 forks source link

Compilation error "error: array subscript is above array bounds" #347

Closed sorenb-xlnx closed 8 years ago

sorenb-xlnx commented 8 years ago

I see the above mentioned error under some circumstances. It seems the condition is something like: debug must be enabled and PLAT_MAX_PWR_LVL=1. In that case compilation fails with

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

This can be reproduced on the current master branch (84091c48167c), by executing:

make DEBUG=1 PLAT=fvp bl31

I suspect this to be a false positive, as I don't see this with all toolchains. I just downloaded the latest aarch64 toolchain from linaro.org (aarch64-linux-gnu-gcc (Linaro GCC 5.1-2015.08) 5.1.1 20150608), which shows this error. Before that I used 'aarch64-linux-gnu-gcc (Linaro GCC 2014.11) 4.9.3 20141031 (prerelease)', which does not produce this error/warning.

I get around this issue by applying the below diff (let me know if you wanted a PR):

diff --git a/services/std_svc/psci/psci_common.c b/services/std_svc/psci/psci_common.c
index 465c5fd9b3a5..f448ed253b34 100644
--- a/services/std_svc/psci/psci_common.c
+++ b/services/std_svc/psci/psci_common.c
@@ -217,6 +217,7 @@ static void psci_set_req_local_pwr_state(unsigned int pwrlvl,
                                         plat_local_state_t req_pwr_state)
 {
        assert(pwrlvl > PSCI_CPU_PWR_LVL);
+       assert(pwrlvl <= PLAT_MAX_PWR_LVL);
        psci_req_local_pwr_states[pwrlvl - 1][cpu_idx] = req_pwr_state;
 }
soby-mathew commented 8 years ago

Thanks Soren for letting us know of the issue. Yes, it seems that the latest version of GCC is having additional static analysis and the error is a false positive. We would like to fix this issue and analyse if any other internal PSCI helpers would benefit from additional assert checks.

sorenb-xlnx commented 8 years ago

It's actually possible to trigger this with the 4.9 toolchain too. It does not show at -Os, but at -O3. So, even with 4.9, this results in the same error/warning:

DEFINES=-O3 make DEBUG=1 PLAT=fvp bl31

Do you want me to open a PR with my proposed "fix"? Or are you gonna work on it further on your side?

soby-mathew commented 8 years ago

Thanks for the update. We will be fixing this issue from our side.

soby-mathew commented 8 years ago

After analysing the callers of psci_do_state_coordination(), I couldn't find anything peculiar that caused the compiler to trigger the error. Adding an assert in psci_do_state_coordination() also seems to resolve the error which I would prefer since it covers both psci_set_req_local_pwr_state() and psci_get_req_local_pwr_states().

diff --git a/services/std_svc/psci/psci_common.c b/services/std_svc/psci/psci_common.c
index 465c5fd..8a2b81c 100644
--- a/services/std_svc/psci/psci_common.c
+++ b/services/std_svc/psci/psci_common.c
@@ -393,6 +393,7 @@ void psci_do_state_coordination(unsigned int end_pwrlvl,
        unsigned int start_idx, ncpus;
        plat_local_state_t target_state, *req_states;

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

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

Please let me know if you are happy to do the fix and raise a PR for the same or we could do it from our end.

sorenb-xlnx commented 8 years ago

If you have a better solution, just go ahead and apply it. There's no reason to make things overly complicated.