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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 4 inline comments as done.
mstorsjo added a comment.

In D57009#1367445 <https://reviews.llvm.org/D57009#1367445>, @jhenderson wrote:

> In D57009#1367437 <https://reviews.llvm.org/D57009#1367437>, @mstorsjo wrote:
>
> > In D57009#1366076 <https://reviews.llvm.org/D57009#1366076>, @jhenderson wrote:
> >
> > > > 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.
> >
> >
> > I'm experimenting with crafting such an input file now. The uncompressed object file weighs in at around 5 MB, and after gzip (as is used for that ELF test) it currently ends up at around 725 KB. Do you think that's acceptable or too large?
>
>
> It's not ideal, if I'm honest, but it might be a quirk of the file format, and therefore unavoidable. The equivalent file for ELF is only 152 KB. I don't know if it was somehow more aggressively compressed though. If you can't get it any smaller, I think it's probably acceptable.


I realized I could make the testcase less interesting and remove some aspects that aren't strictly needed for this test. That reduced the uncompressed object from 5 MB to 2.5, and the compressed one from 725 KB to 7 KB. That's probably small enough :-)



================
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:
> 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?
Oh, right, I forgot about the other comments when focusing on the test data.


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

https://reviews.llvm.org/D57009