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

Tools: Include configuration in ASM #7061

Merged
merged 8 commits into from Jul 20, 2018

Conversation

Projects
None yet
7 participants
@TTornblom
Contributor

TTornblom commented May 30, 2018

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

This comment has been minimized.

Contributor

adbridge commented May 31, 2018

@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'])

@adbridge adbridge requested a review from ARMmbed/mbed-os-tools May 31, 2018

@theotherjimmy

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

@@ -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 [""]

This comment has been minimized.

@theotherjimmy

theotherjimmy May 31, 2018

Contributor

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.

This comment has been minimized.

@TTornblom

TTornblom Jun 1, 2018

Contributor

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?

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 1, 2018

Contributor

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.

This comment has been minimized.

@TTornblom

TTornblom Jun 4, 2018

Contributor

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

This comment has been minimized.

@theotherjimmy

theotherjimmy Jun 4, 2018

Contributor

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

@@ -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 [""]

This comment has been minimized.

@theotherjimmy

theotherjimmy May 31, 2018

Contributor

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?

This comment has been minimized.

@TTornblom

TTornblom Jun 1, 2018

Contributor

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

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 14, 2018

@TTornblom Has any progress been made on this?

@TTornblom

This comment has been minimized.

Contributor

TTornblom commented Jun 15, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Jun 15, 2018

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

This comment has been minimized.

Contributor

TTornblom commented Jun 15, 2018

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 15, 2018

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 15, 2018

@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

This comment has been minimized.

Contributor

theotherjimmy commented Jun 15, 2018

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 15, 2018

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

@TTornblom

This comment has been minimized.

Contributor

TTornblom commented Jun 15, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Jun 15, 2018

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 15, 2018

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

This comment has been minimized.

Contributor

TTornblom commented Jun 15, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Jun 15, 2018

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

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

@TTornblom

This comment has been minimized.

Contributor

TTornblom commented Jun 18, 2018

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 18, 2018

@TTornblom

This comment has been minimized.

Contributor

TTornblom commented Jun 18, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Jun 18, 2018

@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 😞

@TTornblom

This comment has been minimized.

Contributor

TTornblom commented Jul 18, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Jul 18, 2018

@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

This comment has been minimized.

Contributor

TTornblom commented Jul 18, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Jul 18, 2018

/morph export-build

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 19, 2018

/morph build

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 19, 2018

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

This comment has been minimized.

mbed-ci commented Jul 19, 2018

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

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 20, 2018

/morph mbed2-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 20, 2018

Restarting test, CI exception

/morph test

@mbed-ci

This comment has been minimized.

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

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

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 793 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10112 cycles (+492 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9960B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 30, 2018

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

@@ -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:

This comment has been minimized.

@0xc0170

0xc0170 Jul 31, 2018

Member

@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?

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

Merge pull request ARMmbed#7061 from TTornblom/master
Tools: Include configuration in ASM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment