Tools: Include configuration in ASM #7061

TTornblom · 2018-05-30T15:34:35Z

Description

This fix adds assembler command line defines for macros in mbed_app.json and mbed_lib.json files.

Tested with mbed-os-example-blinky project for K64F target, on IAR Embedded Workbench 7.80.4.

Tested on mbed CLI and mbed export for IAR.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

adbridge · 2018-05-31T11:56:21Z

@theotherjimmy Any idea why this failed ?
def assemble(self, source, object, includes):

  _, macros = self.config.get_config_data()

E AttributeError: 'NoneType' object has no attribute 'get_config_data'
tools/toolchains/iar.py:157: AttributeError
---------------------------------- Hypothesis ----------------------------------
Falsifying example: test_toolchain_profile_asm(profile={'asm': [], 'c': [], 'common': [], 'cxx': [], 'ld': []}, source_file=[u'0'])

theotherjimmy

I would like to see more similarity between the exporter and the toolchain implementation of the same thing.

theotherjimmy · 2018-05-31T16:32:50Z

tools/toolchains/iar.py

@@ -154,8 +154,10 @@ def get_compile_options(self, defines, includes, for_asm=False):

    @hook_tool
    def assemble(self, source, object, includes):
+        _, macros = self.config.get_config_data()
+        defines = ['-D%s' % d for d in macros] if macros else [""]


I'm pretty sure this is the same as the result of self.get_compile_options(self.get_symbols(True), includes, True) below, so the changes in this file are not needed.

No, I tried this at first, but the macros from the mbed_app.json and mbed_lib.json files are not included in the set returned by self.get_compile_options(self.get_symbols(True), includes, True). If they are supposed to, then perhaps this is a bug?

get_symbols is a shared bit of infrastructure so we can eliminate any compiler differences caused by it. This leaves get_compile_options
arm: https://github.com/ARMmbed/mbed-os/blob/master/tools/toolchains/arm.py#L136-L139
gcc: https://github.com/ARMmbed/mbed-os/blob/master/tools/toolchains/gcc.py#L144-L155

They seem to disagree on whether or not to include the include paths, but they both treat the defines passed in the same.

This makes me think that the issue this is trying to fix will introduce an inconsistency between the assemblers. That's something we should avoid.

I'm happy to have it fixed globally in get_symbols(), if that is doable.

That's doable, and I think that's how it should be done.

theotherjimmy · 2018-05-31T16:35:07Z

tools/export/iar/__init__.py

@@ -111,6 +111,8 @@ def generate(self):
               self.resources.c_sources + self.resources.cpp_sources + \
               self.resources.objects + self.resources.libraries
        flags = self.flags
+        _, macros = self.toolchain.config.get_config_data()
+        defines = ['-D%s' % d for d in macros] if macros else [""]


Could you do:

macros = self.toolchain.get_symbols(True) defines = self.toolchain.get_compile_options(macros, self.resources.inc_dirs, True)

so that it exactly matches the code run in iar.assemble?

Same thing here, defines does not include the macros from mbed_app.json and mbed_lib.json with this.

cmonr · 2018-06-14T15:37:44Z

@TTornblom Has any progress been made on this?

TTornblom · 2018-06-15T07:46:09Z

No progress.
I've had a private conversation with @theotherjimmy and I don't see how this can be fixed with his comment:
"This makes me think that the issue this is trying to fix will introduce an inconsistency between the assemblers. That's something we should avoid."
Comparing the toolchains I see that there is already an inconsistency between the toolchains, "arm" handles this by running a preprocessor step where mbed_config.h is preincluded for assembler files. No other toolchain seems to do anything similar.
My initial attempt was to use the same idea for IAR, with the CLI, but that would not work when exporting to the IDE. The current "fix" adds the macros from mbed_app.json and mbed_lib.json as -D defines to both CLI and when exporting to the IDE, but apparently it was not kosher.
We can have a discussion on how or if this should be fixed, but in the current state there is an inconsistency between the toolchains.

theotherjimmy · 2018-06-15T13:44:04Z

apparently it was not kosher.

Because it only applies to IAR, and not ARM or GCC_ARM.

Would you like me to change this patch to apply to all 3?

TTornblom · 2018-06-15T13:46:57Z

@theotherjimmy If that's OK with you, then by all means go ahead, I'd be happy to have this sorted out.

theotherjimmy · 2018-06-15T13:51:13Z

@TTornblom Yeah. No problem. Let me get that right now.

theotherjimmy · 2018-06-15T14:13:07Z

@TTornblom I'd like to point out that:

arm" handles this by running a preprocessor step where mbed_config.h is preincluded for assembler files

This is not true. ARM, GCC_ARM, and IAR all have the same behavior here: they include macros for the toolchian and target labels and don't include mbed_config.h. Your master branch may be out of date.

