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 LTO issue with minimal-printf #12712

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Conversation

jamesbeyond
Copy link
Contributor

Summary of changes

Minimal-printf is enabled by default now.
However, LTO have some issues when build with minimal-printf.
This PR update lto profile including all printf related wrap functions.
So the Nightly LTO build job will be passed

Impact of changes

Migration actions required

Documentation

Not Requed

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


@ciarmcom ciarmcom requested review from a team March 29, 2020 21:00
@ciarmcom
Copy link
Member

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

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 30, 2020

So the Nightly LTO build job will be passed

This is one of the things for me to look at today, if this fixes it, nightly build can be green again 💯

@0xc0170 0xc0170 requested a review from a team March 30, 2020 07:21
@mergify mergify bot added needs: CI and removed needs: review labels Mar 30, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 30, 2020

CI started

@mergify mergify bot added needs: work and removed needs: CI labels Mar 30, 2020
@mbed-ci
Copy link

mbed-ci commented Mar 30, 2020

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-pytest
  • jenkins-ci/mbed-os-ci_greentea-test

@jamesbeyond
Copy link
Contributor Author

Seems the failures are not related, maybe due to a flaky target in CI?

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 30, 2020

tests restarted (if the target is again io serial, we will request test to restart the target or remove)

Copy link
Contributor

@mark-edgeworth mark-edgeworth left a comment

Choose a reason for hiding this comment

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

Needs testing properly

tools/profiles/extensions/lto.json Show resolved Hide resolved
@jamesbeyond
Copy link
Contributor Author

Needs testing properly

Yes, they have been tested locally, everything is passed as expected.
What I mean in my last comment was: I knew for sure those linker options are needed when LTO is enabled.

But what I don't know is when LTO is not in use, still apply those option, is there any side effect? reading gcc docs didn't give very clear answers. That why I suggest leaving these options in this JSON file. rather than put into the build scripts, So these options will only get applied when LTO in use

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 31, 2020

Client restarted

@0xc0170 0xc0170 requested a review from kjbracey March 31, 2020 08:57
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 31, 2020

But what I don't know is when LTO is not in use, still apply those option, is there any side effect? reading gcc docs didn't give very clear answers. That why I suggest leaving these options in this JSON file. rather than put into the build scripts, So these options will only get applied when LTO in use

I found a note "Only undefined references are replaced by the linker." in https://sourceware.org/binutils/docs/ld/Options.html. If there are no undefined ref, nothing to replace. Is that case when minimal printf not enabled?

@jamesbeyond
Copy link
Contributor Author

I found a note "Only undefined references are replaced by the linker." in https://sourceware.org/binutils/docs/ld/Options.html. If there are no undefined ref, nothing to replace. Is that case when minimal printf not enabled?

if minimal-printf is not enabled, then we are not going to give --wrap option to those printf() functions, hence all the undefined symbol will be resolved to stand library. so all the __wrap_printf symbols will not be used anyway

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 1, 2020

@evedon @mark-edgeworth All good here to merge it today?

@mark-edgeworth
Copy link
Contributor

ok with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants