[PATCH] D136787: [XCOFF] Decode the relocation entries of loader section of xcoff for llvm-readobj

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:162
+  virtual void printLoaderSection(bool PrintHeader, bool PrintSymbolTable,
+                                  bool PrintRelocation) {}
 
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > This field should be `PrintRelocations` because there can be more than one relocation.
> > I think whether  to use "PrintSymbolTables" instead of "PrintSymbolTable" when I implement the decoding the symbol table of loader section. I think PrintSymbolTable can explain the we want to print symbol table out. (and we only one symbol table in the loader section, we have multi symbol table entries in the symbol table. )  the same reason for the PrintRelocation. if the parameter name is "PrintRelocationEntry", we should be change to "PrintRelocationEntries"
> Your analogy is flawed: "Relocation" and "SymbolTable" are not referring to the same kind of thing.
> 
> There is exactly one symbol table, hence why it is singular. The relocation entries consist of one or more relocations: "relocation entry" is just a verbose way of saying "relocation". The term for a group of relocations could be "relocation table" or "relocation section" (see ELF for good examples of this throughout). In other words, "relocation" is the same sort of thing as a "symbol". You could change the `PrintSymbolTable` parameter to `PrintSymbols`, but not to `PrintSymbol` for example.
thanks


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:338
+      if (!SymbolNameOrErr) {
+        reportUniqueWarning(SymbolNameOrErr.takeError());
+        return;
----------------
jhenderson wrote:
> Test case for this warning?
I am not sure whether we need to test Symbol Name error in all different patch, we have the same test for the code 
 in https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-readobj/XCOFFDumper.cpp#L234


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136787