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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/COFF/bigobj.test:5-8
+# Remove a section, making the section count fit into a small object.
+
+RUN: llvm-objcopy -R '.text$4' %t.in.o %t.small.o
+RUN: llvm-objdump -t %t.small.o | FileCheck %s --check-prefixes=SYMBOLS,SYMBOLS-SMALL,SYMBOLS-REMOVED-SMALL
----------------
I think it probably is easier to associate comments with the corresponding test case without the blank link between them, but I'm not too bothered, so if you prefer it this way, that's fine.


================
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()),
----------------
jhenderson wrote:
> You can get rid of the braces here.
You can still get rid of these braces ;)

I think a few code comments around here explaining what you are doing and why would make it much more understandable. Perhaps a brief explanation of the difference in the BigObj format?


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:148-149
+    if (!S.AuxFile.empty())
+      S.Sym.NumberOfAuxSymbols =
+          alignTo(S.AuxFile.size(), sizeof(SymbolTy)) / sizeof(SymbolTy);
+    S.RawIndex = RawSymIndex;
----------------
This is another place requiring a code comment, I think, just explaining the "why".


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:322-323
     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);
+      // This assumes that unwritten parts of the memory mapped file
----------------
Again, a few comments around here would be good.


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

https://reviews.llvm.org/D57009