-
Notifications
You must be signed in to change notification settings - Fork 15.2k
MC: Centralize X86 PC-relative fixup adjustment in MCAssembler #147113
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
Changes from 8 commits
c5eb84e
a7f6b41
7f477b9
ae33010
34ce16c
e42b897
0c5ca58
8c9fcae
6572856
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -587,22 +587,11 @@ void X86MCCodeEmitter::emitImmediate(const MCOperand &DispOp, SMLoc Loc, | |
| } | ||
| } | ||
|
|
||
| // If the fixup is pc-relative, we need to bias the value to be relative to | ||
| // the start of the field, not the end of the field. | ||
| if (PCRel) { | ||
| ImmOffset -= Size; | ||
| // If this is a pc-relative load off _GLOBAL_OFFSET_TABLE_: | ||
| // leaq _GLOBAL_OFFSET_TABLE_(%rip), %r15 | ||
| // this needs to be a GOTPC32 relocation. | ||
| if (Size == 4 && startsWithGlobalOffsetTable(Expr) != GOT_None) | ||
| FixupKind = X86::reloc_global_offset_table; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't removing this have an effect on test output?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has no effect on all the In 2020, I added this piece of code in reviews.llvm.org/D47507. I am actually not sure I got everything correct, but moving the code from MCCodeEmitter (parse time) to AsmBackend (relocation decision time) is the right direction. |
||
| } | ||
|
|
||
| if (ImmOffset) | ||
| Expr = MCBinaryExpr::createAdd(Expr, MCConstantExpr::create(ImmOffset, Ctx), | ||
| Ctx, Expr->getLoc()); | ||
|
|
||
| // Emit a symbolic constant as a fixup and 4 zeros. | ||
| // Emit a symbolic constant as a fixup and a few zero bytes. | ||
| Fixups.push_back(MCFixup::create(static_cast<uint32_t>(CB.size() - StartByte), | ||
| Expr, FixupKind, PCRel)); | ||
| emitConstant(0, Size, CB); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing some context here. What fixup kinds, other than
X86::reloc_global_offset_table, can end up hitting this code path? startsWithGlobalOffsetTable does some more checks, and is still called in emitImmediate -- could the different checks cause problems? I don't quite like duplicating checks for magical symbol names.. Couldn't we keep the GOT-check in emitImmediate and just adjust the offset here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 2020, I implemented this code path for all PC-relative fixups in https://reviews.llvm.org/D47507 (I overlooked adding corresponding mov tests).
The current implementation is guarded by
if (Fixup.isPCRel()) {, reflecting the original intention. I think testing the magic symbol during the relocation decision phase (https://maskray.me/blog/2025-03-16-relocation-generation-in-assemblers#relocation-decision-phase) is the correct approach. Transitioning anotherstartsWithGlobalOffsetTablecaller in X86MCCodeEmitter.cpp to use X86MCAsmBackend.cpp would be ideal, but that falls outside this change’s scope. X86MCCodeEmitter.cpp has quite a lot of tech debt.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. It would be good to transition the comment from startsWithGlobalOffsetTable here.