Fixing uvisor defines to fix build issues #3718

bridadan · 2017-02-07T22:40:14Z

Description

This issue was found when trying to integrate the GNU ARM Eclipse exporter PR here: #3561

There are a few files that build fine in ALL the other build systems (seriously, the mbed build system, make exporters, IAR, AND uVision all work just fine) but that cause issues in the new Eclipse exporter. This seems to be related to build order.

These changes just add explicit includes in files that were causing issues.

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Related PRs

This is currently blocking #3561 from coming in.

Todos

morph test
morph export-build

Note to reviewers

@Patater and/or @AlessandroA Are there other files where we should add these explicit includes?

bridadan · 2017-02-07T22:43:45Z

/morph test

bridadan · 2017-02-07T22:43:50Z

/morph export-build

mbed-bot · 2017-02-07T23:00:36Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1524

Example Prep failed!

mbed-bot · 2017-02-07T23:04:15Z

Result: FAILURE

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

/morph export-build

Output

mbed Build Number: 111

Exporter Build failed!

Patater · 2017-02-08T10:59:42Z

@bridadan

This doesn't seem like it should be required. I am suspicious of the files the Eclipse project is trying to include.

If you are building without FEATURE_UVISOR, all files under FEATURE_UVISOR should not be included. Likewise, no matter if FEATURE_UVISOR is used or not, any TARGET_IGNORE folder should not be included. Could you tell me whether your project is enabling FEATURE_UVISOR and if the Eclipse project is including TARGET_IGNORE and/or FEATURE_UVISOR files?

Thanks.

bridadan · 2017-02-08T15:20:54Z

@Patater the project is enabling FEATURE_UVISOR (it's using this example: https://github.com/ARMmbed/mbed-os-example-uvisor). Also, it doesn't appear that any of the folders with TARGET_IGNORE are being included, but as far as I can tell that's not important for uVisor? This is the only file I could find in the uVisor tree that is behind a TARGET_IGNORE folder: https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_UVISOR/importer/TARGET_IGNORE/.gitignore

It does seem to be including all FEATURE_UVISOR files though.

bridadan · 2017-02-08T15:23:00Z

/morph test

Patater · 2017-02-08T15:24:06Z

@bridadan Thanks for looking into this. Excluding TARGET_IGNORE is important if the importer script is even run. Developers do this, but CI wouldn't. I see that you are building a uVisor example in your CI log, so yeah, you are building with FEATURE_UVISOR on occasion.

Patater · 2017-02-08T15:25:30Z

features/FEATURE_UVISOR/includes/uvisor/api/inc/uvisor-lib.h

@@ -17,6 +17,8 @@
 #ifndef __UVISOR_API_UVISOR_LIB_H__
 #define __UVISOR_API_UVISOR_LIB_H__

+#include "includes/uvisor-lib/uvisor-lib.h"


If this is needed, it's probably because some other file isn't including uvisor-lib/uvisor-lib.h correctly. Can you get away with not having this? What breaks? Can you include uvisor-lib/uvisor-lib.h from there instead?

Looks like at least cmsis/core_cmSecureAccess.h needs to include uvisor-lib/uvisor-lib.h instead of just uvisor-lib.h.

Note that uvisor-lib/uvisor-lib.h includes api/inc/uvisor-lib.h, which is why we shouldn't include uvisor-lib/uvisor-lib.h from api/inc/uvisor-lib.h.

I can't just remove it, otherwise I get compilation errors. The issue here is without the include, UVISOR_PRESENT doesn't get defined, so I get redefinition errors when the compile process reaches this part of the code: https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_UVISOR/includes/uvisor/api/inc/unsupported.h#L74

Which it definitely shouldn't be getting to since the K64F is a supported platform.

It looks like doing your suggested change with cmsis/core_cmSecureAccess.h allows it to build, though not sure if it actually runs. But I guess we can always start from here! I'll update this PR.

Yeah, removing will cause compilation errors. I was curious which files were having errors.

Thanks for the fix. :)

Actually when I used your cmsis/core_cmSecureAccess.h change I was able to remove the include! Though I just tried the binary and it doesn't seem to work. I'm afraid my knowledge is limited on what files need to include what, maybe this needs to be looked at more in depth?

Patater · 2017-02-08T15:25:44Z

features/FEATURE_UVISOR/includes/uvisor-lib/rtx/rtx_box_index.h

