Add missing --preinclude and -input on assembly files #2303

theotherjimmy · 2016-07-28T21:32:04Z

Except for IAR

Resolves #2272

theotherjimmy · 2016-07-28T21:32:48Z

cc @screamerbg @sg- @AlessandroA @0xc0170

sg- · 2016-07-28T23:26:32Z

Except for IAR

@0xc0170 @bogdanm Didn't we convert these to -D for IAR ASM or is that just the build system logic and not exporters?

0xc0170 · 2016-07-29T06:08:40Z

Except for IAR
@0xc0170 @bogdanm Didn't we convert these to -D for IAR ASM or is that just the build system logic and not exporters?

This change: b4e8cf6. However, looks like just cmd options. @bogdanm Is that correct?

@AlessandroA Please can you test?

@theotherjimmy I can't figure out what the IAR change does. I assume the exporter.py adds preinclude to asm flags, and the iar change removes it ,because of the IAR limitation? Better commit message might answer it.

theotherjimmy · 2016-07-29T06:20:03Z

@0xc0170 your assumtion is correct. The IAR assembler cannot handle the --preinclude command line flag, but all of the other exporters can. It was less code to remove it in IAR than to explicitly add it in all of the other exporters.

bogdanm · 2016-07-29T10:11:32Z

How did you test this? At least with GCC and make, I don't see how this can work, since the exporter template doesn't add the include paths for assembler files:

https://github.com/mbedmicro/mbed/blob/master/tools/export/gcc_arm_common.tmpl#L92

AlessandroA · 2016-07-29T10:14:11Z

I tested the PR and the compilation still fails. The build command should also include the config file mbed_config.h.

Failure:

arm-none-eabi-gcc -mcpu=cortex-m4 -mthumb -mfpu=fpv4-sp-d16 -mfloat-abi=softfp
-c -x assembler-with-cpp -include mbed_config.h -c -Wall -Wextra -Wno-unused-parameter
-Wno-missing-field-initializers -fmessage-length=0 -fno-exceptions -fno-builtin
-ffunction-sections -fdata-sections -funsigned-char -MMD -fno-delete-null-pointer-checks
-fomit-frame-pointer -mcpu=cortex-m4 -mthumb -mfpu=fpv4-sp-d16 -mfloat-abi=softfp -Os
-o mbed-os/hal/targets/cmsis/TARGET_Freescale/TARGET_K64F/TOOLCHAIN_GCC_ARM/startup_MK64F12.o
../../../mbed-os/hal/targets/cmsis/TARGET_Freescale/TARGET_K64F/TOOLCHAIN_GCC_ARM/startup_MK64F12.S

cc1: fatal error: mbed_config.h: No such file or directory

compilation terminated.

File structure (the file is there):

.
├── GettingStarted.htm
├── Makefile
├── exporter.yaml
└── mbed_config.h

Note: To test it I cherry-picked this commit on top of mbed-os-5.1.0-rc3 and removed commit f3c0ea3 to fix #2312.

@geky @0xc0170 @bogdanm @sg-

0xc0170 · 2016-07-29T10:32:43Z

@theotherjimmy please state always how you tested a patch (how to reproduce this prior the patch that it failed, and how it solves the problem after this patch).

bogdanm · 2016-07-29T10:44:46Z

The build command should also include the config file mbed_config.h.

It does here: -include mbed_config.h. However, it doesn't specify an include path, since include paths are not added for the assembler in the makefile generator template:

https://github.com/mbedmicro/mbed/blob/master/tools/export/gcc_arm_common.tmpl#L92

