[PATCH] D66403: Create MutableObjectWriter class

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 13 inline comments as done.
abrachet added a comment.

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?

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)



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mutable-object-no-change.test:3-12
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
----------------
jhenderson wrote:
> It's more normal to put the YAML at the end of the test.
> 
> I think your test case needs to test program headers and symbols too, so this needs some of each. Perhaps a separate test case would be appropriate to test segment layout for nested segments.
Oops forgot to do the first part, I'll do that in a later diff. Haven't done second part see my out of line comment.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:827
+Error executeObjcopyWithMutableObject(const CopyConfig &Config,
+                                      object::ELFObjectFileBase &In,
+                                      Buffer &Out) {
----------------
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?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2332
+                  ArrayRef<uint8_t>(Obj.base() + Obj.getHeader().e_phoff,
+                                    Obj.getHeader().e_phnum),
+                  None});
----------------
jhenderson wrote:
> Shouldn't this be `Obj.getHeader().e_phnum * sizeof(Phdr)`?
Good catch :)


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2337
+  PhdrSegment.p_memsz = sizeof(typename ELFT::Phdr) * Segments.size();
+  llvm::sort(Segments, [](auto First, auto Second) {
+    return First.getHeader().p_offset < Second.getHeader().p_offset;
----------------
jhenderson wrote:
> Maybe a `stable_sort` would be a good plan here?
I actually just learned about this today in class and was thinking it was just what I needed :)


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2354
+      std::advance(ParentSegment, *ParentIndex);
+      MutPhdrOrErr->Header.p_offset =
+          ParentSegment->getHeader().p_offset +
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2385
+      Offset = alignTo(
+          Offset, !Section.Header.sh_offset ? 1 : Section.Header.sh_offset);
+      Section.Header.sh_offset = Offset;
----------------
jhenderson wrote:
> Huh? Do you mean to be using sh_addralign here?
Oh yes thanks!


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2476
+template class MutableObjectWriter<ELF32BE>;
+
 } // end namespace elf
----------------
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.


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

https://reviews.llvm.org/D66403