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

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 16 03:23:27 PDT 2020


grimar marked 3 inline comments as done.
grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:776-779
+      uint64_t FileSize = Fragments.back().Offset - PHeader.p_offset;
+      if (Fragments.back().Type != llvm::ELF::SHT_NOBITS)
+        FileSize += Fragments.back().Size;
+      PHeader.p_filesz = FileSize;
----------------
jhenderson wrote:
> This doesn't look right to me. What if there are multiple SHT_NOBITS sections at the end of a segment?
I have this test case below, it works fine, why it shouldn't?

```
## 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
```


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


================
Comment at: llvm/test/Object/invalid.test:488
   Machine: EM_X86_64
-Sections:
-  - Name: .dynamic
----------------
jhenderson wrote:
> Why has this test changed?
Because my code uses the `Offset` which is misused here. We use it here like it is `ShOffset`, i.e. to override the value,
but it is not what `Offset` field should do. I think it should be used to set value that is <= the offset of the first section in the segment.

We might probably want to add `ShOffset`.


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

https://reviews.llvm.org/D78005





More information about the llvm-commits mailing list