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.

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

Fix MBED_RAM_START/MBED_RAM_SIZE symbol generation #10008

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
8 participants
@ccli8
Copy link
Contributor

commented Mar 8, 2019

Description

This PR tries to fix #9716 with build tool-generated MBED_RAM_START/MBED_RAM_SIZE symbols passed to C/C++ and linker files. It has the effects:

  1. Fix MBED_RAM_START/MBED_RAM_SIZE symbols are not passed to C/C++ file (see build tool-generated .profile-c/.profile-cxx) when there are target.mbed_ram_start/target.mbed_ram_size overrides.
  2. Avoid duplicate MBED_RAM_START/MBED_RAM_SIZE symbols passed to linker file (see build tool-generated .profile-ld).

Pull request type

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

Related issue or PR

#10004
#9716

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-maintainers Mar 8, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@ccli8, thank you for your changes.
@ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-tools Mar 8, 2019

tools/toolchains/mbed_toolchain.py Outdated
@@ -872,7 +872,7 @@ def add_regions(self):
"s" if len(regions) > 1 else "",
", ".join(r.name for r in regions)
))
self._add_all_regions(regions, "MBED_RAM")
self._add_all_regions(regions, "MBED_RAM_APP")

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 8, 2019

Contributor

I'm not keen to make this change yet. While we certainly will get to a point where we have application's ram separated from physical ram, we have no configuration for that use right now. Your proposal to use mbed_ram_start and mbed_ram_size for this purpose will:

  1. Break any project that depends on those configuration settings current behavior.
  2. Be inconsistent with mbed_rom_start and mbed_rom_size

This comment has been minimized.

Copy link
@ccli8

ccli8 Mar 9, 2019

Author Contributor

This is to fix duplicate MBED_RAM_START symbol in .profile-ld. Would you point out another solution to it?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Based on the description in #9716 I would expect this PR to rename the C flag APPLICAITON_RAM_START to MBED_RAM_START.

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

@theotherjimmy Please correct me for my understanding:

  1. target.mbed_ram_start is for overriding physical ram from cmsis pack memory spec, just like target.mbed_rom_start is for physical rom.
  2. Currently, there's no target.mbed_ram_app_start configuration option to override application ram.
  3. MBED_RAM_START is generated from cmsis pack memory spec or target.mbed_ram_start override and means physical ram start.

Then to address #9716, how to make MBED_RAM_START symbol available both to c/c++ and linker files and avoid duplicate?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

@ccli8 I would do something like:

  • remove the if block from 868 - 877
  • expand the memory list in 885 to include RAM defines.

Does that help?

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fix-ram-symbol branch Mar 11, 2019

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@theotherjimmy Follow your suggestion but implement with another approach by skipping duplicate MBED_RAM_START in .profile-ld.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@ccli8 Could you update the PR description to reflect the affects of this PR? Nice diff BTW. Clever.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

@ccli8 Reading through the related code, I think this may have the unintended side affect of never passing CMSIS defined RAM to the linker. Sorry for being blunt, I can't think of a better way of wording this right now. How have you tested this?

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I think this may have the unintended side affect of never passing CMSIS defined RAM to the linker

@theotherjimmy To verify it, I check build-tool generated symbols in .profile-c and .profile-ld:

  1. When there are no target.mbed_ram_start/target.mbed_ram_size overrides, MBED_RAM_START/MBED_RAM_SIZE reflect CMSIS defined RAM.
  2. When there are target.mbed_ram_start/target.mbed_ram_size overrides, MBED_RAM_START/MBED_RAM_SIZE reflect these RAM overrides.
  3. MBED_RAM_START/MBED_RAM_SIZE are always generated in both .profile-c and .profile-ld.
  4. MBED_RAM_START/MBED_RAM_SIZE are not duplicate in both .profile-c and .profile-ld.
@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Could you update the PR description to reflect the affects of this PR?

@theotherjimmy Update done

@theotherjimmy
Copy link
Contributor

left a comment

Diff looks great. Thanks!

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Mar 12, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Needs rebase

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Mar 16, 2019

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fix-ram-symbol branch to a235019 Mar 18, 2019

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@0xc0170 Rebase done

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Mar 18, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 18, 2019

Test run: SUCCESS

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

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fix-ram-symbol branch from a235019 to 39e92f8 Mar 21, 2019

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

@0xc0170 Rebase done

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 21, 2019

Test run: FAILED

Summary: 6 of 9 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-ARMC5
  • jenkins-ci/mbed-os-ci_build-ARMC6
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR8
  • jenkins-ci/mbed-os-ci_build-IAR8
@alekla01

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

CI Restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 21, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

io serial for one device (networking tests), will restart

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@ccli8 Let's wait until we finalize rc4 , to avoid rebasing/CI as we might have another config fix. Will go to CI as soon as 5.12 rc4 is completed. Sorry about it, this week landed lot of fixes to config for 5.12.0 resulting in conflicts with this PR.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Rebase when able. RC4 has since been made and merged.

Fix MBED_RAM_START/MBED_RAM_SIZE symbol generation
1. Fix MBED_RAM_START/MBED_RAM_SIZE are not generated when there are
   target.mbed_ram_start/target.mbed_ram_size overrides
2. Fix MBED_RAM_START/MBED_RAM_SIZE are duplicated.

@ccli8 ccli8 force-pushed the OpenNuvoton:nuvoton_fix-ram-symbol branch from 39e92f8 to da7035e Mar 26, 2019

@ccli8

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@cmonr Rebase done.

@cmonr cmonr added needs: CI and removed needs: work labels Mar 26, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 26, 2019

Test run: SUCCESS

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

cmonr added a commit to cmonr/mbed-os that referenced this pull request Mar 26, 2019

Merge pull request ARMmbed#10008 from OpenNuvoton/nuvoton_fix-ram-symbol
Fix MBED_RAM_START/MBED_RAM_SIZE symbol generation

@cmonr cmonr merged commit a6c9c8c into ARMmbed:master Mar 27, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10344 cycles (+29 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 27, 2019

@ccli8 ccli8 deleted the OpenNuvoton:nuvoton_fix-ram-symbol branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.