[PATCH] D105522: [llvm-readobj][XCOFF] Fix the error dumping for the first item of StringTable.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.cpp:58-59
   const uint8_t *StrContent = StringContent.bytes_begin();
-  const uint8_t *CurrentWord = StrContent;
+  // Set the start at StrContent + BeginByte for the cases where the first few
+  // bytes do not contain a string entry.
+  const uint8_t *CurrentWord = StrContent + BeginByte;
----------------
I'd change this comment to:

"Some formats contain additional metadata at the start which should not be interpreted as strings. Skip these bytes, but account for them in the string offsets."

Don't describe what the code is doing when the code is already clear. Instead, describe why. I think it's important to explain why you don't just use drop_front priort to calling this function: it's because you want to keep the offsets to represent the real offsets in the string table.


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.cpp:68
     }
     W.startLine() << format("[%6tx] ", CurrentWord - StrContent);
     printAsPrintable(W.startLine(), CurrentWord, WordSize);
----------------
Esme wrote:
> shchenz wrote:
> > The output is shown as hexadecimal data, but there is no "0x" prefix for the data, we should add the prefix or use decimal data?
> Changing the output format will affect many tests on dumping sections. That is out of the scope of this patch. We can post another patch for it if necessary.
Don't add `0x`. The output format MUST match that of GNU `readelf -p` for ELF output at least. If you really want to change XCOFF only, that's fine, but you'll need to do it for that format only.


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.h:113
 
-  void printAsStringList(StringRef StringContent);
+  void printAsStringList(StringRef StringContent, uint8_t BeginByte = 0);
 
----------------
qiucf wrote:
> Esme wrote:
> > shchenz wrote:
> > > Maybe a more meaningful name would be `StringSizeFieldLength`?
> > The function also called by ObjDumper::printSectionsAsString, which dumps the specified section(s), which doesn't have the field containing string content's length. So I think `StringSizeFieldLength` may be confused in that case?
> What about `BeginOffset`? And why it's typed as `uint8_t`, `size_t` seems better as offset/length if there's not other reasons.
More concretely: I'd change the signature to `void printAsStringList(StringRef StringContent, size_t StringDataOffset = 0);`


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:462
   DictScope DS(W, "StringTable");
   StringRef StrTable = Obj.getStringTable();
+  W.printNumber("Length (in bytes)", StrTable.size());
----------------
What will happen if you try to call this when there is no string table? XCOFF32 doesn't require one as I understand it.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:463
   StringRef StrTable = Obj.getStringTable();
-  printAsStringList(StrTable);
+  W.printNumber("Length (in bytes)", StrTable.size());
+  // Print strings from byte4, since the 0-3 bytes contain the length (in bytes)
----------------
I'm not opposed to the length field being here, but it should be a separate patch.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:464
+  W.printNumber("Length (in bytes)", StrTable.size());
+  // Print strings from byte4, since the 0-3 bytes contain the length (in bytes)
+  // of the string table.
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:465
+  // Print strings from byte4, since the 0-3 bytes contain the length (in bytes)
+  // of the string table.
+  printAsStringList(StrTable, 4);
----------------
I assume that the length includes the length field itself?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105522