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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:65
+  default:
+    report_fatal_error("Unhandled relocation type.");
+  }
----------------
jhenderson wrote:
> 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.
Sounds good to me.


================
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:
> > 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?).
>  I think the report_fatal_error is appropriate here. In here, we do imply there is an internal problem in llvm for XCOFF object file, as our tooling do not support parsing 64 bit object yet (not that the input object file is malformed input).
> 
> 
I had a look at what other object files do. In ELF, an error is simply thrown away and a null iterator is constructed. In COFF, a report_fatal_error is reached. Mach-O and wasm both don't have any errors.

I'm not happy about the current state, but I suppose there's prior art and it needs a bigger cleanup anyway. Plus, once 64-bit is implemented, this goes away.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:362
+  }
+  llvm_unreachable("Cannot find the containing section for this relocation.");
 }
----------------
jasonliu wrote:
> jhenderson wrote:
> > 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`.
> As mentioned below, I find it a bit challenge to change the existing interface to return llvm::Expected. Although this one might not be as hard as getRelocationSymbol, it is still a non-trivial task after some investigation (i.e. there are python script actually depending on existing interface if I'm not mistaken). So I'm thinking to proceed with some less resistant way.
> 
> Other target for this function will return the virtual address stored in the relocation entry without checking and calculation. 
> Presumably, the caller of this function would have to check if the range is valid, or they would just happy to accept any invalid value and print it.
> So my plan is, for XCOFF target, we could return a magic number (std::numeric_limits<uint64_t>::max()) so that if anyone bother to check the range, they know it's invalid, and if they don't want to check it, they could still proceed gracefully just like other target.
I had a play around with this. I think a change in the interface is viable, and not all that invasive, with an issue in the llvm-c level being the only one that stopped me trying to push the change further for now.

UINT64_MAX is probably a better value than zero, I suppose. I'm not massively happy with it, since technically, there's nothing stopping a single-byte-patching relocation patching the very last value in a massive ELF. However, in practice that's never going to happen.

Could you put the value as a static constant in the XCOFF object file, and then refer to it elsewhere? That would at least ensure the meaning of the value is clear. You'd want to mention that it returns the constant in the function's doxygen comment.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:324
+  if (is64Bit())
+    report_fatal_error("64-bit support not implemented yet.");
+  const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
----------------
Here and all report_fatal_error calls: remove the trailing full stop.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:325
+    report_fatal_error("64-bit support not implemented yet.");
+  const XCOFFSectionHeader32* SectionEntPtr = toSection32(Sec);
+  auto RelocationsOrErr = relocations(*SectionEntPtr);
----------------
hubert.reinterpretcast wrote:
> Please replace `Typename*` binding with `Declarator *binding`.
Isn't this handled by clang-format?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:367-368
+
+  // Have an invalid address, return max value so that caller
+  // of this function could either detect it or proceed with it.
+  return std::numeric_limits<uint64_t>::max();
----------------
If you pull the value into a named constant, you won't need this comment.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:24-25
+  if (SymI == Obj->symbol_end())
+    return make_error<GenericBinaryError>("Could not get relocation symbol.",
+                                          object_error::parse_failed);
+
----------------
Errors usually have lower case first letter and no trailing full stop.


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

https://reviews.llvm.org/D75131