-xcoff-traceback-table is a default option on AIX regardless of optimization and debug levels. An error of relocation for paired relocatable term is not yet supported in XCOFFObjectWriter::recordRelocation occurred when both of the -xcoff-traceback-table and -function-sections are enabled.
The root cause is that we missed to calculate the symbols difference as absolute value before adding fixups when symbol_A without the fragment set is the csect itself and symbol_B is in it.
This patch only sets the fragment for XMC_PR csects because we don't have other cases that hit this problem yet.
Details
Details
- Reviewers
shchenz DiggerLin hubert.reinterpretcast - Group Reviewers
Restricted Project - Commits
- rGc7c7ef8bda1f: [XCOFF] set fragment for XMC_PR csects.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp | ||
|---|---|---|
| 2223 ↗ | (On Diff #472873) | Can we address this where the MCExpr is handled instead? There may be more ways a similar expression involving a csect as an operand is involved in the future (e.g., assembly parsing). |
| llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp | ||
|---|---|---|
| 2223 ↗ | (On Diff #472873) | I do not think we should set setFragment of FuncSectSym in the function emitTracebackTable.
I think we may need to set the The fragment value to CurrentFnSym of AsmPrinter in somewhere when "integrated-as" it will cause the function MCExpr::evaluateAsRelocatableImpl() return "false" when evaluation the fixup of CurrentFnSym-FuncEnd |
| llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp | ||
|---|---|---|
| 2223 ↗ | (On Diff #472873) | maybe you can try to set the fragment of QualName in the function MCSectionXCOFF *MCContext::getXCOFFSection. if (Begin)
Begin->setFragment(F);
+ if (!IsDwarfSec && CsectProp->MappingClass ==XCOFF::XMC_PR){
+ QualName->setFragment(F);
+ QualName->setOffset(0);
+ }not sure whether it work or not. |
| llvm/lib/MC/MCContext.cpp | ||
|---|---|---|
| 833 | sorry that symbol offset is default by 0 . So we do not need to do QualName->setOffset(0); | |
| llvm/test/CodeGen/PowerPC/aix-xcoff-funcsect.ll | ||
|---|---|---|
| 8 | do we want to test the 32bit too ? | |
| llvm/lib/MC/MCContext.cpp | ||
|---|---|---|
| 831 | The change looks reasonable, but I don't think it should be restricted to XMC_PR csects. If we are being conservative about making changes, then let's leave a comment to say that the check for XMC_PR is here just because we don't have other cases that hit this problem yet. We will probably find more to do if we can get the assembly parsing to resolve qualnames. | |
| llvm/test/CodeGen/PowerPC/aix-xcoff-funcsect.ll | ||
| 11 | Please use --symbol-description and update the checks to expect the storage mapping class. | |
| 16 | Same comment: Please use --symbol-description and update the checks to expect the storage mapping class. | |
Comment Actions
Thanks. Can you update the title and description?
The title should indicate "what" (set fragment). The description can further explain the "why".
Comment Actions
I am a bit torn over not actually checking that the calculated function sizes are correct, but I guess that we can infer from there being no relocations generated for it that we are going through the right MCExpr processing.
Comment Actions
LGTM; no objection if we're planning to move ahead without checking the calculated size field in the traceback table.
The change looks reasonable, but I don't think it should be restricted to XMC_PR csects.
If we are being conservative about making changes, then let's leave a comment to say that the check for XMC_PR is here just because we don't have other cases that hit this problem yet.
We will probably find more to do if we can get the assembly parsing to resolve qualnames.