ARMmbed / mbed-os-tf-m-regression-tests

An Mbed OS application that runs the TF-M regression tests
2 stars 12 forks source link

Workaround for regression test build with ARMCLANG #60

Closed LDong-Arm closed 3 years ago

LDong-Arm commented 3 years ago

Fixes: #52

Mbed applications, including the regression tests triggered by main(), are run on the non-secure sides. In Mbed OS we only enable the non-secure timer. However, plat_test.c in trusted-firmware-m defines tests for both secure and non-secure timers. The secure timer tests are not eventually used by the final application, and the GCC's linker handles this to links successfully.

But it's a known behaviour that the Arm Compiler's linker (armlink) does not resolve undefined-but-unused symbols, and it fails to link the tests and complains the secure timer (CMSDK_TIMER0_DEV_S) is undefined:

Link: mbed-os-tf-m-regression-tests
[Warning] @0,0: L3912W: Option 'legacyalign' is deprecated.
[Error] @0,0: L6218E: Undefined symbol CMSDK_TIMER0_DEV_S (referred from ./test/lib/TOOLCHAIN_ARMC6/libtfm_non_secure_tests.ar(plat_test.o)).

As a workaround, this PR enables CMSDK_TIMER0_DEV_S by defining the macro CMSDK_TIMER0_S.

@ARMmbed/mbed-os-security In trusted-firmware-m's vanilla device_cfg.h, both non-secure and secure timers are always defined. In Mbed OS we only define what we need (the non-secure one), but just to fix ARMCLANG build of the tests, does it make sense to define both timers in Mbed OS, or use this PR?

@evedon @jainvikas8 This is ready for review, thanks.

jainvikas8 commented 3 years ago

@ARMmbed/mbed-os-security In trusted-firmware-m's vanilla device_cfg.h, both non-secure and secure timers are always defined. In Mbed OS we only define what we need (the non-secure one), but just to fix ARMCLANG build of the tests, does it make sense to define both timers in Mbed OS, or use this PR?

Can we have this workaround somewhere else apart from .json file? As this is only related to ARM compiler and we may have to raise an issue with the compiler team here?

LDong-Arm commented 3 years ago

Can we have this workaround somewhere else apart from .json file? As this is only related to ARM compiler and we may have to raise an issue with the compiler team here?

As far as I know, .json files are the only places to define macros without editing the code. The lack of handling of undefined-but-unused symbols has been raised numerous times already.

jainvikas8 commented 3 years ago

Can we have this workaround somewhere else apart from .json file? As this is only related to ARM compiler and we may have to raise an issue with the compiler team here?

As far as I know, .json files are the only places to define macros without editing the code. The lack of handling of undefined-but-unused symbols has been raised numerous times already.

I think we should log this as an issue with the compiler team here... Accept this PR as a workaround. Any thoughts @urutva @Patater?

evedon commented 3 years ago

As far as I know, .json files are the only places to define macros without editing the code. The lack of handling of undefined-but-unused symbols has been raised numerous times already.

Given that the issue has been raised several times and that the compiler team is aware of it, we should accept the workaround

urutva commented 3 years ago

@jainvikas8 @LDong-Arm @evedon Unfortunately the way TF-M regression tests are architected is causing this issue. The test plat_test.o which is part of regression tests suite for non-secure is using a secure time even though in a separate function but still object remains the same. Hence the ArmClang linker error.

However, we don't include the regression tests in the final binary checked-in to mbed-os, therefore this issue will not be seen by mbed-os applications using TF-M and no need of workaround.

I can't think of other way to fix this issue in regression test without enabling the macro CMSDK_TIMER0_S either by editing the code or json. This workaround looks ok to me as this is only intended for regression test and is never needed for mbed-os applications using TF-M.

LDong-Arm commented 3 years ago

See https://github.com/ARMmbed/trusted-firmware-m/commit/aaca620d33f57771ebc3829950222270e89b32f4

@urutva made it for Musca A initially, in my opinion we should make the same change for Musca B1 and S1 instead of this PR.