[PATCH] D33964: [LLVM][llvm-objcopy] Added basic plumbing to get things started

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D33964#794100, @jakehehrlich wrote:

> 2. Sometimes sections that don't belong in segments would wind up in a PT_LOAD segment that had Offset == 0. This is fixed by only adding allocated sections to segments (see Object::readSectionHeaders)


I disagree with this approach - yes non-allocatable sections should not end up in PT_LOAD (although using linker scripts, I wouldn't be surprised if it is possible), but it is perfectly legitimate to have segments that are not loadable with non-SHF_ALLOC sections (see the ongoing discussion in https://reviews.llvm.org/D34204 for explanation). I'm not quite sure what the case here is that you've encountered, but it sounds to me like the issue is elsewhere.

Also, I noticed that you don't ignore SHT_NULL sections, but you probably should. Large proposal 74 ELFs with many sections have metadata stored in that section. I'd suggest two things:

1. Don't add support for Prop74 just yet - you can check somewhere if it is one and then bail out, to be safe.
2. Skip the SHT_NULL section when reading section headers, and recreate it later on.



================
Comment at: tools/llvm-objcopy/Object.cpp:107
+  for (const auto &Shdr : unwrapOrError(ElfFile.sections())) {
+    if (Shdr.sh_type == SHT_STRTAB)
+      continue;
----------------
This should probably be SHT_STRTAB || SHT_NULL.


Repository:
  rL LLVM

https://reviews.llvm.org/D33964