[PATCH] D78709: [llvm-readobj] - Simplify conditions used for printing segment mappings. NFCI.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4078
 template <class ELFT>
-bool GNUStyle<ELFT>::checkPTDynamic(const Elf_Phdr &Phdr, const Elf_Shdr &Sec) {
-  if (Phdr.p_type != ELF::PT_DYNAMIC || Sec.sh_size != 0 || Phdr.p_memsz == 0)
+static bool checkPTDynamic(const typename ELFT::Phdr &Phdr,
+                           const typename ELFT::Shdr &Sec) {
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > Similar to the TLS one, this function seems bonkers. Why does it exist at all? Why is the dynamic segment special compared to other segments?
> > > I believe it is based on the GNU readelf logic:
> > > https://github.com/bminor/binutils-gdb/blob/master/include/elf/internal.h#L366
> > > 
> > > It says "No zero size sections at start or end of PT_DYNAMIC nor PT_NOTE.".
> > > Here only the `PT_DYNAMIC` is handled, seems we want to handle `PT_NOTE` too.
> > > 
> > > 
> > Not dug into it, but I guess the intent here is to avoid having non SHT_DYNAMIC/SHT_NOTE sections appear in PT_DYNAMIC/PT_NOTE segments, which does make some sense. Perhaps worth writing/expanding a comment to explain this? What do you think?
> > but I guess the intent here is to avoid having non SHT_DYNAMIC/SHT_NOTE sections appear in PT_DYNAMIC/PT_NOTE segment. Perhaps worth writing/expanding a comment to explain this? What do you think?
> 
> The code does not check the section type, i.e. if it is "SHT_DYNAMIC" or not. We might have (in a mailformed object) 2 SHT_DYNAMIC sections:
> first is empty, second is not and will never see the first one in the output with the current code.
> 
> I guess the best comment could be: "This is just how GNU does, we follow". We can return to it later when will add support for "PT_NOTE" here.
> 
I actually am coming around to GNU's process for this case here. Yes, it doesn't check the section type, but the chances of an empty dynamic section are minimal anyway. Maybe we could special case SHT_DYNAMIC though at a future point. If we did that, I'd want the SHT_DYNAMIC exception for PT_DYNAMIC only and SHT_NOTE for PT_NOTE in a similar manner.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4085
+  // Is section within the phdr both based on offset and VMA?
+  bool CheckOffset = (Sec.sh_type == ELF::SHT_NOBITS) &&
+                     (Sec.sh_offset > Phdr.p_offset &&
----------------
Shouldn't this be an `||` to match the old behaviour?


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

https://reviews.llvm.org/D78709