[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-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)
----------------
jhenderson wrote:
> 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' ...")
I updated the warnings to include the dynamic section name.


================
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:
> 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.
Done. (I also moved `parseDynamicTable`. Not sure it is much better, but isolating it in one place is also perhaps can be reasonable)


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

https://reviews.llvm.org/D61937