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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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)
----------------
jhenderson wrote:
> Does this warning come out with GNU? Same question with the other warnings elsewhere.
Yes, I think so:
(I rewrote it a bit, but idea of the message is the same).


```
umb at ubuntu:~/tests/9$ readelf --dynamic 1.o
readelf: Warning: the .dynamic section is not contained within the dynamic segment

Dynamic section at offset 0x1000 contains 2 entries:
  Tag        Type                         Name/Value
 0x0000000000000018 (BIND_NOW)           
 0x0000000000000000 (NULL)               0x0

umb at ubuntu:~/tests/9$ readelf -v
GNU readelf (GNU Binutils for Ubuntu) 2.31.1

```


================
Comment at: test/tools/llvm-readobj/elf-not-dynamic-in-pt-dynamic.test:11
+
+# WARNING:    warning: The SHT_DYNAMIC section is not at the start of PT_DYNAMIC segment
+# CHECK:      DynamicSection [ (2 entries)
----------------
GNU shows:
`readelf: Warning: the .dynamic section is not the first section in the dynamic segment.`

I think my version is better because:
1) I think the different start address does not mean there is a section there. It is a normal assumption, but still I have a fantazy to imagine there can be just a sequence of data before the .dynamic and no section in the section header table. Perhaps does not sound as a real case, but probably possible? Perhaps it is possible to create such test case with yaml2obj even.
2) I can imagine that SHT_DYNAMIC has name different from `.dynamic`. GNU code just hardoces the name. Doesn't seem as a good choice.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:206-207
 
-  void parseDynamicTable(ArrayRef<const Elf_Phdr *> LoadSegments);
+  void findDynamicTable(const ELFFile<ELFT> *Obj);
+  void parseDynamicTable();
 
----------------
jhenderson wrote:
> 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.
Hmmm. In the current design we store the tables and use them later, see:

```
  DynRegionInfo DynRelRegion;
  DynRegionInfo DynRelaRegion;
  DynRegionInfo DynRelrRegion;
  DynRegionInfo DynPLTRelRegion;
  DynRegionInfo DynSymRegion;
  DynRegionInfo DynamicTable;
```

When I wrote this place, I thought about the following scenario:
1) Rename the method to `findAndParseDynamicTable` or alike.
2) Call the `parseDynamicTable` from there.

(Honestly I did not try, but seems it should work).

What do you think?


================
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;
----------------
jhenderson wrote:
> 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?
Indeed. But I am following GNU behavior here. We have a test case which is failing without that lines: elf-dynamic-no-pt-dynamic.test

I.e. GNU seems to be a bit strange here, but I thought it is important to follow it for the consistency here.
Should we just check the section header and then a dynamic header (if not found) to lookup the .dynamic table?
(I am not sure so I would suggest to land this and do such change in a follow up so that we could revert it easily if that will not work for some reason).


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:377
+void reportWarning(Twine Msg) {
+  outs() << "\n";
+  WithColor::warning(outs()) << Msg << "\n";
----------------
jhenderson wrote:
> Do GNU readelf warnings definitely get printed on stdout?
Hmm. No. Looks like I tested that it does not change the return code (and does not exit after warning), i.e.
like in my code and somehow tested where do it prints the output wrong.

After retesting I see it prints warnings to stderr, indeed. Fixed, thanks!


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

https://reviews.llvm.org/D61937