Skip to content

CMake: Use armclang for ASM files #13534

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

Merged
merged 3 commits into from
Sep 3, 2020

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Sep 2, 2020

Summary of changes

The ARM Assembler armasm has a bug processing macro with array values. It omits the , delimiter which results in syntax errors.
As it is being deprecated, Arm Compiler armclang is the recommended tool to compile ARM Assembly Language.
However, given that the various Mbed ARM TOOLCHAIN assembly code are written using the legacy armasm syntax, the command line option -masm needs to be passed to armclang so it can select the correct assembler for the input assembly source files. This has been successfully tested with mbed-os-example-blinky.
The option was set to auto as there are circumstances where the auto-detection might select the wrong assembler for the input file. One of thos circumstances is if the input file is a .S file that requires preprocessing, this is the case for a number of Mbed Assembly source file.
Note: The current release of CMake (3.18.2) has a bug for armclang where it does not append target definitions to the ASM compiler command.
This was reported, subsequentenly fixed and scheduled to be part of the next CMake release.
Thanks @hugueskamba for fixing it.

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

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

Reviewers


Fixes the issue of armasm, passing -DMACRO={0,0} - fails to parse.

We use armclang frontend but passing -masm=armasm.
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Sep 2, 2020
@ciarmcom ciarmcom requested review from maclobdell and a team September 2, 2020 15:00
@ciarmcom
Copy link
Member

ciarmcom commented Sep 2, 2020

@0xc0170, thank you for your changes.
@maclobdell @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-core @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-mesh @ARMmbed/mbed-os-pan @ARMmbed/mbed-os-security please review.

@0xc0170 0xc0170 removed request for a team and maclobdell September 2, 2020 15:01
@0xc0170
Copy link
Contributor Author

0xc0170 commented Sep 2, 2020

Oh, the bot was fast, due to the wrong base branch, all reviewers :-) I fixed that.

@0xc0170 0xc0170 changed the base branch from master to feature-cmake September 2, 2020 15:01
target_compile_options(${target}
PUBLIC
$<$<COMPILE_LANGUAGE:ASM>:--target=arm-arm-none-eabi -masm=armasm>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

-masm=auto works for me. If confirmed, I'll change this

Copy link
Collaborator

Choose a reason for hiding this comment

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

-masm=auto also worked on my Mac OS system with cmake version 3.17.3.
However, would it not be prudent to explicitly set it?

In rare circumstances, the auto-detection might select the wrong assembler for the input file.
For example, if the input file is a .S file that requires preprocessing, auto-detection might select
the wrong assembler. For the files where auto-detection selects the wrong assembler, you must
select -masm=gnu or -masm=armasm explicitly.

Source: https://www.keil.com/support/man/docs/armclang_ref/armclang_ref_yml1538053827173.htm

Copy link
Contributor Author

@0xc0170 0xc0170 Sep 3, 2020

Choose a reason for hiding this comment

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

For now, armasm would be better but what about the future? New MCU startup files will soon have GNU syntax for ARMClang .S files, therefore this armasm won't work. Auto mode is neat and exactly what we need. if we find bugs, we shall report it to the compiler (it is state as "rare").

I would rather take care of these rare cases (overwrite armasm to use gnu manually - the component would request to be compiled with GNU syntax for ARMClang6, for instance target component for specific board). Is this feasible ?

hugueskamba
hugueskamba previously approved these changes Sep 2, 2020
target_compile_options(${target}
PUBLIC
$<$<COMPILE_LANGUAGE:ASM>:--target=arm-arm-none-eabi -masm=armasm>
Copy link
Collaborator

Choose a reason for hiding this comment

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

-masm=auto also worked on my Mac OS system with cmake version 3.17.3.
However, would it not be prudent to explicitly set it?

In rare circumstances, the auto-detection might select the wrong assembler for the input file.
For example, if the input file is a .S file that requires preprocessing, auto-detection might select
the wrong assembler. For the files where auto-detection selects the wrong assembler, you must
select -masm=gnu or -masm=armasm explicitly.

Source: https://www.keil.com/support/man/docs/armclang_ref/armclang_ref_yml1538053827173.htm

@@ -27,7 +27,7 @@ function(mbed_set_cpu_core_options target mbed_toolchain)
PUBLIC
$<$<COMPILE_LANGUAGE:C>:${compile_options}>
$<$<COMPILE_LANGUAGE:CXX>:${compile_options}>
$<$<COMPILE_LANGUAGE:ASM>:--cpu=Cortex-M0plus>
$<$<$<$<COMPILE_LANGUAGE:ASM>:-mcpu=Cortex-M0plus>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a CMake expert but the extra $<$< looks imbalanced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixing now

@mergify mergify bot added needs: CI and removed needs: review labels Sep 2, 2020
@mergify mergify bot dismissed hugueskamba’s stale review September 3, 2020 06:08

Pull request has been modified.

Auto mode for ASM files. There might be rare cases where this fails, we will fix
them by requesting the component to use gnu syntax for assembly files.
@0xc0170
Copy link
Contributor Author

0xc0170 commented Sep 3, 2020

I fixed the typos in cores

@mergify mergify bot added needs: CI and removed needs: review labels Sep 3, 2020
@0xc0170 0xc0170 mentioned this pull request Sep 3, 2020
6 tasks
@0xc0170 0xc0170 merged commit 1272a43 into ARMmbed:feature-cmake Sep 3, 2020
@0xc0170 0xc0170 removed the release-type: patch Indentifies a PR as containing just a patch label Sep 3, 2020
@mergify mergify bot removed the ready for merge label Sep 3, 2020
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.

4 participants