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

CMake: fix memory map generation #13985

Merged
merged 2 commits into from
Dec 3, 2020
Merged

CMake: fix memory map generation #13985

merged 2 commits into from
Dec 3, 2020

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Nov 30, 2020 •
edited
Loading

Summary of changes

Fixes #13983
Move linker script and memmap to the function mbed_set_mbed_target_linker_script. Done for both toolchains.

Impact of changes

Migration actions required

Documentation

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@0xc0170
CMake: GCC ARM linker script fix
f97c7bd 
Fixes ARMmbed#13983
Move linker script to the function mbed_set_mbed_target_linker_script.

I also moved memmap as it is needed for an app. The location might not be the best fit,
we will address this in separate pull request.
@0xc0170 0xc0170 mentioned this pull request Nov 30, 2020
Closed
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Nov 30, 2020
@ciarmcom ciarmcom requested review from a team November 30, 2020 11:30
@ciarmcom
Copy link
Member

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

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 30, 2020

This is just fixing the issue reported, we shall look at how linker is defined. I'll create a separate issue for this (related to latest CMake linker report #13981)

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 30, 2020

cc @ladislas

@ladislas
Copy link
Contributor

It works! :) Thanks for the fast fix! 🚀

@hugueskamba
Copy link
Collaborator

hugueskamba commented Nov 30, 2020 •
edited
Loading

Do we need to do the same thing for the ARM toolchain? If so, why not have this PR update both toolchains?

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 30, 2020

Do we need to do the same thing for the ARM toolchain? If so, why not have this PR update both toolchains?

No, as it preprocess sct files. We do not need to do it separately as for GCC ARM. To this applies only to GCC ARM

@hugueskamba
Copy link
Collaborator

Do we need to do the same thing for the ARM toolchain? If so, why not have this PR update both toolchains?

No, as it preprocess sct files. We do not need to do it separately as for GCC ARM. To this applies only to GCC ARM

I was referring to the memmap flags. Do we also need to move the memmap flags we have in ./tools/cmake/toolchains/ARM.cmake to ./CMakeLists.txt like you did in this PR? We probably should for the two toolchains to be in sync.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Dec 1, 2020 •
edited
Loading

Is this the one:

    list(APPEND link_options
        "--map"
    )

To be moved? I can move it. But I would not have moved if we do not need an app name to generate specific map file.

I'll check the naming with armclang.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Dec 1, 2020

ARMClang has already --list=<TARGET_BASE>.map in CMake, it took me a while to figure out why my command to overwrite where memory map is stored did not work.

I moved --map for ARMClang as well as we might get output directed as well if it's removed in ARMClang module in CMake (I crated CMake issue to find out what we can do about it). I would like to use --list to redirect it to the location same as with Gcc Arm but due to the bug, I can't so it stays as it is.

@hugueskamba please review

@hugueskamba
Copy link
Collaborator

Can you please amend the PR title and description?

@0xc0170 0xc0170 changed the title CMake: GCC ARM linker script fix CMake: fix memory map generation Dec 2, 2020
@0xc0170
Copy link
Contributor Author

0xc0170 commented Dec 2, 2020

Can you please amend the PR title and description?

Done, CI started

@mbed-ci
Copy link

mbed-ci commented Dec 3, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit a847ab3 into ARMmbed:master Dec 3, 2020
@mergify mergify bot removed the ready for merge label Dec 3, 2020
@mergify mergify bot added the needs: work label Dec 11, 2020
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ladislas ladislas ladislas approved these changes

@hugueskamba hugueskamba Awaiting requested review from hugueskamba

@rajkan01 rajkan01 Awaiting requested review from rajkan01

Assignees
No one assigned
Labels
needs: work release-version: 6.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CMake: GCC ARM toolchain script specifies the wrong target variable for linker script and map file paths
6 participants
@0xc0170 @ciarmcom @ladislas @hugueskamba @mbed-ci @mbedmain