[PATCH] D78005: [yaml2obj] - Reimplement how tool calculates file and memory sizes of segments.

[George Rimar via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:788-802
+      uint64_t MemSize = Fragments.front().Offset - PHeader.p_offset;
+      for (size_t I = 0, E = Fragments.size(); I != E; ++I) {
+        const Fragment &Cur = Fragments[I];
+        const Fragment *Next =
+            (I + 1 < Fragments.size()) ? &Fragments[I + 1] : nullptr;
+
+        // The distance between the current and the next section can
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > This logic seems awfully complicated for what it's doing. If we enforce an address order as well as offset order (see above), then it can just be the relative offset of the last section within the segment plus the section size, i.e. p_offset - sh_offset + sh_size.
> > > > 
> > > > By the way, p_memsz should only be set if the segment contains allocatable segments. If it doesn't, I think it should be 0. That might be slightly tangential to this patch though, so could be a different one.
> > > > 
> > > > If we enforce an address order as well 
> > > 
> > > We should not. The intention of yaml2obj is to produce any kinds of object including broken objects,
> > > I do not think we should enforce anything. Enforcing of file offsets was natural, we already have it at fact,
> > > it can be broken with `ShOffset` because we set it too early, but that is all what we should do.
> > How about lld's approach? (consistent with GNU ld in my observations)
> > 
> > ```lang=cpp
> >       p->p_filesz = last->offset - first->offset;
> >       if (last->type != SHT_NOBITS)
> >         p->p_filesz += last->size;
> > 
> >       p->p_memsz = last->addr + last->size - first->addr;
> > ```
> > 
> > When there are more than one SHT_NOBITS, the trailing SHT_NOBITS sections have the same sh_size. The algorithm will not overly increase p_filesz.
> >p->p_filesz = last->offset - first->offset;
> >if (last->type != SHT_NOBITS)
> >  p->p_filesz += last->size;
> 
> This is the same what this patch does for p_filesz.
> 
> > When there are more than one SHT_NOBITS, the trailing SHT_NOBITS sections have the same sh_size. 
> 
> Why? In my sample below used in the test case it is not true and yaml2obj should be able to handle any layout given.
> 
> ```
> ## We have two SHT_NOBITS section at the end of the segment.
> ## It is important to have at least two of them because we had a bug
> ## related to an incorrect computation of the memory size of a segment when
> ## there were multiple SHT_NOBITS sections at the end.
>   - Name:    .nobits3
>     Type:    SHT_NOBITS
>     Flags:   [ SHF_WRITE, SHF_ALLOC ]
>     Size:    0x1D0
>   - Name:    .nobits4
>     Type:    SHT_NOBITS
>     Flags:   [ SHF_WRITE, SHF_ALLOC ]
>     Size:    0x100
> ```
> 
> 
> p->p_memsz = last->addr + last->size - first->addr;

Also this can't be used because depends on addressed that must be sorted, this is not our case here.


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

https://reviews.llvm.org/D78005