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

[Reid Kleckner via Phabricator via llvm-commits]

rnk 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.
----------------
mstorsjo wrote:
> 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.
I see.


================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:119
+                           AuxData.data() + I * SymSize +
+                               sizeof(coff_symbol16));
+    }
----------------
mstorsjo wrote:
> 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.
Of course now I read you already clarified this. I think there should be a comment about how this is normalizing from coff_symbol32-sized entries to AuxSymbol sized entries, and discarding the padding bytes that are present in a bigobj.


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

https://reviews.llvm.org/D57009