[PATCH] D75131: [XCOFF][AIX] Enable -r option for llvm-objdump

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:65
+  default:
+    report_fatal_error("Unhandled relocation type.");
+  }
----------------
davidb wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > This seems fishy. If I was to create a XCOFF object file with a relocation with an unknown type value, would I get this error when e.g. dumping relocations?
> > > 
> > > My feeling is that anything that uses `report_fatal_error` is a bug waiting to happen. Better would be to return some string indicating an unknown relocation type.
> > for enum type , if your switch case  has enumerate all the case,
> > please do not use the default, otherwise it will cause a compiler error with clang 
> ELF returns the string "Unknown". Will make it less painful in case this code falls out of sync with new relocs.
> for enum type , if your switch case has enumerate all the case,
please do not use the default, otherwise it will cause a compiler error with clang

Does this apply for all enum variations? It's trivially easy to cast something such that it isn't a recognised value.

Regardless, in this context, you can easily return outside the switch.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:322
 relocation_iterator XCOFFObjectFile::section_rel_begin(DataRefImpl Sec) const {
-  llvm_unreachable("Not yet implemented!");
-  return relocation_iterator(RelocationRef());
+  assert(!is64Bit() && "64-bit support not implemented yet.");
+  const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
----------------
jasonliu wrote:
> jhenderson wrote:
> > Does other code further up the stack check to make sure the file is not 64 bits? If not, this isn't a valid assertion, since the program could reach this point. What would happen then if assertions are disabled?
> > 
> > Same comment goes for all the other instances of this.
> I agree. I will make them all report_fatal_error instead of assertion. 
I consider `report_fatal_error` to be as bad as an assertion for code that can get to here, as the error is useless and implies an internal problem in llvm, which isn't true if it's only hit due to malformed input. If at all possible, I'd recommend using `llvm::Expected` to report errors, if it's possible within the interface (what do other ObjectFile variations do?).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:362
+  }
+  llvm_unreachable("Cannot find the containing section for this relocation.");
 }
----------------
jasonliu wrote:
> davidb wrote:
> > jhenderson wrote:
> > > Is this really unreachable? Could you get to here with an object with some invalid relocation or section property?
> > Agree. I think a relocation with an invalid r_vaddr could fall through this.
> I see. I could make them report_fatal_error instead. 
See my earlier comment. Please use `llvm::Expected` if possible, not `report_fatal_error`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75131/new/

https://reviews.llvm.org/D75131