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

NEW TOOLCHAIN: Add the ARMC6 Compiler #4949

Merged
merged 25 commits into from Sep 13, 2017

Conversation

Projects
None yet
7 participants
@theotherjimmy
Contributor

theotherjimmy commented Aug 21, 2017

Description

This implementation shares much more code with ARMC5 and comes with an
exporter: make_armc6. This all comes at a cost though: I was required to
remove the CPP invocations from the top of all of the scatter files and
add them back dynamically,
I had to replace (or not) the scatter shebang dynamically, based on the toolchain being used.

Design Notes

This variant was designed to minimize the change set. In particular, I removed all of the #!s from the top of Arm Compiler scatter files, and I replace the shebangs at runtime (of the tools). This does impose a requirement that the tools must always be able to do this going forward, but they should (and will, soon) detect if a scatter file does not contain a preprocessor invocation or contains a compatible preprocessor invocation and use it directly without modification. This may lead to linker errors, but I hope that "command armcc not found" and "command armclang not found" will be descriptive enough to users.

Comparison to #4905

Advantages

  • Exporting is possible, and it even comes with a working uVision5 exporter (armc5 only) and make exporter pair (for both armc5 and armc6).
  • Coexistence of armc5 and armc6 code is allowed and supported
    • Migration can then be done on a single target, single MCU, family or vendor basis

Disadvantages

  • There is no longer a single Arm toolchain, which might confuse users.
  • The tools do a bit of "magic" by forcing all scatter files to be preprocessed. that corrects armc5 invocations to armc6 invocations and armc6 invocations to armc5 invocations.

TODO:

  • Write section detailing this new toolchain in the Handbook
  • Add ARMC6 to the CI
  • Run nightly test to verify my implementation of ARMC6
  • Process dependency information for ARM6
  • Add small profile
  • Small error/warning messages
  • Test skip detection
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 21, 2017

Note this is an alternate implementation for #4905

@theotherjimmy theotherjimmy changed the title from NEW TOOLCHAIN: ARMC6, alternate implementation to NEW TOOLCHAIN: Add the ARMC6 Compiler, alternate implementation Aug 21, 2017

tools/toolchains/arm.py Outdated
if mem_map:
args.extend(["--scatter", mem_map])
def make_real_scatter(self, scatter_file):

This comment has been minimized.

@c1728p9

c1728p9 Aug 21, 2017

Contributor

What about leaving the shebang in as is for ARMC5 so all the linker script stay as they are and have make_real_scatter replace the ARMC5 shebang with the ARMC6 shebang only when building for ARMC6?

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 21, 2017

Contributor

Sounds great! I'll do it first thing tomorrow.

