[PATCH] D68575: [llvm-readobj][xcoff] implement parsing overflow section header.

[Digger via Phabricator via llvm-commits]

DiggerLin marked 7 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-overflow-section.test:37
+# SECOVERFLOW-NEXT:     Name: .ovrflo
+# SECOVERFLOW-NEXT:     NumberOfRelocations: 0x3
+# SECOVERFLOW-NEXT:     NumberOfLineNumbers: 0x0
----------------
hubert.reinterpretcast wrote:
> The same information, when conveyed not via an overflow header, is expressed in decimal format (and not hexadecimal).
changed as suggestion.


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-overflow-section.test:38
+# SECOVERFLOW-NEXT:     NumberOfRelocations: 0x3
+# SECOVERFLOW-NEXT:     NumberOfLineNumbers: 0x0
+# SECOVERFLOW-NEXT:     Size: 0x0
----------------
hubert.reinterpretcast wrote:
> Ditto.
changed as  suggestion.


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-overflow-section.test:41
+# SECOVERFLOW-NEXT:     RawDataOffset: 0x0
+# SECOVERFLOW-NEXT:     RelocationPointer: 0x0
+# SECOVERFLOW-NEXT:     LineNumberPointer: 0x0
----------------
hubert.reinterpretcast wrote:
> According to the XCOFF documentation: The `s_relptr` and `s_lnnoptr` fields must have the same values as in the corresponding primary section header.
changed raw data and test case 


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:441
+      if (Obj.is64Bit())
+        W.printString(
+            "64 bits XCOFF objectfile should not have overflow section!");
----------------
hubert.reinterpretcast wrote:
> I think we should not be printing a "warning" to the same stream as the output.
changed


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:448
+    case XCOFF::STYP_TYPCHK:
+      // TODO : The interpret of loader, exception, type check section header
+      // are different from generic section header. We will implement them
----------------
hubert.reinterpretcast wrote:
> s/TODO : /TODO /;
> s/interpret/interpretation/;
> s/, type check section header/, and type check section headers/;
> s/from/from that of/;
> s/generic section header/generic section headers/;
> s/generic seciton header now/generic section headers for now/;
changed as  suggestion.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:456
     // The most significant 16-bits represent the DWARF section subtype. For
     // now we just dump the section type flags.
     if (Flags & SectionFlagsReservedMask)
----------------
hubert.reinterpretcast wrote:
> Since we removed the context of the code line from here, the comment is ambiguous. This should say that, for now, we dump only the section type (low order 16 bits).
changed as suggestions.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68575