-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update gcc.py for preprocessing in linker script #11254
Conversation
To fix ARMmbed#11214, we need this update. :-)
@jh228, thank you for your changes. |
Is it only GCC , the other toolchains do not need any fix? I can't recall if there was a reason for not having this in. We preprocess linker script (how bootloader gets in or out via start/end address). Is this just fixing adding some symbols rather than adding preprocessing? |
Hi, @0xc0170 |
Yes, can be treated separately just trying to understand the scope of this - if it is generic issue or toolchain specific - what we have missed. This review should help us understand if we missed one toolchain or it's generic and ask the question - was there an intention of keeping this away? cc @ARMmbed/mbed-os-tools |
I reviewed other exporters, they do not export config symbols neither. This means there was an intention perhaps. I could not locate any more information for this neither an issue besides the one mentioned above. @ARMmbed/mbed-os-tools Your review needed. One thing that comes to my mind is extending the cmd by adding config symbols to the ld. |
Hi, @ARMmbed/mbed-os-tools |
I'm sorry but I don't know enough about this to be able to approve or otherwise. |
@mark-edgeworth Can you catch up with Jimmy ? he might know a reason why was not there. |
I'll run CI meanwhile |
@0xc0170 This is not an exporter, so discussion about exporters is not relevant. The other compilers build preprocessing into their linker in some fashion, and make this change unnecessary to IAR and ARMC5/6. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like this is fine.
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @theotherjimmy
Awesome. Thank you for the support! |
To fix #11214, we need this update. :-)
Description
Pull request type
Reviewers
Release Notes