tools/toolchains/__init__.py Outdated
from tools.toolchains.gcc import GCC_ARM
from tools.toolchains.iar import IAR
TOOLCHAIN_CLASSES = {
'ARM': ARM_STD,
'uARM': ARM_MICRO,
'ARMC6': ARMC6,

This comment has been minimized.

@0xc0170

0xc0170 Aug 22, 2017

Member

are we removing uARM ,because ARM_MICRO class is still defined ? Is this breaking change or what is the intention here? or I have missed something - most probably

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 22, 2017

Contributor

Did not mean to do that. Rebase coming anyway. I'll remove that change.

tools/profiles/release.json Outdated
@@ -14,6 +14,13 @@
"-Wl,--wrap,_calloc_r", "-Wl,--wrap,exit", "-Wl,--wrap,atexit",
"-Wl,-n"]
},
"ARMC6": {
"common": ["-c", "--target=arm-arm-none-eabi", "-mthumb", "-Oz", "-DNDEBUG", "-flto"],

This comment has been minimized.

@0xc0170

0xc0170 Aug 22, 2017

Member

isn't this duplicate flto as it is also defined in the ld ?

Curious about choosing - Oz for the release?

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 22, 2017

Contributor

No. ld flags are not taken from common.

-Oz is for minimizing the size. That's what the release profile does. It used to be called small after all.

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:feature-armc5+armc6 branch 2 times, most recently Aug 22, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 22, 2017

@c1728p9 @0xc0170 I think I have addressed all of your comments. Please take another look.

targets/targets.json Outdated
@@ -600,7 +600,7 @@
"K64F": {
"supported_form_factors": ["ARDUINO"],
"core": "Cortex-M4F",
"supported_toolchains": ["ARM", "GCC_ARM", "IAR"],
"supported_toolchains": ["ARM", "GCC_ARM", "IAR", "ARMC6"],

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 23, 2017

Contributor

Drop this change.

tools/toolchains/__init__.py Outdated
TOOLCHAIN_PATHS = {
'ARM': ARM_PATH,
'uARM': ARM_PATH,

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 23, 2017

Contributor

Fix.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 24, 2017

I closed the other implementation, so I'm changing the title and removing the dupe label.

@theotherjimmy theotherjimmy changed the title from NEW TOOLCHAIN: Add the ARMC6 Compiler, alternate implementation to NEW TOOLCHAIN: Add the ARMC6 Compiler Aug 24, 2017

...Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/device/TOOLCHAIN_ARM_STD/MK64FN1M0xxx12.sct Outdated
@@ -122,4 +122,4 @@ LR_m_text m_interrupts_start m_text_start+m_text_size-m_interrupts_start { ; l
}
RW_IRAM1 ImageLimit(RW_m_data_2) { ; Heap region growing up
}
}
}

This comment has been minimized.

@c1728p9

c1728p9 Aug 25, 2017

Contributor

nitpick - this file doesn't need to be modified.

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 25, 2017

Contributor

Yup. I'll rebase that out.

"""
with open(scatter_file, "rb") as input:
lines = input.readlines()
if (lines[0].startswith(self.SHEBANG) or

This comment has been minimized.

@c1728p9

c1728p9 Aug 25, 2017

Contributor

should the line be normalized before using this check? That way extra spaces won't cause this comparison to fail

This comment has been minimized.

@theotherjimmy

theotherjimmy Aug 25, 2017

Contributor

The comparison failing would not be that bad: it would just emit another linker script with the correct shebang. I'm not sure I want to make that check complicated, but you're right that it can be improved to reduce the frequency of emitting linker scripts.

This comment has been minimized.

@c1728p9

c1728p9 Aug 25, 2017

Contributor

Good point. I didn't realize there is no harm done if the comparison fails. I agree, it would be good to keep it simple like it is now.

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:feature-armc5+armc6 branch Aug 25, 2017

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:feature-armc5+armc6 branch Aug 28, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Aug 29, 2017

I need to update this PR to include support for all targets.

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:feature-armc5+armc6 branch 5 times, most recently Aug 29, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 12, 2017

/morph test

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 12, 2017

So I assume this is the outstanding Realtek error

[DEBUG] Output: /home/arm/mbed_jenkins/workspace/bm_wrap/1374/mbed-os/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_RTL8195A/device/rtl8195a_init.c:21:10: fatal error: 'cmsis_gcc.h' file not found
17:52:41         [DEBUG] Output: #include "cmsis_gcc.h"

In this file we have :

#if defined(__CC_ARM)
#include "cmsis_armcc.h"
#elif defined(__GNUC__)
#include "cmsis_gcc.h"
#else
#include <cmsis_iar.h>
#endif

So it looks like GNUC is being defined for ARMCC 6 ? Which is a bit weird in itself. Speaking to Bartek he says it should be including cmsis_armclang.h for ARMCC 6 so I think this needs to look like -

#if defined(__CC_ARM) 
#include "cmsis_armcc.h"
#elif (defined (__ARMCC_VERSION) && __ARMCC_VERSION >= 6010050)
#include "cmsis_armclang.h"
#elif defined(__GNUC__)
#include "cmsis_gcc.h"
#else
#include <cmsis_iar.h>
#endif
@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 12, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1288

Build failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 12, 2017

REALTEK_RTL8195AM,toolchain ARMC6 there are long of warning : http://mbed-ci-master-2.austin.arm.com:8081/job/build_matrix/1138/target=REALTEK_RTL8195AM,toolchain=ARMC6/console

Build time for REALTEK_RTL8195AM :

  • ARM 5 - 4 min
  • ARM 6 - 1 hr 27 min

The temp fix to this is reduce the verbosity of logs, caveat to reduced info in case of failure.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 12, 2017

So it looks like __GNUC__ is being defined for ARMCC 6 ?

Yep

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 12, 2017

@adbridge Yes, that's the correct fix. Thanks for pushing it!

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 12, 2017

/morph test

1 similar comment
@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 12, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 12, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1289

Build failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 12, 2017

/morph test

@ARMmbed ARMmbed deleted a comment from mbed-bot Sep 12, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 12, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1291

Example Build failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 12, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 13, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1294

Example Build failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 13, 2017

Run with ARM 6 build disabled since we have following issue to be resolved

  1. Build time for REALTEK device : #4949 (comment)
  2. NORDIC application are not build properly missing boot loader resulting in device fails to boot the firmware.

/morph test

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 13, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 13, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1298

All builds and test passed!

@adbridge adbridge added ready for merge and removed needs: CI labels Sep 13, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 13, 2017

@studavekar Thanks for getting this through CI.

@adbridge adbridge merged commit 7b42891 into ARMmbed:master Sep 13, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@theotherjimmy theotherjimmy deleted the theotherjimmy:feature-armc5+armc6 branch Sep 13, 2017

pmancele added a commit to catiedev/mbed-os that referenced this pull request Oct 6, 2017

pmancele added a commit to catiedev/mbed-os that referenced this pull request Oct 6, 2017

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