[PATCH] D66403: Create MutableObjectWriter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Overall, the design looks pretty good now. I think it is clear to me what is happening. I do think you need to write significantly more tests though. Possibly the way to do it is to add a case to the existing tests for basic copies using the switch, and show that the output is identical. If it isn't, you need to understand why there are differences and whether they are important.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mutable-object-no-change.test:1
+## Test that the object file is properly coppied.
+
----------------
coppied -> copied

Copied under what circumstance? Perhaps "is properly copied when using the mutable object interface, and no switches are specified".


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


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mutable-object-no-change.test:16
+# RUN: llvm-readobj --sections %t | FileCheck %s
+
+
----------------
Delete the extra blank line.


================
Comment at: llvm/tools/llvm-objcopy/CommonOpts.td:87
+def use_mutable_object : Flag<["--"], "use-mutable-object">,
+                        HelpText<"Use MutableELFObject instead of ObjectFile">;
+
----------------
ObjectFile -> Object


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:818
+
+  /// TODO: handleArgs
+
----------------
`///` -> `//`

Maybe make this comment more explanatory too.


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


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2313
+  using SegmentRef = typename MutableELFObject<ELFT>::SegmentRef;
+  typename ELFT::Phdr EhdrSegment;
+  EhdrSegment.p_type = PT_PHDR;
----------------
Probably worth a `using` for the `typename ELFT::Phdr` type, maybe even in the class definition.


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


================
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;
----------------
Maybe a `stable_sort` would be a good plan here?


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


================
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(),
----------------
Why?

(I know it's the null section, but it's a good idea to be clear about it)


================
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;
----------------
Huh? Do you mean to be using sh_addralign here?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2476
+template class MutableObjectWriter<ELF32BE>;
+
 } // end namespace elf
----------------
Nit: delete the blank line


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

https://reviews.llvm.org/D66403