This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Emit code alignment after .arm and .thumb directives
ClosedPublic

Authored by antangelo on Apr 6 2023, 10:30 PM.

Details

Summary

Emit a 4-byte alignment after the .arm directive and a 2-byte alignment
after the .thumb directive. The new behavior matches GNU assembler.

Fixes #53386

Diff Detail

Event Timeline

antangelo created this revision.Apr 6 2023, 10:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2023, 10:30 PM
antangelo requested review of this revision.Apr 6 2023, 10:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2023, 10:30 PM
antangelo updated this revision to Diff 511770.Apr 7 2023, 12:29 PM

Fix lld tests, rebase

MaskRay added inline comments.
lld/test/ELF/arm-fix-cortex-a8-nopatch.s
94

This address adjustment will break what the test wants to check. The tests need a proper update.

@peter.smith

antangelo updated this revision to Diff 512330.Apr 10 2023, 9:11 PM

Fixed arm-fix-cortex-a8-nopatch test cases, added back a check in -recognized that was accidentally removed

I changed the offsets around the .arm directive such that the instruction offsets are the same as before this patch.
The only exception is that the target5 label has moved slightly, but it is still in region 1, so the test case should
be valid now to my knowledge.

Tests updates look OK to me.

A couple of comments overall.

  • While I personally prefer the proposed behaviour as I don't think anyone other than CPU and toolchain validation tests should want to write an unaligned instruction; this is a difference in behaviour with GNU assembler. Can we add to the Arm section of the LLVM release notes to the next release? Could also be worth a binutils bug to see if we can get GNU to follow.
  • If .arm is going to be aligning to a 4-byte boundary then .thumb should be aligning to a 2-byte boundary. It doesn't look like it is at the moment. While less likely to be a problem in practice I think it is important that it gets done so that the behaviour of the directives is consistent. Could be done in a follow up patch.

A couple of comments overall.
... Can we add to the Arm section of the LLVM release notes to the next release? Could also be worth a binutils bug to see if we can get GNU to follow.

FYI It's llvm/docs/ReleaseNotes.rst Changes to the ARM Backend

Rebased, updated release notes, updated .thumb to align to a 2-byte boundary, and added a test case

To my knowledge (testing with arm-none-eabi), the GNU assembler aligns code after the .arm directive to the next
4-byte boundary when switching from thumb to ARM mode. It does not seem to emit any alignment when switching
from ARM to thumb mode with .thumb.

antangelo updated this revision to Diff 521544.May 11 2023, 8:11 PM
antangelo retitled this revision from [ARM] Align code to 4-byte boundary after .arm directive to [ARM] Emit code alignment after .arm and .thumb directives.
antangelo edited the summary of this revision. (Show Details)

Rebased and updated the commit message to reflect the .thumb directive changes

Also noting that I don't have commit access to commit this myself after the review is completed

Sorry for the delay.

llvm/docs/ReleaseNotes.rst
110

Two backsticks in reST for something mostly equivalent in Markdown

llvm/test/MC/ARM/arm-thumb-directive-alignment.s
1 ↗(On Diff #521544)

For directive tests, the most common filenames are directive-*.

We should test section headers as well: llvm-readelf -S -s . Define a custom section and check that its alignment is affected by .arm. Then define another section and check its alignment is affected by .thumb

22 ↗(On Diff #521544)

The name unaligned_thumb_to_arm may be confusing as the value is actually aligned. thumb_to_arm suffices.

MaskRay added inline comments.May 25 2023, 7:41 PM
lld/test/ELF/arm-fix-cortex-a8-nopatch.s
94

With this change, the test will pass even if I remove --fix-cortex-a8. This means the directive semantics change nullifies the purpose of the test. I think we should repair the test properly.

MaskRay added inline comments.May 25 2023, 7:43 PM
lld/test/ELF/arm-fix-cortex-a8-recognize.s
168

I suspect that this may need a proper fix as well.

antangelo added inline comments.May 25 2023, 9:29 PM
lld/test/ELF/arm-fix-cortex-a8-nopatch.s
94

With this change, the test will pass even if I remove --fix-cortex-a8. This means the directive semantics change nullifies the purpose of the test. I think we should repair the test properly.

I've tried this on the current main branch and this test passes with --fix-cortex-a8 removed. Is it possible the test itself is in disrepair?

Rebase and addressing feedback

Updated the release notes syntax, fixed the directive-arm-thumb-alignment test file name,
and added tests for section alignment.

After studying the tests again more closely, to my understanding, the nopatch tests
are for edge cases where the fix should not be applied, so having the same result when --fix-cortex-a8
is removed seems to be correct. I also noticed a bug where the nop.w instruction that is manually
encoded in that test is encoded incorrectly, so I have updated that. If the .arm directive
under target5 is removed, the case is now detected as requiring the cortex-a8 fix (so it is now testing
the right thing).

I have verified the offsets in both the nopatch and recognize suites and they match what is currently
in main (and match the condition that triggers the fix). I don't otherwise know what to look for in
terms of what could be incorrect there. Is there anything about those tests in particular that need updating,
or something else I should verify against?

MaskRay accepted this revision.May 27 2023, 6:50 PM
MaskRay added inline comments.
llvm/test/MC/ARM/directive-arm-thumb-alignment.s
6

Replace Off column values with {{.*}}. The offsets are not interesting for the purpose of the test and will cause unneeded trouble for changes that shuffle section order/string table optimization/etc, see https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-too-much .

Replace Size column value of .strtab with {{.*}}

65

Delete trailing blan lines

This revision is now accepted and ready to land.May 27 2023, 6:50 PM

Replace offset and strtab size columns with wildcard, remove trailing newline in directive test

I don't have commit access to commit the changes myself, if someone can commit on my behalf. My name and email are:

Name: Antonio Abbatangelo
Email: contact@antangelo.com

MaskRay edited the summary of this revision. (Show Details)Jun 1 2023, 9:58 AM
MaskRay edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Jun 1 2023, 11:05 AM
This revision was automatically updated to reflect the committed changes.