[PATCH] D61937: [llvm-readelf] - Rework how we parse the .dynamic section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test:2
+## Show that llvm-readobj/llvm-readelf tools can dump the .dynamic
+## section which is not in PT_DYNAMIC segment.
+
----------------
which is not in -> when it is not in a


================
Comment at: test/tools/llvm-readobj/elf-dynamic-not-in-pt-dynamic.test:8
+
+# CHECK:      warning: The SHT_DYNAMIC section is not contained within the PT_DYNAMIC segment
+# CHECK:      DynamicSection [ (2 entries)
----------------
Does this warning come out with GNU? Same question with the other warnings elsewhere.


================
Comment at: test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test:5
+## In the first case .text is placed before .dynamic and we check that
+## show a warning about that.
+
----------------
either ". We check that we print a warning for this case" or ". We check that we warn about this case".


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:206-207
 
-  void parseDynamicTable(ArrayRef<const Elf_Phdr *> LoadSegments);
+  void findDynamicTable(const ELFFile<ELFT> *Obj);
+  void parseDynamicTable();
 
----------------
I feel like a cleaner interface would be for findDynamicTable to return a start/end offset or even an ArrayRef of the contents, and pass that into parseDynamicTable.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1330-1332
+  // We do not want to dump dynamic section if we have no PT_DYNAMIC header.
+  if (!DynamicPhdr)
+    return;
----------------
This feels like a weird behaviour. If it can dump the .dynamic section when it's not in a segment, why does it need the segment to exist?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1343
 
+  // Information in the section header has a priority under the information
+  // in a PT_DYNAMIC header.
----------------
has a priority under -> has priority over


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:377
+void reportWarning(Twine Msg) {
+  outs() << "\n";
+  WithColor::warning(outs()) << Msg << "\n";
----------------
Do GNU readelf warnings definitely get printed on stdout?


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

https://reviews.llvm.org/D61937