[PATCH] D66403: Create MutableObjectWriter class

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've made a number of first-pass comments for you.



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1098-1100
+  Expected<MutableELFSection<ELFT> &> ShstrtabOrErr =
+      MutableObject.getMutableSection(SectionRef(
+          toDataRef(MutableObject.getHeader().e_shstrndx), &MutableObject));
----------------
Don't need to add this now, but some TODOs to add are:

1) What to do if the string table index is a removed section.
2) What to do if the e_shstrndx value is either SHN_XINDEX.
3) What to do if the e_shstrndx value is 0.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1104
+  Shstrtab = &ShstrtabOrErr.get();
+  assert(Shstrtab->Header.sh_type == ELF::SHT_STRTAB);
+
----------------
This shouldn't be an assert. What if the e_shstrndx value points at a non-string table?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1140
+  Shstrtab->Data = OwningArrayRef<uint8_t>(ShstrBuilder.getSize());
+  ShstrBuilder.write(Shstrtab->Data.data());
+
----------------
This seems like an unnecessary copy here. Why can't it wait until you actually need to write the .shstrtab to the buffer?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1142-1151
+  for (BasicSymbolRef BSR : MutableObject.symbols()) {
+    ELFSymbolRef Sym(BSR);
+    Expected<StringRef> NameOrErr = Sym.getName();
+    if (!NameOrErr)
+      return NameOrErr.takeError();
+    SymStrBuilder.add(*NameOrErr);
+  }
----------------
This block of code is very similar to the above block. Consider sharing it.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1167
+  Sec.Header.sh_offset = CurrentOffset;
+  Sec.Header.sh_name = 0; // ShstrBuilder.getOffset(StringRef(Sec.Name));
+  if (Sec.Header.sh_type != ELF::SHT_NOBITS)
----------------
abrachet wrote:
> I'm not sure if I'm using `StringTableBuilder` incorrectly. An assertion failed saying that the string was not in the table. I looked into it with lldb and manually added the string it said was not there, it still didn't work.
I can't see from the code what is going wrong. Check to make sure that the strings are actually added to the `StringTableBuilder` when you call `add`, and take a look at the return values from that. I doubt they'll be much different from what `getOffset` eventually will return.

Also, setting the section name isn't part of laying out a section, so this feels like it's being done in the wrong palce.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1175
+Error MutableObjectWriter<ELFT>::layoutSegmentsAndSections() {
+  for (auto Seg : MutableObject.segments()) {
+    Expected<MutableELFSegment<ELFT> &> SegmentOrErr =
----------------
What about segments that are nested in other segments? This code doesn't handle that.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1181
+    MutableELFSegment<ELFT> &Segment = *SegmentOrErr;
+    Segment.Header.p_offset = CurrentOffset;
+
----------------
What if a program header covers the ELF header and program header table? This code won't work for that.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1190-1191
+    }
+    CurrentOffset = alignTo(
+        CurrentOffset, Segment.Header.p_align ? Segment.Header.p_align : 1);
+    Segment.Header.p_filesz = Segment.Header.p_offset - CurrentOffset;
----------------
I think you've got this alignment in the wrong place. I'm pretty sure it should be before the offset is set...


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1201-1203
+  for (uint64_t Sec :
+       seq<uint64_t>(1, std::distance(MutableObject.section_begin(),
+                                      MutableObject.section_end())))
----------------
It would be easier to see what's going on with a traditional for loop here.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:1250
+      return PhdrOrErr.takeError();
+    WriteToBuf(&PhdrOrErr->Header, sizeof(typename ELFT::Phdr));
+  }
----------------
What about data within segments not covered by sections? This code doesn't handle that.


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

https://reviews.llvm.org/D66403