@@ -18,6 +18,7 @@
 #define __RTX_BOX_INDEX_H__

 #include "cmsis_os.h"
+#include "api/inc/vmpu_exports.h"


This is a good change.

mbed-bot · 2017-02-08T15:52:11Z

Result: ABORTED

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

/morph test

Output

mbed Build Number: 1530

Example Build failed!

bridadan · 2017-02-08T15:55:11Z

/morph test

Patater

Our CI passed, so you didn't break anything with the normal build process. I'm happy with the changes here.

We can debug the exporter issues with uVisor in priority with other tasks. I motion to accept this PR and the exporter one, given that an issue is filed regarding uVisor and the exporter. Could you please file an issue?

Thanks again for the fixes.

bridadan · 2017-02-08T16:27:47Z

/morph export-build

AlessandroA · 2017-02-08T16:31:15Z

@bridadan Historically exporters have not worked with uVisor due to mismatching FPU configurations: #2320.

Since this is already communicated (it's in our release note), I agree with @Patater that from our perspective this can be merged. Thanks for the fix!

bridadan · 2017-02-08T16:49:44Z

Thanks for the quick support @AlessandroA and @Patater!

mbed-bot · 2017-02-08T17:18:43Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1531

All builds and test passed!

mbed-bot · 2017-02-08T17:26:58Z

Result: FAILURE

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

/morph export-build

Output

mbed Build Number: 112

Exporter Build failed!

bridadan · 2017-02-08T17:27:18Z

I was doing some Jenkins maintenance and ended up causing a few failures in the exporter builds 😞 restarting now.

/morph export-build

mbed-bot · 2017-02-08T18:20:33Z

Result: SUCCESS

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

/morph export-build

Output

mbed Build Number: 113

All exports and builds passed!

bridadan · 2017-02-08T19:20:54Z

@0xc0170 @sg- Any chance we could get a quick merge on this? This should fix the last failures in #3561

bridadan requested review from Patater, theotherjimmy and AlessandroA February 7, 2017 22:40

bridadan mentioned this pull request Feb 7, 2017

Add GNU ARM Eclipse exporter #3561

Merged

bridadan added needs: CI needs: review labels Feb 7, 2017

Patater suggested changes Feb 8, 2017

View reviewed changes

Fixing uvisor defines to fix build issues

bca993a

bridadan force-pushed the uvisor_define_fix branch from 44e92ce to bca993a Compare February 8, 2017 15:51

Patater approved these changes Feb 8, 2017

View reviewed changes

AlessandroA approved these changes Feb 8, 2017

View reviewed changes

bridadan removed the needs: review label Feb 8, 2017

bridadan added the needs: review label Feb 8, 2017

bridadan removed the needs: CI label Feb 8, 2017

sg- added ready for merge and removed needs: review labels Feb 8, 2017

sg- merged commit cd55625 into ARMmbed:master Feb 8, 2017

sg- added release-version: 5.3.5 and removed ready for merge labels Feb 8, 2017

Fixing uvisor defines to fix build issues #3718

Fixing uvisor defines to fix build issues #3718

Conversation

bridadan commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Related PRs

Todos

Note to reviewers

Uh oh!

bridadan commented Feb 7, 2017

Uh oh!

bridadan commented Feb 7, 2017

Uh oh!

mbed-bot commented Feb 7, 2017

Output

Uh oh!

mbed-bot commented Feb 7, 2017

Output

Uh oh!

Patater commented Feb 8, 2017

Uh oh!

bridadan commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bridadan commented Feb 8, 2017

Uh oh!

Patater commented Feb 8, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbed-bot commented Feb 8, 2017

Output

Uh oh!

bridadan commented Feb 8, 2017

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

bridadan commented Feb 8, 2017

Uh oh!

AlessandroA commented Feb 8, 2017

Uh oh!

bridadan commented Feb 8, 2017

Uh oh!

mbed-bot commented Feb 8, 2017

Output

Uh oh!

mbed-bot commented Feb 8, 2017

Output

Uh oh!

bridadan commented Feb 8, 2017

Uh oh!

mbed-bot commented Feb 8, 2017

Output

Uh oh!

bridadan commented Feb 8, 2017

Uh oh!

Uh oh!

bridadan commented Feb 7, 2017 •

edited

Loading

bridadan commented Feb 8, 2017 •

edited

Loading