[PATCH] D67547: [llvm-readobj/llvm-objdump] - Improve how tool locate the dynamic table and report warnings about that.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1500
                   DynamicSec->sh_size, sizeof(Elf_Dyn), ObjF->getFileName()});
-    parseDynamicTable();
+    IsSecTableValid = !FromSec.getAsArrayRef<Elf_Dyn>().empty();
   }
----------------
MaskRay wrote:
> grimar wrote:
> > MaskRay wrote:
> > > The logic is:
> > > 
> > > ```
> > > findDynamic
> > >   find PT_DYNAMIC
> > >   find .dynamic
> > >   if p_offset is out of bounds
> > >     reportWarning
> > >   if both PT_DYNAMIC and .dynamic exist
> > >     check if the information matches
> > > 
> > > loadDynamicTable
> > >   findDynamic()
> > >   if PT_DYNAMIC exists
> > >     IsPhdrTableValid = PT_DYNAMIC's DynRegionInfo is not empty
> > >   if .dynamic exists
> > >     IsSecTableValid = .dynamic's DynRegionInfo is not empty
> > >   if either does not exist
> > >     if (IsPhdrTableValid && IsSecTableValid)
> > >       parseDynamicTable()
> > >     else
> > >       warn
> > >     return
> > > 
> > >   validate information matches
> > >   ...
> > > ```
> > > 
> > > Shall we check whether p_offset is out of bounds and whether DynRegionInfo of PT_DYNAMIC is empty together?
> > > 
> > > ```
> > > findDynamic
> > >   find PT_DYNAMIC
> > >   IsPhdrTableValid = whether PT_DYNAMIC exists
> > >   if p_offset is out of bounds or DynRegionInfo not empty
> > >     reportWarning
> > >     IsPhdrTableValid = false
> > > 
> > >   find .dynamic
> > >   validate .dynamic
> > > 
> > >   if either does not exist
> > >     if ((DynamicPhdr && IsPhdrTableValid) || (DynamicSec && IsSecTableValid))
> > >       parseDynamicTable()
> > >     else
> > >       warn
> > >     return
> > > 
> > >   ...
> > >   validate information matches
> > >   parseDynamicTable()
> > > ```
> > I am not sure I understand what you suggest, sorry, Do you want to merge 2 existing methods into one large one?
> > (I do not understand why your `findDynamic` might call `arseDynamicTable()` for example).
> > Do you want to merge 2 existing methods into one large one?
> 
> Yes. Locate PT_DYNAMIC, and do all PT_DYNAMIC specific checks. Then, locate .dynamic, and do all .dynamic related checks. Last, check whether PT_DYNAMIC and .dynamic mismatch.
Ok. Now things became clear :) I spent some time experimenting with this code while worked on a patch and one of things I tried to achieve was to reduce the amount of the code in each method. I.e. I did splitting intentionally (and was very happy about that), because it looks simpler to me generally to work with a smaller pieces of code. 
Even now, looking at `loadDynamicTable` it seems a bit too large to me.

Are you strong it this suggestion? I.e. I am not sure it will look better, but I do not mind to try to do it again if you really insist.


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

https://reviews.llvm.org/D67547