[JITLink][AArch32] Fix Unaligned Data Symbol Address Resolution #97030
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The ARM architecture uses the LSB bit for ARM/Thumb mode switch flagging. This is true for alignments of 2 and 4 but in data relocations the alignment is 1 allowing the LSB bit to be set. ELF::STT_OBJECT typed symbols are made to bypass the TargetFlag mechanism assuming they are the only symbols affected by data relocations. The test is a minimal example of the issue mentioned below. Fixes the issue "Orc global constructor order test fails on 32 bit ARM llvm#95911".
eymay
requested review from
DavidSpickett and
weliveindetail
and removed request for
DavidSpickett
| @@ -200,6 +200,9 @@ class ELFLinkGraphBuilder_aarch32 | |||
|
|
|||
| protected: | |||
| TargetFlagsType makeTargetFlags(const typename ELFT::Sym &Sym) override { | |||
| // Data symbols do not have Arm or Thumb flags. | |||
| if (Sym.getType() == ELF::STT_OBJECT) | |||
| return TargetFlagsType{}; | |||
There was a problem hiding this comment.
With your fix in getRawOffset() this check shouldn't be necessary anymore, isn't it? Instead we might want to assert that all symbols with the ThumbSymbol target flag are ELF::STT_FUNC.
There was a problem hiding this comment.
Agree but I had to keep it because of a failed assert in
There was a problem hiding this comment.
With your fix in
getRawOffset()this check shouldn't be necessary anymore, isn't it? Instead we might want to assert that all symbols with theThumbSymboltarget flag areELF::STT_FUNC.
Updated the condition to check for STT_FUNC. I found a similar assert to what you mentioned in the previous comment, should I still add one?
| .asciz "H1" | ||
| .size Lstr.H1, 3 | ||
|
|
||
| # Not 0x324800 |
There was a problem hiding this comment.
Is there anything special here compared to the other two cases? Or is this a left-over comment?
There was a problem hiding this comment.
There is actually, this 'H2' string is the critical part which caused the errors. Before the fix we were getting 0x324800 as its address was indexed by 0x2 instead of 0x3. I wanted something like CHECK-NOT though we do not have it in Checker infra currently. So I left it as comment there.
|
Nice work, I would have had 0 chance of finding this. I've tested this natively. The new test and the existing test are now passing. |
weliveindetail
approved these changes
Jun 28, 2024
| # FIXME: The expression we want is either *{3}(Lstr.H1) = ... | ||
| # or *{4}(Lstr.H1) & 0x00ffffff = ... | ||
| # The first is not supported and the latter segfaults. | ||
| # Also, whitespaces are not recognized and not consumed by the checker. |
There was a problem hiding this comment.
I think you could write either (*{4}(Lstr.H1))[23:0] or (*{4}(Lstr.H1)) & 0x00ffffff. Does that work?
There was a problem hiding this comment.
It does! Thanks for the tip.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/849 Here is the relevant piece of the build log for the reference: |
eymay
added a commit
that referenced
this pull request
Jun 29, 2024
… build The expressive test added in PR #97030 requires debug option in cli.
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
…#97030) The ARM architecture uses the LSB bit for ARM/Thumb mode switch flagging. This is true for alignments of 2 and 4 but in data relocations the alignment is 1 allowing the LSB bit to be set. Now only `ELF::STT_FUNC` typed symbols are used in the TargetFlag mechanism. The test is a minimal example of the issue mentioned below. Fixes llvm#95911 "Orc global constructor order test fails on 32 bit ARM".
lravenclaw
pushed a commit
to lravenclaw/llvm-project
that referenced
this pull request
Jul 3, 2024
… build The expressive test added in PR llvm#97030 requires debug option in cli.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The ARM architecture uses the LSB bit for ARM/Thumb mode switch
flagging. This is true for alignments of 2 and 4 but in data
relocations the alignment is 1 allowing the LSB bit to be set.
ELF::STT_OBJECTtyped symbols are made to bypass the TargetFlagmechanism assuming they are the only symbols affected by data
relocations.
The test is a minimal example of the issue mentioned below.
Fixes #95911 "Orc global constructor order test fails on 32
bit ARM".