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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay 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();
   }
----------------
grimar wrote:
> 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.
I am not strong about the suggestion. I think the functions are still long after the split (the current patch). I was trying to figure out why we added so many lines for the extra warnings.

> p_offset is out of bounds or DynRegionInfo not empty

I was hoping regrouping the diagnostics can simplify the code, and then there may be other ways to split the function, but maybe I am wrong.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1550
+  parseDynamicTable();
+  return;
 }
----------------
Delete `return;`


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

https://reviews.llvm.org/D67547