Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deduplicate IAR MBED_ROM__xxxx defines #10157

Merged
merged 3 commits into from
Mar 20, 2019
Merged

Deduplicate IAR MBED_ROM__xxxx defines #10157

merged 3 commits into from
Mar 20, 2019

Conversation

theotherjimmy
Copy link
Contributor

@theotherjimmy theotherjimmy commented Mar 19, 2019 •
edited
Loading

Description

PR #10113 allowed a porter or application author to configure
secure or non-secure rom through the use of:

  • target.secure-rom-start
  • target.secure-rom-size
  • target.non-secure-rom-start
  • target.non-secure-rom-size

and use these in managed bootloader modes and report them as ROM.
This created a duplicate MBED_ROM_START and MBED_ROM_SIZE
define on the IAR linker command line.

Further, the overrides:

  • target.secure-ram-start
  • target.secure-ram-size
  • target.non-secure-ram-start
  • target.non-secure-ram-size

Would create further command line defines that conflicted with RAM
reporting.

This PR removes the duplicated command line arguments.

Fixes #10153

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 19, 2019

@mikisch81 Please can you test?

@mikisch81
Copy link
Contributor

I am OOO all day today, not near devices.
@alzix @davidsaada @orenc17 anyone with LPC55S69 device and iar8 toolchains at hand?

mikisch81
mikisch81 reviewed Mar 19, 2019
View reviewed changes
tools/toolchains/mbed_toolchain.py
@@ -929,15 +929,11 @@ def add_linker_defines(self):
flags2params = {}
if self.target.is_PSA_non_secure_target:
flags2params = {
"MBED_ROM_START": "target.non-secure-rom-start",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how will these parameter be passed to the linker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With other code that was duplicating this define. It was duplicated this is the one that's no longer needed.

@orenc17
Copy link
Contributor

orenc17 commented Mar 19, 2019

@theotherjimmy are you sure that it won't break targets without bootloader?

@theotherjimmy
Copy link
Contributor Author

@orenc17 Yes. ROM is always reported as of 5.11.

@alzix
Copy link
Contributor

alzix commented Mar 19, 2019

tested this PR on the target with @davidsaada help.
We see regression when building secure image with new changes - the board does not boot regardless to toolchain used to build nonsecure image
In addition the board does not boot when building nonsecure image with IAR and using default prebuilt secure image. GCC_ARM seems to work fine though.

@theotherjimmy
Copy link
Contributor Author

@alzix Thanks. Is this regression compared to master or compared to some other point?

@davidsaada
Copy link
Contributor

@alzix Thanks. Is this regression compared to master or compared to some other point?

Sorry - just double checked. It was compared to master, but behaves just the same. No harm to secure side, but no improvement to non secure side on IAR toolchain.

@orenc17
Copy link
Contributor

orenc17 commented Mar 19, 2019

@theotherjimmy i can confirm that MBED_RAM_START/SIZE also has duplications

@orenc17
Copy link
Contributor

orenc17 commented Mar 19, 2019

@theotherjimmy to be clear i'm talking about GCC_ARM

@theotherjimmy
Copy link
Contributor Author

@orenc17 Good catch. I'll deduplicate the RAM as well.

@theotherjimmy
Copy link
Contributor Author

It looks like the RAM duplication is possibly causing bugs as they are not the same value.

@theotherjimmy
Copy link
Contributor Author

@orenc17 New commit should deduplicate RAM defines as well.

@cmonr
Copy link
Contributor

cmonr commented Mar 19, 2019

CI started

@orenc17
Copy link
Contributor

orenc17 commented Mar 19, 2019 •
edited
Loading

@theotherjimmy looks and build fine on GCC_ARM
will do a full regression check in the morning

Update: CI build log looks ok

@mbed-ci
Copy link

mbed-ci commented Mar 19, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

@cmonr
Copy link
Contributor

cmonr commented Mar 19, 2019

Hold until @orenc17 does his checks: #10157 (comment)

@mikisch81
Copy link
Contributor

@ARMmbed/mbed-os-maintainers I can verify that IAR builds and runs OK with this PR changes.

Ran all the PSA related tests:

| target          | platform_name | test suite                                                   | result | elapsed_time (sec) | copy_method |
|-----------------|---------------|--------------------------------------------------------------|--------|--------------------|-------------|
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_attestation-test_a001 | OK     | 32.58              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c001      | OK     | 17.14              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c002      | OK     | 17.92              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c003      | OK     | 17.78              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c004      | OK     | 17.85              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c005      | OK     | 17.6               | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c006      | OK     | 17.81              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c007      | OK     | 17.76              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c008      | OK     | 17.72              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c009      | OK     | 17.24              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c010      | OK     | 17.4               | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c011      | OK     | 17.03              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c012      | OK     | 17.29              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c013      | OK     | 17.29              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c014      | OK     | 17.3               | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c015      | OK     | 17.13              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c016      | OK     | 17.53              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c017      | OK     | 17.42              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c018      | OK     | 17.05              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c019      | OK     | 17.05              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c020      | OK     | 17.43              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c021      | OK     | 17.04              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c022      | OK     | 17.59              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c023      | OK     | 17.27              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c024      | OK     | 17.6               | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c025      | OK     | 17.71              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c026      | OK     | 17.63              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c027      | OK     | 17.18              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c028      | OK     | 17.15              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c029      | OK     | 17.71              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c030      | OK     | 17.12              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c031      | OK     | 17.18              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c032      | OK     | 17.93              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c033      | OK     | 17.95              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c034      | OK     | 17.18              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c035      | OK     | 17.15              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c036      | OK     | 17.69              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c037      | OK     | 17.83              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c038      | OK     | 17.44              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c039      | OK     | 18.4               | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c040      | OK     | 20.07              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c041      | OK     | 19.63              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c042      | OK     | 20.0               | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_crypto-test_c043      | OK     | 21.75              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s001         | OK     | 17.44              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s002         | OK     | 17.44              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s004         | OK     | 17.01              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s005         | OK     | 16.99              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s006         | OK     | 16.93              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s007         | OK     | 17.12              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s008         | OK     | 17.08              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s009         | OK     | 17.23              | default     |
| LPC55S69_NS-IAR | LPC55S69      | components-target_psa-tests-compliance_its-test_s010         | OK     | 16.91              | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-mbed-crypto-sanity                                     | OK     | 19.77              | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-psa-attestation                                        | OK     | 20.11              | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-psa-crypto_access_control                              | OK     | 20.5               | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-psa-crypto_init                                        | OK     | 17.17              | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-psa-entropy_inject                                     | OK     | 18.18              | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-psa-its_ps                                             | OK     | 18.6               | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-psa-spm_client                                         | OK     | 22.65              | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-psa-spm_server                                         | OK     | 20.17              | default     |
| LPC55S69_NS-IAR | LPC55S69      | tests-psa-spm_smoke                                          | OK     | 16.7               | default     |
mbedgt: test suite results: 62 OK

@0xc0170 0xc0170 merged commit da3a759 into ARMmbed:master Mar 20, 2019
@theotherjimmy theotherjimmy mentioned this pull request Mar 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonr cmonr cmonr approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@mikisch81 mikisch81 mikisch81 approved these changes

@orenc17 orenc17 orenc17 approved these changes

@yossi2le yossi2le Awaiting requested review from yossi2le

Assignees
No one assigned
Labels
release-version: 5.12.0-rc4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@theotherjimmy @0xc0170 @mikisch81 @orenc17 @alzix @davidsaada @cmonr @mbed-ci