theotherjimmy · 2018-06-15T14:30:32Z

@TTornblom I think this is now consistant: all compilers use mbed_config.h

theotherjimmy · 2018-06-15T14:31:53Z

Note: --preinclude is an option that appears in the iar EWARM assembler reference manual. I hope it works as advertised.

TTornblom · 2018-06-15T16:40:55Z

Unfortunately the EWARM assembler is based on an older codebase and does not support —preinclude. That would have been my preferred solution too.

My first attempt was to run the C preprocessor, which does support —preinclude, but it was just too complicated to get that to work in the IDE.

theotherjimmy · 2018-06-15T17:30:26Z

@TTornblom Dang. Any chance that the assembler reference could be updated to remove that option?

theotherjimmy · 2018-06-15T17:32:23Z

I'll come back to this on Monday, to see if I can work around this problem with a via file full of -D.

theotherjimmy

--preinclude does not work in the EWARM assembler.

TTornblom · 2018-06-15T17:50:49Z

I can submit an RFE against the EWARM assembler, but it is not a quick fix that would solve this problem, especially as the Mbed tool chain is still based on 7.x.

theotherjimmy · 2018-06-15T18:24:59Z

Removing it from the documentation would not fix the problem, but prevent others from encountering the not-supported option.

TTornblom · 2018-06-18T06:42:59Z

@theotherjimmy Where did you find that the EWARM assembler supports --preinclude? Need to fix this documentantion issue.

theotherjimmy · 2018-06-18T14:20:59Z

~~http://supp.iar.com/FilesPublic/UPDINFO/005832/arm/doc/EWARM_AssemblerReference.ENU.pdf~~
http://supp.iar.com/FilesPublic/UPDINFO/007748/ew/doc/EWRL78_AssemblerReference.ENU.pdf

My mistake, that's the wrong assembler reference. The correct PDF does not claim that --preinclude should work.

TTornblom · 2018-06-18T14:43:08Z

Yes, the RL78 assembler is based on a newer code base, which does support --preinclude.
The ARM C/C++ compiler does support --preinclude, just not the assembler.

theotherjimmy · 2018-06-18T16:14:20Z

@TTornblom I got everything passing along as I wanted, with export getting the -Ds and compile getting a -f, but the -f returns error code "3" which is not described by http://supp.iar.com/FilesPublic/UPDINFO/005832/arm/doc/EWARM_AssemblerReference.ENU.pdf 😞

theotherjimmy · 2018-07-17T15:12:23Z

@TTornblom That's incorrect. Our CI reports IDE failures (can't find the project file after generation), and the Mbed CLI build works

Unless you mean CLI to mean IAR CLI. CLI can be quite ambiguous.

TTornblom · 2018-07-18T07:59:11Z

Yes, I mean the IAR CLI and the IAR IDE.

The test I did yesterday was to import "mbed-os-example-blinky", and replacing mbed-os in that with what's on github.

I added an mbed_app.json file with the following content to the root of the project:

{
    "macros": [
        "MBED_FAULT_HANDLER_DISABLED"
    ]
}

and then built it as:

mbed compile -t IAR -m K64F

and it failed linking as mbed-os/rtos/TARGET_CORTEX/TARGET_CORTEX_M/TOOLCHAIN_IAR/except.S was not compiled using the mbed_app.json file, so it referred to the undefined symbol "mbed_fault_context".

I then exported the project to EWARM:

mbed export -i IAR -m K64F

and built the project using EWARM 7.80, which worked OK, with no link errors and no "mbed_fault_context" defined, so the mbed_app.json file was handled correctly.

My conclusion is that the IAR CLI is not handling the mbed_app.json (and I guess mbed_lib.json) files correctly, but that the IAR IDE exporter does handle them correctly.

theotherjimmy · 2018-07-18T14:27:19Z

@TTornblom I'm sorry but that clarification did not help my understanding of the issues at hand.

That's also quite interesting, as it's exactly the opposite of what our CI is reporting.

I just tried your mbed_app.json on this branch with mbed compile -t IAR -m K64F and I did not receive any compiler or assembler errors.

TTornblom · 2018-07-18T14:52:10Z

Sorry, my bad. I had a setting in EWARM 7.80 left over from my earlier work on this that apparently allowed the project to be built.

I set up a new project, without replacing mbed-os, and with the setting in EWARM removed, and both the IAR CLI and EWARM builds then fails with an undefined "mbed_fault_context".

theotherjimmy · 2018-07-18T18:04:08Z

/morph export-build

cmonr · 2018-07-19T15:28:55Z

/morph build

theotherjimmy · 2018-07-19T15:40:26Z

I think that'll do it. I escaped the macros. Maybe the parser was having a bad day with the other file. I can no longer reproduce the issue locally (with K64F + mesh-minimal export)

