-
Notifications
You must be signed in to change notification settings - Fork 17
Work around ARM and IAR optimization issue #48
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
Conversation
5581e38 to
82287f9
Compare
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command line in the commit message is wrong given the commit contents: since mbed_profile.json only contains settings to change from the default profile, the default profile needs to be listed explicitly.
mbed compile -m CY8CKIT_062_WIFI_BT -t ARMC6 --profile mbed-os/tools/profiles/develop.json --profile mbed_profile.json
With the command line above, I have verified that the build produces significantly larger object files compared to the default profile, which is evidence that the optimization level changed from -Os to -O1. However the content of the file is fragile, see my comments.
| "--no_wrap_diagnostics", "-e", | ||
| "--diag_suppress=Pa050,Pa084,Pa093,Pa082,Pe540", "-Ol", "--enable_restrict", | ||
| "-DMBED_TRAP_ERRORS_ENABLED=1"] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add "GCC_ARM": {} so that we can use the same command line regardless of the target.
(Arguably the fact that mbed compile -t FOO --profile P1 --profile P2 requires FOO to have an entry in both P1 and P2 is a bug in mbed-cli, but we have to work with mbed-cli as it is.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting the user not to pass the profile option if they didn't need the workaround. I've added all toolchains, since there is at least one user who wanted to use the profile options on non-affected toolchains.
getting-started/mbed_profile.json
Outdated
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "ARMC6": { | |||
| "common": ["-c", "--target=arm-arm-none-eabi", "-mthumb", "-O1", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the build with mbed compile -vv … shows that if this is used to override the default profile, the compiler command line combines options from the two profile files. Repeating options here is not only superfluous, it's misleading, since for example if we wanted to avoid a -DFOO in the default profile we'd need to have -UFOO here. Therefore this should just be "common": ["-Os"]. The same goes for IAR.
Alternatively, if this profile is meant to be used on its own, it needs to have the settings for the other tools ("c", "cxx", "ld"). Otherwise the build fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on standalone or not. You are correct that the IAR build failed without the other options. The ARMC6 build, as listed in the commit message, worked OK.
Primarily for maintainability sake, I've switch to override style and now require both profile options to be specified. This also allows use of the release profile or debug profile with the workaround, whereas with the standalone profile you were forced into a "develop"-like profile.
getting-started/mbed_profile.json
Outdated
| "common": [ | ||
| "--no_wrap_diagnostics", "-e", | ||
| "--diag_suppress=Pa050,Pa084,Pa093,Pa082,Pe540", "-Ol", "--enable_restrict", | ||
| "-DMBED_TRAP_ERRORS_ENABLED=1"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using this profile, I get errors due to variable length arrays not being enabled. I'd suggest adding the rest of the mbed-os profile information:
"asm": [],
"c": ["--vla", "--diag_suppress=Pe546"],
"cxx": ["--guard_calls", "--no_static_destruction"],
"ld": ["--skip_dynamic_initialization", "--threaded_lib"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try again with latest patch set.
82287f9 to
356c591
Compare
Add a custom profile to work around an issue with the ARM and IAR
compilers, where the example appears to hang without any error. Building
the example with `-O1` (ARM) and `-Ol` (IAR), rather than the default of
`-Os` (ARM) and `-Ohz` (IAR), works as a workaround.
All toolchains, not just ARM and IAR, are added to the profile in the
case where the user wishes to always specify the --profile selecting
options even when using a toolchain which doesn't exhibit faulty
behavior.
To use the custom profile with mbed-cli, along with the develop profile,
use the --profile option to provide the develop profile followed by the
custom override-only profile.
mbed compile -m CY8CKIT_062_WIFI_BT -t ARMC6 --profile mbed-os/tools/profiles/develop.json --profile mbed_profile.json
Arm internal reference: SDCTRESPONSE-3550
356c591 to
37cafd8
Compare
|
New patch set lists all toolchains explicitly, for users who want to always pass Tested working with CY8CKIT_062_WIFI_BT: develop profile and ARMC6, develop profile and GCC_ARM, release profile and ARMC6, debug profile and ARMC6. |
| "common": ["-O1"] | ||
| }, | ||
| "IAR": { | ||
| "common": ["-Ol"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes an error with IAR: [ERROR] Command line error: Option -O can only occur once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, so we need a standalone profile. I am not a fan of maintaining that. I'll see if we can take a better approach.
|
Took approach of reducing stack usage of the example instead. |
Add a custom profile to work around an issue with the ARM and IAR
compilers, where the example appears to hang without any error. Building
the example with
-O1, rather than the default of-Os, works as aworkaround.
To use the custom profile with mbed-cli, use the --profile option.
Arm internal reference: SDCTRESPONSE-3550