[PATCH] D66403: Create MutableObjectWriter class

Thu Aug 29 02:04:18 PDT 2019

jhenderson added a comment.

In D66403#1650286 <https://reviews.llvm.org/D66403#1650286>, @abrachet wrote:

> I didn't get to all comments yet, and I also haven't added more tests because currently the object file isn't being create properly. Once I am making a valid ELF file I will do more rigorous testing.
>
> Right now `llvm-readelf` complains that that the .shstrtab is not null terminated, so probably a layout issue of sections. I've been looking into it and couldn't find the problem yet. My section offsets are slightly lower than how `llvm-objcopy` currently does layout. Does anyone have any ideas why that is?

No idea myself off the top of my head, but the best thing to do is probably just to manually look at the produced object in a hex viewer, and see what looks wrong (consider section sizes, the section offset, and whether the right section is marked as the section header string table).

> Currently this patch creates sections at offsets 0, 38, 38, 50, 51 and llvm-objcopy does 0, 40, 40, 58, 59 (both with no arguments)

I'd try manually laying out the ELF by hand, and see what your calculations come to, then compare them to what happens when you run the tool with a debugger. Where do the offsets start differing, and what causes that? Obvious suggestion might be problems with alignment, but I don't know.

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:827
+Error executeObjcopyWithMutableObject(const CopyConfig &Config,
+                                      object::ELFObjectFileBase &In,
+                                      Buffer &Out) {
----------------
abrachet wrote:
> jhenderson wrote:
> > I'm hesitant about this not taking an r-value reference or copy, because you std::move it further down the stack. This feels like it might be at risk of surprising a user of the interface later on.
> There is no copy constructor for `ObjectFile`s so I could make one for `MutableELFObject`, I can't move `In` or `*O` the compiler says. I don't really know the intricacies of when you can and cannot move an object, probably I could move the `OwningObjectFile` though, this would require larger changes to this method. Do you have any thoughts?
I don't really think we want a copy, thinking about it more. We probably do want an r-value reference though. I'd change the signatures and call sites as follows, assuming it works (not had the time to verify them myself):

```
return elf::executeObjcopyWithMutableObject(Config, std::move(*ELFBinary), Out)
```

```
Error executeObjcopyWithMutableObject(const CopyConfig &Config,
                                      object::ELFObjectFileBase &&In,
                                      Buffer &Out) {
  ...
  return handleArgsAndWrite(std::move(*O), Out)
}
```

```
Error handleArgsAndWrite(ELFObjectFile<ELFT> &&Obj, Buffer &Out)
```

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2354
+      std::advance(ParentSegment, *ParentIndex);
+      MutPhdrOrErr->Header.p_offset =
+          ParentSegment->getHeader().p_offset +
----------------
abrachet wrote:
> jhenderson wrote:
> > What if the parent segment hasn't been laid out yet? I believe that you could theoretically have the parent segment after the child segment, in the ordered vector, if the alignment of the parent is greater than an otherwise identical child.
> I haven't gotten around to this yet because I have bigger problems that I want to address before potentially adding more bugs.
> 
> I can do this the quick way which would be having an `std::set<size_t>` and whenever `*ParentIndex` is larger than `Index` I layout that segment and then add it to the set and check every iteration of the loop if `Index` is in there. Seems ugly though. I could also a `bool laidOut` to `MutableELFSegment<ELFT>` because its ok to make every segment mutable because there are so few of them I believe.
I don't think you need to add a member to MutableELFSegment. You could have a local vector/map of bool though, so you can do `IsLaidOut[Index]` and just check and update that value at appropriate times.

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2369
+Error MutableObjectWriter<ELFT>::layoutSections(uint64_t &Offset) {
+  // Skip first section.
+  iterator_range<section_iterator> Sections(++Obj.section_begin(),
----------------
jhenderson wrote:
> Why?
> 
> (I know it's the null section, but it's a good idea to be clear about it)
Thinking about it from a strict correctness point of view, I think you just want to skip all sections with type SHT_NULL. From the gABI:

> SHT_NULL - This value marks the section header as inactive; it does not have an associated section. Other members of the section header have undefined values. 

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2476
+template class MutableObjectWriter<ELF32BE>;
+
 } // end namespace elf
----------------
abrachet wrote:
> jhenderson wrote:
> > Nit: delete the blank line
> Are you sure? There was previously a blank line between `template class ELFWriter<ELF32BE>;` and the end of namesapce elf. I think it looks better too.
My mistake. I misread the old diff. Please re-add!

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2327-2332
+  Obj.addSegment({EhdrSegment,
+                  ArrayRef<uint8_t>(Obj.base(), sizeof(Ehdr)),
+                  None});
+  Obj.addSegment({PhdrSegment,
+                  ArrayRef<uint8_t>(Obj.base() + Obj.getHeader().e_phoff,
+                                    Obj.getHeader().e_phnum * sizeof(Phdr)),
----------------
Adding the segments like this is a good idea taken from llvm-objcopy's existing handling (even if I do say so myself, since I remember suggesting this originally!), but where do you prevent them actually being written out?

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2385
+      Offset = alignTo(
+          Offset, !Section.Header.sh_addralign ? 1 : Section.Header.sh_addralign);
+      Section.Header.sh_offset = Offset;
----------------
clang-format?

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2458
+              sizeof(Ehdr));
+
+  return Buf.commit();
----------------
I might be being blind, but where do you write the program header table?

================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:360-363
+  using Ehdr = typename ELFT::Ehdr;
+  using Phdr = typename ELFT::Phdr;
+  using Shdr = typename ELFT::Shdr;
+  using Addr = typename ELFT::Addr;
----------------
Canonical names are `Elf_Ehdr`, `Elf_Phdr` etc.

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

https://reviews.llvm.org/D66403