[XCOFF] Use RLDs to print branches even without -r #74342
Merged
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
This presents misleading and confusing output.
|
@llvm/pr-subscribers-llvm-binary-utilities Author: None (stephenpeckham) ChangesThis presents misleading and confusing output. If you have a function defined at the beginning of an XCOFF object file, and you have a function call to an external function, the function call disassembles as a branch to the local function. That is,
disassembles as With this PR, the second call will display:
Using -r can help, but you still get the confusing output: >10: 4b ff ff f1 bl 0x0 <.f> Full diff: https://github.com/llvm/llvm-project/pull/74342.diff 3 Files Affected:
diff --git a/llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands.ll b/llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands.ll
index adedb6b7a5abf..2b4d6806292ce 100644
--- a/llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands.ll
+++ b/llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands.ll
@@ -21,7 +21,7 @@
; CHECK-NEXT: 68: cmplwi 3, 11
; CHECK-NEXT: 6c: bt 0, 0x60 <L2>
; CHECK-NEXT: 70: mr 31, 3
-; CHECK-NEXT: 74: bl 0x0 <.internal>
+; CHECK-NEXT: 74: bl 0x0 <.extern>
; CHECK-NEXT: 78: nop
; CHECK-NEXT: 7c: mr 3, 31
; CHECK-NEXT: 80: b 0x60 <L2>
diff --git a/llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands2.ll b/llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands2.ll
new file mode 100644
index 0000000000000..48ce2e7cc5514
--- /dev/null
+++ b/llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands2.ll
@@ -0,0 +1,62 @@
+; RUN: llc -mtriple=powerpc-ibm-aix-xcoff %s -filetype=obj -o %t
+; RUN: llvm-objdump %t -d --symbolize-operands --no-show-raw-insn \
+; RUN: | FileCheck %s
+
+; CHECK-LABEL: <.a>:
+;; No <L0> should appear
+; CHECK-NEXT: 0: mflr 0
+; CHECK-NEXT: 4: stwu 1, -64(1)
+; CHECK-NEXT: 8: lwz 3, 0(2)
+; CHECK-NEXT: c: stw 0, 72(1)
+; CHECK-NEXT: 10: lwz 3, 0(3)
+; CHECK-NEXT: 14: bl 0x4c <.b>
+; CHECK-NEXT: 18: nop
+; CHECK-NEXT: 1c: li 3, 1
+; CHECK-NEXT: 20: bl 0x0 <.c>
+
+; CHECK-LABEL: <.b>:
+; CHECK-NEXT: 4c: mflr 0
+; CHECK-NEXT: 50: stwu 1, -64(1)
+; CHECK-NEXT: 54: cmplwi 3, 1
+; CHECK-NEXT: 58: stw 0, 72(1)
+; CHECK-NEXT: 5c: stw 3, 60(1)
+; CHECK-NEXT: 60: bf 2, 0x6c <L0>
+; CHECK-NEXT: 64: bl 0x0 <.a>
+; CHECK-NEXT: 68: nop
+; CHECK-NEXT:<L0>:
+; CHECK-NEXT: 6c: li 3, 2
+; CHECK-NEXT: 70: bl 0x0 <.c>
+
+target triple = "powerpc-ibm-aix7.2.0.0"
+
+@var = external global i32, align 4
+
+; Function Attrs: noinline nounwind optnone
+define i32 @a() {
+entry:
+ %0 = load i32, ptr @var, align 4
+ %call = call i32 @b(i32 noundef %0)
+ %call1 = call i32 @c(i32 noundef 1)
+ ret i32 %call1
+}
+
+; Function Attrs: noinline nounwind optnone
+define i32 @b(i32 noundef %x) {
+entry:
+ %x.addr = alloca i32, align 4
+ store i32 %x, ptr %x.addr, align 4
+ %0 = load i32, ptr %x.addr, align 4
+ %cmp = icmp eq i32 %0, 1
+ br i1 %cmp, label %if.then, label %if.end
+
+if.then: ; preds = %entry
+ %call = call i32 @a()
+ br label %if.end
+
+if.end: ; preds = %if.then, %entry
+ %call1 = call i32 @c(i32 noundef 2)
+ ret i32 %call1
+}
+
+declare i32 @c(i32 noundef)
+
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 2b7dee7bf46b9..8645625a367f7 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -1292,6 +1292,8 @@ collectLocalBranchTargets(ArrayRef<uint8_t> Bytes, MCInstrAnalysis *MIA,
// So far only supports PowerPC and X86.
if (!STI->getTargetTriple().isPPC() && !STI->getTargetTriple().isX86())
return;
+ const bool isXCOFF = STI->getTargetTriple().isOSBinFormatXCOFF();
+ const bool isPPC = STI->getTargetTriple().isPPC();
if (MIA)
MIA->resetState();
@@ -1312,18 +1314,22 @@ collectLocalBranchTargets(ArrayRef<uint8_t> Bytes, MCInstrAnalysis *MIA,
Size = std::min<uint64_t>(ThisBytes.size(),
DisAsm->suggestBytesToSkip(ThisBytes, Index));
- if (Disassembled && MIA) {
- uint64_t Target;
- bool TargetKnown = MIA->evaluateBranch(Inst, Index, Size, Target);
- // On PowerPC, if the address of a branch is the same as the target, it
- // means that it's a function call. Do not mark the label for this case.
- if (TargetKnown && (Target >= Start && Target < End) &&
- !Labels.count(Target) &&
- !(STI->getTargetTriple().isPPC() && Target == Index))
- Labels[Target] = ("L" + Twine(LabelCount++)).str();
- MIA->updateState(Inst, Index);
- } else if (!Disassembled && MIA) {
- MIA->resetState();
+ if (MIA) {
+ if (Disassembled) {
+ uint64_t Target;
+ bool TargetKnown = MIA->evaluateBranch(Inst, Index, Size, Target);
+ if (TargetKnown && (Target >= Start && Target < End) &&
+ !Labels.count(Target)) {
+ // On PowerPC and AIX, a function call is encoded as a branch to 0.
+ // On other PowerPC platforms (ELF), a function call is encoded as
+ // a branch to self. Do not add a label for these cases.
+ if (!(isPPC && ((Target == 0 && isXCOFF) ||
+ (Target == Index && !isXCOFF))))
+ Labels[Target] = ("L" + Twine(LabelCount++)).str();
+ }
+ MIA->updateState(Inst, Index);
+ } else
+ MIA->resetState();
}
Index += Size;
}
@@ -1487,7 +1493,7 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
}
std::map<SectionRef, std::vector<RelocationRef>> RelocMap;
- if (InlineRelocs)
+ if (InlineRelocs || Obj.isXCOFF())
RelocMap = getRelocsMap(Obj);
bool Is64Bits = Obj.getBytesInAddress() > 4;
@@ -1979,6 +1985,8 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
DT->InstrAnalysis->resetState();
while (Index < End) {
+ uint64_t RelOffset;
+
// ARM and AArch64 ELF binaries can interleave data and text in the
// same section. We rely on the markers introduced to understand what
// we need to dump. If the data marker is within a function, it is
@@ -2013,6 +2021,29 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
}
}
+ auto findRel = [&]() {
+ // Hexagon handles relocs in pretty printer
+ if (Obj.getArch() == Triple::hexagon)
+ return false;
+ while (RelCur != RelEnd) {
+ RelOffset = RelCur->getOffset() - RelAdjustment;
+ // If this relocation is hidden, skip it.
+ if (getHidden(*RelCur) || SectionAddr + RelOffset < StartAddress) {
+ ++RelCur;
+ continue;
+ }
+
+ // Stop when RelCur's offset is past the disassembled
+ // instruction/data.
+ if (RelOffset >= Index + Size)
+ return false;
+ if (RelOffset >= Index)
+ return true;
+ ++RelCur;
+ }
+ return false;
+ };
+
if (DumpARMELFData) {
Size = dumpARMELFData(SectionAddr, Index, End, Obj, Bytes,
MappingSymbols, *DT->SubtargetInfo, FOS);
@@ -2081,17 +2112,19 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
DT->InstPrinter->setCommentStream(llvm::nulls());
- // If disassembly has failed, avoid analysing invalid/incomplete
- // instruction information. Otherwise, try to resolve the target
- // address (jump target or memory operand address) and print it on the
+ // If disassembly succeeds, we try to resolve the target address
+ // (jump target or memory operand address) and print it to the
// right of the instruction.
+ //
+ // Otherwise, we don't print anything else so that we avoid
+ // analyzing invalid or incomplete instruction information.
if (Disassembled && DT->InstrAnalysis) {
- // Branch targets are printed just after the instructions.
llvm::raw_ostream *TargetOS = &FOS;
uint64_t Target;
bool PrintTarget = DT->InstrAnalysis->evaluateBranch(
Inst, SectionAddr + Index, Size, Target);
- if (!PrintTarget)
+
+ if (!PrintTarget) {
if (std::optional<uint64_t> MaybeTarget =
DT->InstrAnalysis->evaluateMemoryOperandAddress(
Inst, DT->SubtargetInfo.get(), SectionAddr + Index,
@@ -2105,6 +2138,8 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
*TargetOS << "0x" << Twine::utohexstr(Target);
}
}
+ }
+
if (PrintTarget) {
// In a relocatable object, the target's section must reside in
// the same section as the call instruction or it is accessed
@@ -2114,7 +2149,8 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
// In that case, locate the section(s) containing the target
// address and find the symbol in one of those, if possible.
//
- // N.B. We don't walk the relocations in the relocatable case yet.
+ // N.B. Except for XCOFF, we don't walk the relocations in the
+ // relocatable case yet.
std::vector<const SectionSymbolsTy *> TargetSectionSymbols;
if (!Obj.isRelocatableObject()) {
auto It = llvm::partition_point(
@@ -2160,9 +2196,11 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
break;
}
+ // Branch targets are printed just after the instructions.
// Print the labels corresponding to the target if there's any.
bool BBAddrMapLabelAvailable = BBAddrMapLabels.count(Target);
bool LabelAvailable = AllLabels.count(Target);
+
if (TargetSym != nullptr) {
uint64_t TargetAddress = TargetSym->Addr;
uint64_t Disp = Target - TargetAddress;
@@ -2170,9 +2208,21 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
: TargetSym->Name.str();
*TargetOS << " <";
- if (!Disp) {
- // Always Print the binary symbol precisely corresponding to
- // the target address.
+ // On XCOFF, we use relocations, even without -r, so we
+ // can print the correct name for an extern function call.
+ if (Obj.isXCOFF() && findRel()) {
+ SmallString<32> Val;
+
+ // If we have a valid relocation, try to print the
+ // corresponding symbol name. Multiple relocations on the
+ // same instruction are not handled.
+ if (!getRelocationValueString(*RelCur, Val))
+ *TargetOS << Val;
+ else
+ *TargetOS << TargetName;
+ if (Disp)
+ *TargetOS << "+0x" << Twine::utohexstr(Disp);
+ } else if (!Disp) {
*TargetOS << TargetName;
} else if (BBAddrMapLabelAvailable) {
*TargetOS << BBAddrMapLabels[Target].front();
@@ -2209,36 +2259,19 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
if (BTF)
printBTFRelocation(FOS, *BTF, {Index, Section.getIndex()}, LVP);
- // Hexagon does this in pretty printer
- if (Obj.getArch() != Triple::hexagon) {
- // Print relocation for instruction and data.
- while (RelCur != RelEnd) {
- uint64_t Offset = RelCur->getOffset() - RelAdjustment;
- // If this relocation is hidden, skip it.
- if (getHidden(*RelCur) || SectionAddr + Offset < StartAddress) {
- ++RelCur;
- continue;
- }
-
- // Stop when RelCur's offset is past the disassembled
- // instruction/data. Note that it's possible the disassembled data
- // is not the complete data: we might see the relocation printed in
- // the middle of the data, but this matches the binutils objdump
- // output.
- if (Offset >= Index + Size)
- break;
-
+ if (InlineRelocs) {
+ while (findRel()) {
// When --adjust-vma is used, update the address printed.
if (RelCur->getSymbol() != Obj.symbol_end()) {
Expected<section_iterator> SymSI =
RelCur->getSymbol()->getSection();
if (SymSI && *SymSI != Obj.section_end() &&
shouldAdjustVA(**SymSI))
- Offset += AdjustVMA;
+ RelOffset += AdjustVMA;
}
printRelocation(FOS, Obj.getFileName(), *RelCur,
- SectionAddr + Offset, Is64Bits);
+ SectionAddr + RelOffset, Is64Bits);
LVP.printAfterOtherLine(FOS, true);
++RelCur;
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
EsmeYi
reviewed
Dec 6, 2023
Comment on lines
2221
to
2224
| if (!getRelocationValueString(*RelCur, false, Val)) | ||
| *TargetOS << Val; | ||
| else | ||
| *TargetOS << TargetName; |
There was a problem hiding this comment.
It's better to use consumeError or reportWarning when an error is returned by getRelocationValueString().
Comment on lines
2027
to
2029
| // Hexagon handles relocs in pretty printer | ||
| if (Obj.getArch() == Triple::hexagon) | ||
| return false; |
There was a problem hiding this comment.
There is no need to put the check in findRel().
| if (Offset >= Index + Size) | ||
| break; | ||
|
|
||
| if (InlineRelocs) { |
There was a problem hiding this comment.
Leave the check Obj.getArch() != Triple::hexagon here.
|
@EsmeYi Did you have any other comments? |
|
The improved behavior in this patch looks reasonable. However, I get some concern on the fact that the system |
EsmeYi
reviewed
Dec 14, 2023
| @@ -0,0 +1,62 @@ | |||
| ; RUN: llc -mtriple=powerpc-ibm-aix-xcoff %s -filetype=obj -o %t | |||
| ; RUN: llvm-objdump %t -d --symbolize-operands --no-show-raw-insn \ | |||
There was a problem hiding this comment.
Added -r option. This shows that the "bl" instruction and the RLD display the same function.
|
There is no system |
diggerlin
reviewed
Dec 18, 2023
| @@ -456,7 +456,7 @@ declare i32 @bar(i32) | |||
| ; SYM-NEXT: ] | |||
|
|
|||
|
|
|||
| ; DIS: {{.*}}aix-xcoff-reloc.ll.tmp.o: file format aixcoff-rs6000 | |||
| ; DIS: : file format aixcoff-rs6000 | |||
There was a problem hiding this comment.
change content of the line checking on purpose?
There was a problem hiding this comment.
When the temporary object file name is encoded in the FileCheck patterns, you're prevented from running commands by hand during development. For example, I frequently create an object file first, then run
llvm-objdump ... | FileCheck ...
to see if the test passes.
There was a problem hiding this comment.
you can change to DIS: {{.*}} : file format aixcoff-rs6000.
| @@ -495,7 +495,7 @@ declare i32 @bar(i32) | |||
| ; DIS: 00000084 <globalB>: | |||
| ; DIS-NEXT: 84: 00 00 00 44 <unknown> | |||
|
|
|||
| ; DIS_REL: {{.*}}aix-xcoff-reloc.ll.tmp.o: file format aixcoff-rs6000 | |||
| ; DIS_REL: : file format aixcoff-rs6000 | |||
| @@ -1292,6 +1294,8 @@ collectLocalBranchTargets(ArrayRef<uint8_t> Bytes, MCInstrAnalysis *MIA, | |||
| // So far only supports PowerPC and X86. | |||
| if (!STI->getTargetTriple().isPPC() && !STI->getTargetTriple().isX86()) | |||
| return; | |||
| const bool isXCOFF = STI->getTargetTriple().isOSBinFormatXCOFF(); | |||
| const bool isPPC = STI->getTargetTriple().isPPC(); | |||
There was a problem hiding this comment.
suggest moving the line to before line 1328 (closest to the variable be used)
There was a problem hiding this comment.
I don't like to move const variables inside a loop, because I would need to analyze the initialization to make sure the compiler can initialize the variables once, rather than each time through the loop. I rearranged the code a bit.
|
LGTM, thanks. |
|
The current llvm-objdump code does not handle fixed-up relocations. I was going to add this in a separate PR, but it really needs to be include in this PR. A testcase requires a linked program, which is not supported yet by yaml, so a test will have to be added later. With the latest update, a branch to fixup code prints a comment for the target symbol. The comment is not printed if -r is used. |
diggerlin
approved these changes
Dec 19, 2023
| if (RelOffset >= Index + Size) | ||
| return false; | ||
| if (RelOffset >= Index) | ||
| return true; |
There was a problem hiding this comment.
suggest change these four lines
if (RelOffset >= Index + Size)
return false;
if (RelOffset >= Index)
return true;
to
if (RelOffset >= Index)
return RelOffset >= Index + Size ? false : true;
There was a problem hiding this comment.
I could make this change, but I think the code is easier to read as it is.
There was a problem hiding this comment.
it is suggestion , feel free to keep your code. thanks
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.
This presents misleading and confusing output. If you have a function defined at the beginning of an XCOFF object file, and you have a function call to an external function, the function call disassembles as a branch to the local function. That is,
void f() { f(); g();}disassembles as
With this PR, the second call will display:
10: 4b ff ff f1 bl 0x0 <.g>Using -r can help, but you still get the confusing output: