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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:756
     std::vector<Fragment> Fragments = getPhdrFragments(YamlPhdr, SHeaders);
-
-    if (YamlPhdr.Offset) {
+    if (!std::is_sorted(Fragments.begin(), Fragments.end(),
+                        [](const Fragment &A, const Fragment &B) {
----------------
`llvm::is_sorted`


================
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:
> 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.


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

https://reviews.llvm.org/D78005