(you'd also need the -DDEVICE_ to fix your bug, but that's yet another fix, I'm afraid).

theotherjimmy · 2016-07-29T15:27:45Z

Woops. Yes @bogdanm, you are correct I did not specify an include path for the mbed_config.h.

theotherjimmy · 2016-07-29T16:00:53Z

That should do it.

Except for IAR, it does not support the --preinclude option for assembly files but all of the other exporters can. It was less code to remove it in IAR than to explicitly add it in all of the other exporters.

theotherjimmy · 2016-07-29T16:03:28Z

I also added the full explanation of why not IAR that I gave to @0xc0170 to the first commit message.

bogdanm · 2016-07-29T16:08:43Z

tools/export/gcc_arm_common.tmpl

 .s.o:
 	+@$(call MAKEDIR,$(dir $@))
-	$(CC) $(CPU) -c $(ASM_FLAGS) -o $@ $<
+	$(CC) $(CPU) -c $(ASM_FLAGS $(INCLUDE_PATHS)) -o $@ $<


Syntax error here (parantheses opening/closing incorrectly).

Doh! Just a moment. It passed my test.

Thanks for being on top of code review!

bogdanm · 2016-07-29T16:13:49Z

tools/export/exporters.py

@@ -196,6 +197,7 @@ def scan_and_copy_resources(self, prj_paths, trg_path, relative=False):
            self.config_header = self.toolchain.MBED_CONFIG_FILE_NAME
            config.get_config_data_header(join(trg_path, self.config_header))
            self.config_macros = []
+            self.resources.inc_dirs.append(dirname(self.config_header))


I don't think this does anything useful. 3 lines above (line 197) we set config_header to the name of the config header file, not to its path. So its dirname will always be empty.

True.......... So it should just append trg_path then? (I'm apparently still asleep over here :P)

I think that'll help, yes.

Actually that does some strange stuff. -I..//home/jimbri01/src/mbedmicro/mbed/.build/export/.temp ends up in my command line.

I'm pretty sure that the mbed_config.h file will always be in the same directory as the Makefile.

bogdanm · 2016-07-29T17:16:58Z

tools/export/exporters.py

@@ -196,6 +197,7 @@ def scan_and_copy_resources(self, prj_paths, trg_path, relative=False):
            self.config_header = self.toolchain.MBED_CONFIG_FILE_NAME
            config.get_config_data_header(join(trg_path, self.config_header))
            self.config_macros = []
+            self.resources.inc_dirs.append(".")


I'm going to assume that you veridifed that . == trg_path :)

Actually, the trg_path is an absolute path to the .temp directory where the project files are generated. I did verify that the Makefile and mbed_config.h always live in the same directory.

geky · 2016-07-29T21:17:40Z

I tested locally with ARMmbed/mbed-os-example-uvisor#8:

depends on [tools] Fixed infinite loop in config scan due to list comparison error #2317
mbed_config.h is now successfully included
non-config defines are not included, this can be fixed by adding CC_FLAGS asm rules in the makefile template
mbed-os-example-uvisor still does not run

Follow up on the issue:
ARMmbed/mbed-os-example-uvisor#8 (comment)

After fixing the CC_FLAGS issue, this pr should be good to go.

bogdanm · 2016-08-01T13:02:07Z

tools/export/iar.py

@@ -76,6 +76,8 @@ def generate(self, progen_build=False):
        project_data['tool_specific']['iar'].setdefault("misc", {})
        project_data['tool_specific']['iar'].update(tool_specific['iar'])
        project_data['tool_specific']['iar']['misc'].update(self.progen_flags)
+        project_data['tool_specific']['iar']['misc']['asm_flags'].remove(
+            self.toolchain.get_config_option(self.config_header)[0])


Is there a way to provide configuration options to the IAR assembler in the exporters now? On the command line, it's done by switching to -Ds instead of using --preinclude:

https://github.com/ARMmbed/mbed-os/blob/master/tools/toolchains/iar.py#L155

bogdanm · 2016-08-02T14:30:22Z

@theotherjimmy: any news on this?

theotherjimmy · 2016-08-02T15:15:12Z

Updated per @geky's comment.

geky · 2016-08-02T15:45:47Z

Ah! Sorry @theotherjimmy, it should be CC_SYMBOLS for asm rules.

geky · 2016-08-02T15:49:48Z

Looks good to me and worked locally. LGTM 👍

bogdanm · 2016-08-02T16:04:38Z

Any answer to this:

Is there a way to provide configuration options to the IAR assembler in the exporters now? On the command line, it's done by switching to -Ds instead of using --preinclude:

Or should we just ignore this for now?

theotherjimmy · 2016-08-02T16:07:06Z

from iasmarm help:

-DSYMB        Equivalent to: #define SYMB 1
-DSYMB=xx     Equivalent to: #define SYMB xx

That's my best guess.

bogdanm · 2016-08-02T16:08:11Z

from iasmarm help:

And again, as I already explained in a previous comment, that's already implemented here for command line:

https://github.com/ARMmbed/mbed-os/blob/master/tools/toolchains/iar.py#L155

Question is, do we plan to do the same thing in exporters?

theotherjimmy · 2016-08-02T16:16:14Z

I think we should. The question in my mind is: should it be part of this PR? EDIT: Yes.

bridadan · 2016-08-02T16:17:52Z

/morph test

mbed-bot · 2016-08-02T17:47:35Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 582

All builds and test passed!

bogdanm · 2016-08-02T17:57:36Z

Given the circumstances, I think we can go ahead with this and fix the IAR ASM flags issue when/if we need to.

bogdanm · 2016-08-02T18:05:46Z

...so yeah, the fix for IAR ASM is already in this PR. Thanks @theotherjimmy. I'll be right back, need to find a good optician.

theotherjimmy mentioned this pull request Jul 28, 2016

[exporter] [GCC_ARM] The build command for ASM files does not include custom defines #2272

Closed

0xc0170 added the needs: review label Jul 29, 2016

0xc0170 added the needs: work label Jul 29, 2016

theotherjimmy force-pushed the export-asm-symbols branch from d1547ac to 8e17375 Compare July 29, 2016 16:01

Add missing --preinclude and -input on assembly files

fed50f8

Except for IAR, it does not support the --preinclude option for assembly files but all of the other exporters can. It was less code to remove it in IAR than to explicitly add it in all of the other exporters.

theotherjimmy force-pushed the export-asm-symbols branch from 8e17375 to 5ef2a56 Compare July 29, 2016 16:02

bogdanm reviewed Jul 29, 2016
View reviewed changes

theotherjimmy force-pushed the export-asm-symbols branch from 5ef2a56 to 43737c1 Compare July 29, 2016 16:11

bogdanm reviewed Jul 29, 2016
View reviewed changes

theotherjimmy force-pushed the export-asm-symbols branch from 43737c1 to 437c84b Compare July 29, 2016 16:17

Add config file inculde directory when present

209cf9f

theotherjimmy force-pushed the export-asm-symbols branch from 437c84b to 209cf9f Compare July 29, 2016 16:40

bogdanm reviewed Jul 29, 2016
View reviewed changes

geky mentioned this pull request Jul 29, 2016

mbed-os-example-uvisor GCC_ARM export doesn't work on both Linux and Mac for mbed-os-5.1.0-rc1 ARMmbed/mbed-os-example-uvisor-thread#8

Closed

bogdanm reviewed Aug 1, 2016
View reviewed changes

bogdanm mentioned this pull request Aug 2, 2016

client-tls: Test fail K64F for export to GCC OoB#4 ARMmbed/mbed-os-example-tls#9

Closed

Add CC_SYMBOLS to assembler invocation

965ffc3

theotherjimmy force-pushed the export-asm-symbols branch from e4de665 to 965ffc3 Compare August 2, 2016 15:47

Add -D options to iar assembler on export

3873ccc

bogdanm merged commit c6c8d42 into ARMmbed:master Aug 2, 2016

bogdanm deleted the export-asm-symbols branch August 2, 2016 17:57

theotherjimmy restored the export-asm-symbols branch August 3, 2016 15:37

Add missing --preinclude and -input on assembly files #2303

Add missing --preinclude and -input on assembly files #2303

Conversation

theotherjimmy commented Jul 28, 2016

theotherjimmy commented Jul 28, 2016

sg- commented Jul 28, 2016

0xc0170 commented Jul 29, 2016

theotherjimmy commented Jul 29, 2016

bogdanm commented Jul 29, 2016

AlessandroA commented Jul 29, 2016 • edited Loading

0xc0170 commented Jul 29, 2016

bogdanm commented Jul 29, 2016

theotherjimmy commented Jul 29, 2016

theotherjimmy commented Jul 29, 2016

theotherjimmy commented Jul 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherjimmy Jul 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theotherjimmy Jul 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky commented Jul 29, 2016

Choose a reason for hiding this comment

bogdanm commented Aug 2, 2016

theotherjimmy commented Aug 2, 2016

geky commented Aug 2, 2016

geky commented Aug 2, 2016

bogdanm commented Aug 2, 2016

theotherjimmy commented Aug 2, 2016

bogdanm commented Aug 2, 2016

theotherjimmy commented Aug 2, 2016 • edited Loading

bridadan commented Aug 2, 2016

mbed-bot commented Aug 2, 2016

Result: SUCCESS

Output

bogdanm commented Aug 2, 2016

bogdanm commented Aug 2, 2016

AlessandroA commented Jul 29, 2016 •

edited

Loading

theotherjimmy Jul 29, 2016 •

edited

Loading

theotherjimmy Jul 29, 2016 •

edited

Loading

theotherjimmy commented Aug 2, 2016 •

edited

Loading