Skip to content
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

STM32: Fix alignment of execute region to 8byte boundary #8013

Merged
merged 1 commit into from
Oct 10, 2018

Conversation

deepikabhavnani
Copy link

Description

Resolves #8004

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

@JanneKiiskila
Copy link
Contributor

Nope, this is only fixing a very small subset of targets. It's no good, you must fix ALL of them.

@deepikabhavnani
Copy link
Author

Nope, this is only fixing a very small subset of targets. It's no good, you must fix ALL of them.

Please share which targets are failing, we do not support ARMC6 for all v6/v7 targets.

@deepikabhavnani
Copy link
Author

Fix is for all ST targets

@jeromecoutant
Copy link
Collaborator

Hi
We need to fix also icf IAR files and ld GCC files....

@ARMmbed/team-st-mcd

@jeromecoutant
Copy link
Collaborator

define symbol __region_SRAM1_start__ = 0x200001AC; /* Aligned on 8 bytes (428 = 53 x 8) */

I like comment "428 = 53 x 8" ...

@deepikabhavnani
Copy link
Author

@jeromecoutant IAR and GCC files are updated. Please review

Copy link
Contributor

@LMESTM LMESTM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with the changes.
Would be just great to add a few words in the commit messages themselves, basically a kind of copy of #8004 description that explains that been breaking change when removing --legacyalign compile option (or at least this is my understanding ..)

@@ -35,8 +35,8 @@ LR_IROM1 0x08000000 0x20000 { ; load region size_region (128K)
.ANY (+RO)
}

; 59 vectors (16 core + 43 peripheral) * 4 bytes = 236 bytes to reserve (0xEC)
RW_IRAM1 (0x20000000+0xEC) (0x5000-0xEC) { ; RW data
; 59 vectors (16 core + 43 peripheral) * 4 bytes = 236 bytes to reserve (0xEC+0x4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a comment that says +0x04 says it's for overall 8 bytes alignment

Copy link
Contributor

@bcostm bcostm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for me. It seems this PR is only for STM32 devices, so maybe add "STM32:" in the PR title ?

@JanneKiiskila
Copy link
Contributor

We should do the rest of the chips as well, not just STM (just my humble opinion). This is a quite large change - it's trivial, but amount of files modified is quite large. My 1st scan on this (when I stated there's more than >250 files impacted) missed actually quite a few files...

@deepikabhavnani
Copy link
Author

deepikabhavnani commented Sep 19, 2018

#8024 #8186

@deepikabhavnani deepikabhavnani changed the title Fix alignment of execute region to 8byte boundary STM32: Fix alignment of execute region to 8byte boundary Sep 19, 2018
Copy link
Collaborator

@jeromecoutant jeromecoutant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ST CI tests OK

@cmonr
Copy link
Contributor

cmonr commented Sep 29, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Sep 29, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Sep 29, 2018

@mbed-ci
Copy link

mbed-ci commented Sep 30, 2018

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 1, 2018

The test failure not related to this changeset, we will restart after 5.10.1 RC is completed

@NirSonnenschein
Copy link
Contributor

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 9, 2018

@cmonr
Copy link
Contributor

cmonr commented Oct 9, 2018

Stopped job because it's consistently failing against NUCLEO_F207ZG.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 9, 2018

Stopped job because it's consistently failing against NUCLEO_F207ZG.

From the changes, I do not see why it would fail @ARMmbed/team-st-mcd any pointers?

…r files

--legacyalign, --no_legacyalign are deprecated from ARMC6 compiler, in order to
remove deprecated flags all linker files should strictly align to 8-byte boundary
@deepikabhavnani
Copy link
Author

Got the culprit..

-/* 8-byte aligned(0x184) = 0x188
+/* 8-byte aligned(0x184) = 0x188 */

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 9, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 9, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Oct 10, 2018

@mbed-ci
Copy link

mbed-ci commented Oct 10, 2018

@cmonr
Copy link
Contributor

cmonr commented Oct 18, 2018

#8024 supercedes this PR.
Patch creation complains about empty patch.

@cmonr
Copy link
Contributor

cmonr commented Oct 18, 2018

Adding label back in. Not convinced this isn't an ordering issue.

@adbridge
Copy link
Contributor

Mmm can't see why this won't automatically apply to the release. Will try manually patching this when all others have gone in...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.