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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> Other llvm-objcopy reviewers: I'd like to add a custom hidden option for testing, for triggering using the big object format. Without that, a test would have to create over 32k sections to trigger that.

I'm not a fan of adding hidden options purely for testing when there are alternatives. In the ELF tests, we use a pre-built zip file containing an object with many sections (see ELF/many-sections.test). I think you could probably do the same thing here.

Regarding the code, I honestly don't really understand it, so I don't feel like I'm qualified to review this COFF-ism.



================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:111
+    assert(AuxData.size() == SymSize * SymRef.getNumberOfAuxSymbols());
+    if (SymRef.isFileRecord()) {
+      Sym.AuxFile = StringRef(reinterpret_cast<const char *>(AuxData.data()),
----------------
You can get rid of the braces here.


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:320
     Ptr += sizeof(SymbolTy);
-    std::copy(S.AuxData.begin(), S.AuxData.end(), Ptr);
-    Ptr += S.AuxData.size();
+    if (!S.AuxFile.empty()) {
+      std::copy(S.AuxFile.begin(), S.AuxFile.end(), Ptr);
----------------
Strictly speaking, you don't need the braces in this if/else.


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:326-328
+        std::copy(S.AuxData.data() + I * sizeof(coff_symbol16),
+                  S.AuxData.data() + (I + 1) * sizeof(coff_symbol16),
+                  Ptr + I * sizeof(SymbolTy));
----------------
Are coff_symbol16 and SymbolTy supposed to be the same size? Or are you deliberately writing less (or more) into the buffer than you iterate over?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57009