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

CMake: fix Gcc Arm compiler check #13993

Closed
wants to merge 1 commit into from

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Dec 2, 2020

Summary of changes

Fixes the issue with "Detecting C compiler ABI info". CMake invokes simple check during project().
As we add flags after project is invoked, this check can fail. We should provide init values via toolchain
file to make checks passing and CMake configuration properly done.

We found an issue with Gcc Arm checks only so far. Linker needs nosys to be able to link simple program as we are cross compiling and not providing sys functionality required (like _exit).

I only add to FLAGS_INIT required flags to pass linking when CMake is doing try_compile(). We could move there all linker flags and do the same for ARMClang - unification of linker FLAGS_INIT for both toolchains. Let me know what you think, I can fix that if it receives +1. Or maybe it is fine as it is as CMake does very minimal example testing - it won't test for us what we use - retargeting, wrapping some libs symbols. Therefore the minimum approach to satisfy simple app should be sufficient.

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


Fixes the issue with "Detecting C compiler ABI info". CMake invokes simple check during project().
As we add flags after project is invoked, this check can fail. We should provide init values via toolchain
file to make checks passing and CMake configuration properly done.

We found an issue with Gcc Arm checks only so far. Linker needs nosys to be able to link simple program as we are cross compiling and not providing sys functionality required (like _exit).
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Dec 2, 2020
@ciarmcom ciarmcom requested review from a team December 2, 2020 10:30
@ciarmcom
Copy link
Member

ciarmcom commented Dec 2, 2020

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

@multiplemonomials
Copy link
Contributor

This is better than nothing, and it at least fixes the issues with try compiles under GCC Arm. However, it's still far from ideal: it means that compile and link tests will be performed with the default link flags for the compiler, rather than the correct compile and link flags for this target. This would be a disaster, for example, if Mbed OS ever added support for a 64 bit ARM processor. If you were building for this processor and checked the size of a type like void* from CMake, it would return 4 instead of 8 bytes because all of the CMake checks are running with the default linker flags. My PR (#13987) provides a more complete fix that also addresses this use case. So, I'd still recommend merging the one I submitted.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Dec 2, 2020

This is better than nothing, and it at least fixes the issues with try compiles under GCC Arm. However, it's still far from ideal

We will get there. We are keeping PR 13987 opened to continue the discussion. We can fix the flag linker init as well in there or separate PR.

@rajkan01 @hugueskamba Please review

@0xc0170
Copy link
Contributor Author

0xc0170 commented Dec 2, 2020

I'll close now that I was testing what actually was a problem with this PR and what CMake does. I'll close this as we can get _INIT flags in the other PR fixed.

@0xc0170 0xc0170 closed this Dec 2, 2020
@mergify mergify bot removed needs: review release-type: patch Indentifies a PR as containing just a patch labels Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants