[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-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)
----------------
grimar wrote:
> 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.
I agree with your comments here. It is possible to create such a test case with yaml2obj, with something like this:

```
Sections:
  - Name: .dynamic
    Type: SHT_DYNAMIC
    Address: 0x1000
    AddressAlign: 0x1000
    Entries:
      # ...
ProgramHeaders:
 - Type: PT_DYNAMIC
    Offset: 0x900
    Sections:
      - Section: .dynamic
```

Maybe it would be good for the type and name of the section to be printed (e.g. "warning: the SHT_DYNAMIC section '.dynamic' ...")


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:206-207
 
-  void parseDynamicTable(ArrayRef<const Elf_Phdr *> LoadSegments);
+  void findDynamicTable(const ELFFile<ELFT> *Obj);
+  void parseDynamicTable();
 
----------------
grimar wrote:
> 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?
Okay. How about `loadDynamicTable` instead of `findDynamicTable`? That could parse as well, but I'm okay if it doesn't.


================
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;
----------------
grimar wrote:
> 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).
I agree with doing it as a separate change, thanks. I think there is no logic in being able to dump a section not in a segment, but requiring the segment to be present. Of course, it is weird having a .dynamic section without a PT_DYNAMIC segment at all, but if we handle one case, we might as well handle the whole behaviour.


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

https://reviews.llvm.org/D61937