[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.
----------------
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.


================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:114
+                              AuxData.size())
+                        .rtrim(StringRef("\0", /*length=*/1));
+    } else {
----------------
I think this can just be `.rtrim('\0')`, there is a single character overload of rtrim.


================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:119
+                           AuxData.data() + I * SymSize +
+                               sizeof(coff_symbol16));
+    }
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57009