mbed-ci · 2018-07-19T20:31:10Z

Build : SUCCESS

Build number : 2649
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7061/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-07-19T21:51:20Z

Exporter Build : SUCCESS

Build number : 2282
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7061/

cmonr · 2018-07-20T01:44:41Z

/morph mbed2-build

0xc0170 · 2018-07-20T10:08:03Z

Restarting test, CI exception

/morph test

mbed-ci · 2018-07-20T12:11:42Z

Test : SUCCESS

Build number : 2389
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7061/2389

cmonr · 2018-07-30T15:50:19Z

Postponing this since an old issue seems to have resurfaced with it: #4879

0xc0170 · 2018-07-31T07:49:42Z

tools/toolchains/iar.py

@@ -140,17 +140,26 @@ def get_config_option(self, config_header):

    def get_compile_options(self, defines, includes, for_asm=False):
        opts = ['-D%s' % d for d in defines]
-        if for_asm :
+        if for_asm:


@TTornblom Can you review this change in this file , regarding -f for assembly? There's related issue #4879 that we faced again in the recent days.

How this can lead to IAR assembler crashing?

Tools: Include configuration in ASM

adbridge added the needs: review label May 31, 2018

adbridge requested a review from a team May 31, 2018 11:57

theotherjimmy suggested changes May 31, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jun 4, 2018

theotherjimmy approved these changes Jun 15, 2018

View reviewed changes

cmonr added needs: CI release-version: 5.9.2 and removed needs: work labels Jun 15, 2018

theotherjimmy suggested changes Jun 15, 2018

View reviewed changes

cmonr added needs: work and removed needs: CI labels Jun 15, 2018

theotherjimmy added 2 commits July 19, 2018 10:18

Condense exported IAR project file

d6b0f80

Re-indent IAR ewp temptate

14c2eff

cmonr added needs: CI and removed needs: work labels Jul 19, 2018

0xc0170 added ready for merge and removed needs: CI labels Jul 20, 2018

cmonr merged commit 4bcca89 into ARMmbed:master Jul 20, 2018

0xc0170 removed the ready for merge label Jul 20, 2018

cmonr added release-version: 5.9.4 release-version: 5.9.5 and removed release-version: 5.9.4 labels Jul 25, 2018

0xc0170 reviewed Jul 31, 2018

View reviewed changes

0xc0170 mentioned this pull request Aug 6, 2018

Add Error Decoding URL to error report #7662

Merged

adbridge added release-version: 5.10.0-rc1 and removed release-version: 5.9.5 labels Aug 13, 2018

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018

Merge pull request ARMmbed#7061 from TTornblom/master

032a3f2

Tools: Include configuration in ASM

kjbracey mentioned this pull request Sep 20, 2018

Hotfix for PR #7778 on Silicon Labs targets #8023

Merged

Tools: Include configuration in ASM #7061

Tools: Include configuration in ASM #7061

Uh oh!

Conversation

TTornblom commented May 30, 2018

Description

Pull request type

Uh oh!

adbridge commented May 31, 2018

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TTornblom Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TTornblom Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jun 14, 2018

Uh oh!

TTornblom commented Jun 15, 2018

Uh oh!

theotherjimmy commented Jun 15, 2018

Uh oh!

TTornblom commented Jun 15, 2018

Uh oh!

theotherjimmy commented Jun 15, 2018

Uh oh!

theotherjimmy commented Jun 15, 2018

Uh oh!

theotherjimmy commented Jun 15, 2018

Uh oh!

theotherjimmy commented Jun 15, 2018

Uh oh!

TTornblom commented Jun 15, 2018

Uh oh!

theotherjimmy commented Jun 15, 2018

Uh oh!

theotherjimmy commented Jun 15, 2018

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

TTornblom commented Jun 15, 2018

Uh oh!

theotherjimmy commented Jun 15, 2018

Uh oh!

TTornblom commented Jun 18, 2018

Uh oh!

theotherjimmy commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TTornblom commented Jun 18, 2018

Uh oh!

theotherjimmy commented Jun 18, 2018

Uh oh!

theotherjimmy commented Jul 17, 2018

Uh oh!

TTornblom commented Jul 18, 2018 • edited by theotherjimmy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Jul 18, 2018

Uh oh!

TTornblom commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Jul 18, 2018

TTornblom Jun 1, 2018 •

edited

Loading

TTornblom Jun 1, 2018 •

edited

Loading

theotherjimmy commented Jun 18, 2018 •

edited

Loading

TTornblom commented Jul 18, 2018 •

edited by theotherjimmy

Loading

TTornblom commented Jul 18, 2018 •

edited

Loading

theotherjimmy commented Jul 19, 2018 •

edited

Loading

0xc0170 Jul 31, 2018 •

edited

Loading