Remove deprecated --legacyalign from ARMC6 link #10373

kjbracey · 2019-04-11T07:14:54Z

Description

This previously had to be reinstated to avoid failures on certain platforms due to their linker map not having sufficient alignment.

We now have the test infrastructure in place to attempt to remove it and find failures.

Pull request type

[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@deepikabhavnani, @JanneKiiskila

kjbracey · 2019-04-11T10:40:51Z

Going to be a sequence of scatter file adjustments here - breaking them up into 1 commit per vendor - seem commit messages for notes.

deepikabhavnani · 2019-04-11T14:38:55Z

targets/TARGET_Maxim/TARGET_MAX32600/device/TOOLCHAIN_ARM_STD/MAX32600.sct

@@ -19,8 +19,7 @@ LR_IROM1 0x00000000 0x40000  {    ; load region size_region
        .ANY (+RO)
    }

-    ; [RAM] Vector table dynamic copy: 79 vectors * 4 bytes = (0x140) - alignment
-    RW_IRAM1 (0x20000000+0x140) (0x8000-0x140-Stack_Size)  {  ; RW data
+    RW_IRAM1 (0x20000000) (0x8000-Stack_Size)  {  ; RW data


Why is vector size removed from start of RAM?
In order to remove --legacyalign every section should be 8-byte aligned and for that I updated the linker scripts (most) to have vectors and all other sections to be 8-byte aligned.

Explanation is in commit message: 3fea439

Not sure why Maxim is being unusual here - alternatively we could change their code to work like all the other targets.

But I do actually prefer Maxim's approach. It seems a lot simpler - just put the vectors in the normal RAM with an align directive.

Only reason I can see for the special linker map handling is that maybe it avoids stupid padding? The vectors do have a high alignment requirement (512-byte or more), so putting them at the front avoids a linker being dumb and sticking them in the middle with a huge padding block in front of them.

But are any of the linkers really that dumb?

cc @ARMmbed/team-maximintegrated

Or maybe that commit message isn't clear enough.

Because the Maxim build puts its vectors in main RAM, the main RAM needs to be 512-byte aligned, if you want to turn --legacyalign off.

Taking that unused vector space out makes the main RAM 512-byte aligned.

The vector space is not there in the IAR and GCC maps.

deepikabhavnani · 2019-04-11T14:40:42Z

targets/TARGET_Maxim/TARGET_MAX32600/device/TOOLCHAIN_IAR/MAX32600.icf

-/* [RAM] Vector table dynamic copy: 79 vectors * 4 bytes = 316 bytes (0x13C) */
-define symbol __NVIC_start__          = 0x00000000;
-define symbol __NVIC_end__            = 0x00000140; /* to be aligned on 8 bytes */
+/* [RAM] */


vector size was 79*4=0x13C (not aligned to 8-byte)
Was updated to be 8-byte aligned as 0x140. I am not sure why are we removing vector region from RAM ?

deepikabhavnani

In order to remove --legacyalign we need all regions to be 8-byte aligned in linker script.
~~Please explain why vector region is removed form RAM?~~ OK i see it explained in commit message, but I don;t think it should be part of this PR. It will be good to have it as a separate PR for Maxim

cmonr · 2019-04-12T01:41:03Z

@deepikabhavnani Having a seperate commit, while still being related to the PR, is acceptable.
We can get them to review the PR.

@ARMmbed/mbed-os-maintainers Thoughts?

deepikabhavnani · 2019-04-12T01:51:11Z

@cmnor - both commits are completely different its two different topics touching linker files.

cmonr · 2019-04-12T02:09:58Z

@deepikabhavnani Ah! So it is.

@kjbracey-arm Please either split those commits into their own PR, or make them more relevant to this PR.

kjbracey · 2019-04-12T08:57:28Z

Having made the Maxim changes I'm now getting "Region table handler '__scatterload_copy' needed by entry for RW_IRAM1 was not found." on its build, and I can't figure out why. More work required.

mbed-ci · 2019-04-16T19:27:37Z

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM

kjbracey · 2019-04-25T10:07:31Z

The Maxim approach looks neat, but it doesn't actually work well. It ends up wasting RAM and ROM space because of the large alignment requirement, and the block just gets blopped in the middle of all the static data, with up to 512 bytes of padding.

So I'm going to change the Maxim devices to put the vectors explicitly at the RAM start like other targets.

@ARMmbed/team-maximintegrated ?

Putting the vectors in the main RAM region with 512-byte alignment causes a number of issues - linker tends to put padding in, and not fill the gap nicely. The ARM toolchain actually still had space reserved for the vectors anyway, and this triggered alignment problems when legacyalign was turned off - the unused vector space meant the main RAM wasn't 512-byte aligned. This commit removes the vector table array from main RAM, and positions the vectors at RAM start, like other targets. This avoids any excess padding. Sizes in the pre-existing ARM maps were in some cases not correct - either missing the 16 entries for the built-in exceptions, or having a slightly the wrong number of interrupts (cross-referencing against MXC_IRQ_COUNT - 32600/32610 = 79, 32620/32625 = 81, 32620C/32630 = 84). For MAX32625NEXPAQ builds, move the RAM split base to align it to 512 bytes - this is the alignment requirement for the vectors at the start. squash! Maxim: ARM alignment, remove unneeded vector space squash! Maxim: Position vectors manually, like other targets

Make sure the default handlers branch to themselves, rather than branching to the non-weak definition. Should have no effect, but avoids build warnings.

maclobdell · 2019-04-25T13:44:20Z

@kjbracey-arm Please avoid changing partner code without getting their review and approval. Please engage with partners via @ARmmbed/Team-[Partner] or with the TAM group to discuss proposals, get feedback, or alert them to a change that affects their targets.

0xc0170 · 2019-05-03T13:08:13Z

@maclobdell will do, I'll tag them the remaining teams now

0xc0170 · 2019-05-03T13:09:25Z

@ARMmbed/team-maximintegrated @ARMmbed/team-st-mcd Please review

maclobdell · 2019-05-03T13:39:41Z

cc @bentcooke - please help with the review

adbridge · 2019-05-17T09:03:12Z

@maclobdell @bentcooke any update on this? Code freeze for 5.13 is one week today...

0xc0170 · 2019-05-22T14:12:16Z

@ARMmbed/team-st-mcd Please review

@ARMmbed/team-st-mcd Can you please review?

0xc0170 · 2019-05-22T14:12:49Z

I started CI while finalizing reviews

mbed-ci · 2019-05-22T15:22:33Z

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM

0xc0170 · 2019-05-23T08:36:10Z

@kjbracey-arm The build errors are related - usb tests fail to link with an error __scatterload_copy needed by entry for. I recall we discussed this earlier, what was the solution?

mark-edgeworth

This change includes many other changes which appear not to be related to the simple removal of the legacyalign flag. These should be in a separate PR in my view.

hugueskamba · 2019-05-23T11:12:25Z

This change includes many other changes which appear not to be related to the simple removal of the legacyalign flag. These should be in a separate PR in my view.

@mark-edgeworth Removing the linker option causes a failure to build for some targets. The other changes are necessary to meet the following requirement of the Mbed OS workflow

Each commit should be the minimum self-contained commit for a change. A commit should always result in a new state that is again in a compilable state. You should (if possible) split large changes into logical smaller commits that help reviewers follow the reasoning behind the full change.

Source: https://os.mbed.com/docs/mbed-os/v5.12/contributing/workflow.html

kjbracey · 2019-05-23T11:18:30Z

usb tests fail to link with an error __scatterload_copy needed by entry for. I recall we discussed this earlier, what was the solution?

There wasn't a proper solution. It appeared to be a linker bug triggered by certain layouts with large alignment. I avoided it by backing away from the "put vectors in general RAM" approach for Maxim.

I'll look at the new failure to see what I can figure out. If it is a linker bug, we might be stuck for a while.

I think this may have to miss 5.13 - I don't think I'll have time to investigate that in time with the other stuff I've got open, and removal of this flag isn't urgent. PR was initially prompted by some suspicion that other weird behaviour may be linked to it, but that issue has been resolved.

kjbracey · 2019-05-23T11:23:07Z

This change includes many other changes which appear not to be related to the simple removal of the legacyalign flag. These should be in a separate PR in my view.

The other changes must be made before the flag can be turned off.

I guess it could be split into a first PR that makes everything aligned, and a second PR that turns off the flag. But GitHub really doesn't know how to handle dependent PRs nicely. You'd have one PR with 4 commits and another with the same 4 commits and 1 more.

Each commit within this dependent series is fairly neatly self-contained - selecting the individual commits to review in the GitHub UI is probably clearer than a trying to look at separate PRs.

0xc0170 · 2019-05-24T08:02:30Z

Removed 5.13 label for now

bulislaw · 2019-05-28T11:35:46Z

It make sense to have it in one PR, if it's all related (change + fix).

0xc0170 · 2019-07-09T12:17:34Z

@kjbracey-arm should we continue with this one for 5.14?

kjbracey · 2019-07-09T12:20:25Z

This one's fallen to the bottom of my priority queue, so not sure when I'll be getting back around to it. I don't believe the flag is currently having any ill effects, so maybe park it altogether for now.

0xc0170 · 2019-07-09T12:22:49Z

Let's close it for now.

0xc0170 approved these changes Apr 11, 2019

View reviewed changes

0xc0170 added needs: review release-version: 5.13.0-rc1 labels Apr 11, 2019

0xc0170 requested review from deepikabhavnani and JanneKiiskila April 11, 2019 07:52

deepikabhavnani reviewed Apr 11, 2019

View reviewed changes

deepikabhavnani suggested changes Apr 11, 2019

View reviewed changes

cmonr added needs: work and removed needs: review labels Apr 12, 2019

kjbracey added 5 commits April 25, 2019 13:49

Remove deprecated --legacyalign from ARMC6 link

048fcb9

STM: Fix up ARMC6 alignment

f01760b

Maxim: Fix default handler build warnings

428c8ff

Make sure the default handlers branch to themselves, rather than branching to the non-weak definition. Should have no effect, but avoids build warnings.

ARM CM3DS: Align RAM for ARM linker

0699512

kjbracey force-pushed the no_legacyalign branch from 717ee98 to 0699512 Compare April 25, 2019 11:14

0xc0170 added needs: review and removed needs: work labels May 3, 2019

jeromecoutant approved these changes May 22, 2019

View reviewed changes

0xc0170 mentioned this pull request May 23, 2019

tools: Fix build output warning message #10637

Closed

0xc0170 added needs: work and removed needs: review labels May 23, 2019

0xc0170 requested review from evedon and a team May 23, 2019 08:36

mark-edgeworth suggested changes May 23, 2019

View reviewed changes

0xc0170 removed the release-version: 5.13.0-rc1 label May 24, 2019

bulislaw approved these changes Jun 21, 2019

View reviewed changes

kjbracey mentioned this pull request Jun 26, 2019

bootloader: Fix LPC55S69 bootloader segmentation #10791

Merged

0xc0170 closed this Jul 9, 2019

kjbracey mentioned this pull request Jan 14, 2021

Remove systematic 'legacyalign' warning #14151

Closed

0xc0170 mentioned this pull request Apr 7, 2021

Remove systematic 'legacyalign' warning #14290

Open

Remove deprecated --legacyalign from ARMC6 link #10373

Remove deprecated --legacyalign from ARMC6 link #10373

Uh oh!

Conversation

kjbracey commented Apr 11, 2019

Description

Pull request type

Reviewers

Uh oh!

kjbracey commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepikabhavnani Apr 11, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

0xc0170 Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

kjbracey Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani Apr 11, 2019

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Apr 12, 2019

Uh oh!

deepikabhavnani commented Apr 12, 2019

Uh oh!

cmonr commented Apr 12, 2019

Uh oh!

kjbracey commented Apr 12, 2019

Uh oh!

mbed-ci commented Apr 16, 2019

Test run: FAILED

Uh oh!

kjbracey commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maclobdell commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented May 3, 2019

Uh oh!

0xc0170 commented May 3, 2019

Uh oh!

maclobdell commented May 3, 2019

Uh oh!

adbridge commented May 17, 2019

Uh oh!

0xc0170 commented May 22, 2019

Uh oh!

0xc0170 commented May 22, 2019

Uh oh!

mbed-ci commented May 22, 2019

Test run: FAILED

Uh oh!

0xc0170 commented May 23, 2019

Uh oh!

mark-edgeworth left a comment

Choose a reason for hiding this comment

Uh oh!

hugueskamba commented May 23, 2019

Uh oh!

kjbracey commented May 23, 2019

Uh oh!

kjbracey commented May 23, 2019

Uh oh!

0xc0170 commented May 24, 2019

Uh oh!

bulislaw commented May 28, 2019

Uh oh!

0xc0170 commented Jul 9, 2019

Uh oh!

kjbracey commented Apr 11, 2019 •

edited

Loading

deepikabhavnani left a comment •

edited

Loading

kjbracey commented Apr 25, 2019 •

edited

Loading

maclobdell commented Apr 25, 2019 •

edited

Loading