[XCOFF/objdump] Fix crash in objdump when trying to use -r with XCOFF binaries

[davidb]

Summary

Add a warning and disable -r if specified by the user for now.

[davidb]

• fix space issues

[MaskRay]

I think XCOFF is a WIP, so many things can break. It may be fine not adding a warning.

[davidb]

In D74823#1883269, @MaskRay wrote:

I think XCOFF is a WIP, so many things can break. It may be fine not adding a warning.

Is it ok to silently disable Relocations to prevent crashing at least? This becomes an issue in D74824.

[jhenderson]

It's good that we don't crash, but I'm wondering why not just implement relocation printing for XCOFF? Also, is the intention to not print relocations mixed with disassembly only (i.e. -r + -d/D etc), or also relocation printing without disassembly (i.e. -r on its own)? If the latter, there needs to be a test for that too.

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

19

You could avoid the need for this -NOT check, by using CHECK-EMPTY/CHECK-NEXT lines to match between the previous CHECK and the CHECK: disassembly... lines. That way, you don't need to have two identical strings here.

llvm/tools/llvm-objdump/llvm-objdump.cpp

2198

Nit: missing trailing full stop.

[jasonliu]

I want to understand how this change would help you avoid crashes in D74824.
I think the crash is due to removal of "if (InlineRelocs)" in

if (InlineRelocs)
  RelocMap = getRelocsMap(*Obj);

After this change, you would still call getRelocsMap() no matter Relocations set to false or not, which is still resulting in crash.

[jasonliu]

FYI, I do have a downstream patch that would implement '-r' for XCOFF format. I could post that asap if that helps. Please let me know.

[davidb]

In D74823#1889317, @jhenderson wrote:

It's good that we don't crash, but I'm wondering why not just implement relocation printing for XCOFF? Also, is the intention to not print relocations mixed with disassembly only (i.e. -r + -d/D etc), or also relocation printing without disassembly (i.e. -r on its own)? If the latter, there needs to be a test for that too.

I've essentially did the bare minimum required to continue with D74824 where I discovered the failure with -r specified. I suspect that adding relocation printing support would require a lot more effort than it would take for introducing this warning (Which I currently cannot commit to, but was intending to come back to).

In D74823#1889422, @jasonliu wrote:

I want to understand how this change would help you avoid crashes in D74824.
I think the crash is due to removal of "if (InlineRelocs)" in

if (InlineRelocs)
  RelocMap = getRelocsMap(*Obj);

After this change, you would still call getRelocsMap() no matter Relocations set to false or not, which is still resulting in crash.

Hi Jason, yes, specifying -r results in a crash. The way that I currently have implemented D74824 means that getRelocsMap()is always called, which would result in a crash if objdump was to call it elsewhere.

In D74823#1889461, @jasonliu wrote:

FYI, I do have a downstream patch that would implement '-r' for XCOFF format. I could post that asap if that helps. Please let me know.

That would be great to get a ready implementation upstream. No rush where D74824 is concerned, I think that requires more work/discussion.

[jasonliu]

That would be great to get a ready implementation upstream. No rush where D74824 is concerned, I think that requires more work/discussion.

I created a patch at D75131.

[davidb]

no longer needed after D75131

[MaskRay]

In D74823#1970969, @davidb wrote:

no longer needed after D75131

You can click "Add Action..." -> "Abandon.." if you no longer need a change. This is also nice for reviewers because the change will disappear from their review queues.

llvm/include/llvm/Object/XCOFFObjectFile.h

public:
   void checkSymbolEntryPointer(uintptr_t SymbolEntPtr) const;
 
   // Relocation-related interfaces.
   Expected<uint32_t>
   getLogicalNumberOfRelocationEntries(const XCOFFSectionHeader32 &Sec) const;
 
   Expected<ArrayRef<XCOFFRelocation32>>
   relocations(const XCOFFSectionHeader32 &) const;
+  static bool classof(const Binary *v) { return v->isXCOFF(); }
 }; // XCOFFObjectFile
 
 class XCOFFSymbolRef {
   const DataRefImpl SymEntDataRef;
   const XCOFFObjectFile *const OwningObjectPtr;
 
 public:
   XCOFFSymbolRef(DataRefImpl SymEntDataRef,

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

-# RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
-# RUN: FileCheck %s
+# RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o 2>&1 | \
+# RUN: FileCheck %s --check-prefixes=CHECK,NO-RELOC-OPTION
+# RUN: llvm-objdump -D -r %p/Inputs/xcoff-section-headers.o 2>&1 | \
+# RUN: FileCheck %s --check-prefixes=CHECK,RELOC-OPTION
 
 # xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0
 # compiler command: xlc -qtls -o xcoff-section-headers.o -c test.c
 # test.c:
 # int a;
 # int b = 12345;
 # __thread int c;
 # __thread double d = 3.14159;
 #
 # int func(void)  {
 #   return a;
 # }
 ; REQUIRES: powerpc-registered-target
 CHECK: Inputs/xcoff-section-headers.o:  file format aixcoff-rs6000
+NO-RELOC-OPTION-NOT: -r/--relocs option not supported for XCOFF object files, ignoring option
+RELOC-OPTION: -r/--relocs option not supported for XCOFF object files, ignoring option
 CHECK: Disassembly of section .text:
 CHECK: 00000000 .text:
 CHECK-NEXT:        0: 80 62 00 04                   lwz 3, 4(2)
 CHECK-NEXT:        4: 80 63 00 00                   lwz 3, 0(3)
 CHECK-NEXT:        8: 4e 80 00 20                   blr
 CHECK-NEXT:        c: 00 00 00 00                   <unknown>
 CHECK-NEXT:       10: 00 00 20 40                   <unknown>
 CHECK-NEXT:       14: 00 00 00 01                   <unknown>

llvm/tools/llvm-objdump/llvm-objdump.cpp

 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/COFFImportFile.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
+#include "llvm/Object/XCOFFObjectFile.h"
 #include "llvm/Object/Wasm.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
if (SymbolTable)
     printSymbolTable(O, ArchiveName);
   if (DwarfDumpType != DIDT_Null) {
     std::unique_ptr<DIContext> DICtx = DWARFContext::create(*O);
     // Dump the complete DWARF structure.
     DIDumpOptions DumpOpts;
     DumpOpts.DumpType = DwarfDumpType;
     DICtx->dump(outs(), DumpOpts);
   }
+
+  // Relocation processing is not yet implemented for XCOFF binaries
+  if(Relocations && isa<XCOFFObjectFile>(O)) {
+    reportWarning("-r/--relocs option not supported for XCOFF object files, "
+                  "ignoring option", O->getFileName());
+    Relocations = false;
+  }
+ 
   if (Relocations && !Disassemble)
     printRelocations(O);
   if (DynamicRelocations)
     printDynamicRelocations(O);
   if (SectionContents)
     printSectionContents(O);
   if (Disassemble)
     disassembleObject(O, Relocations);