[PATCH] D57009: [llvm-objcopy] [COFF] Fix handling of aux symbols for big objects

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 2 inline comments as done.
mstorsjo added inline comments.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:52-53
   StringRef Name;
+  // AuxData stores the aux symbols as a series of coff_symbol16 sized
+  // entries in the vector. If written to a bigobj, each aux symbol is padded
+  // with 2 null bytes at the end.
----------------
rnk wrote:
> Well, they aren't always coff_symbol16 sized are they? For an input bigobj, it'll be coff_symbol32, or we should make this a vector of coff_symbol16 directly.
> 
> I don't know much about objcopy, but I think it might be more in the spirit of it to widen into coff_symbol32 as is done for the main symbol field above, instead of keeping this as an opaque binary blob.
Even for bigobj inputs, the aux symbols (except for .file) only have coff_symbol16 worth of payload. There's no wide version of any of the `coff_aux_..` structs, so they can't be widened into the intermediate storage.

Making it a vector of coff_symbol16 would make things clearer, but as the data actually isn't that struct, maybe `struct { uint8_t Opaque[sizeof(coff_symbol16)]; }` would be more correct?

Or alternatively `Optional<coff_aux_*>` for each of the known types - but I prefer being able to passthrough unknown data untouched.


================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:119
+                           AuxData.data() + I * SymSize +
+                               sizeof(coff_symbol16));
+    }
----------------
rnk wrote:
> Should this second `sizeof(coff_symbol16)` be SymSize?
> 
> Maybe an easier way to express it would be:
> ```
> ArrayRef<uint8_t> Chunk = AuxData.take_front(SymSize);
> Sym.AuxData.insert(Sym.AuxData.end(), Chunk.begin(), Chunk.end());
> AuxData = AuxData.drop_front(SymSize);
> ```
> It mutates a local variable, but takes less math.
No, this is intentionally (within the current patch design) copying 18 bytes from a source which has got either 18 or 20 bytes stride.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57009