[PATCH] D47565: Fix /WholeArchive bug.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Driver.cpp:1234
+  for (auto *Arg : Args.filtered(OPT_wholearchive_file))
+    WholeArchive.insert(Arg->getValue());
+
----------------
rnk wrote:
> rnk wrote:
> > I think we need to canonicalize the path a little to ensure that this works on case insensitive file systems:
> > $ lld-link foo.lib -wholearchive:Foo.lib
> > 
> > We shouldn't enqueue the same file twice, right?
> > 
> > Also, maybe we should just enqueue all whole archive inputs in this loop, up front, and then only process inputs later if the input wasn't already added as a whole archive input.
> I guess to make it easy to write a cross-platform test for path canonicalization, the test could use:
> $ lld-link ./foo.lib -wholearchive:foo.lib
We shouldn't generally add the same file more than once. Added code to do that check.


================
Comment at: lld/COFF/Driver.cpp:1242
+    if (Optional<StringRef> Path = findFile(Arg->getValue()))
+      enqueuePath(*Path, Args.hasArg(OPT_wholearchive_flag) ||
+                             WholeArchive.count(Arg->getValue()));
----------------
rnk wrote:
> I don't think `Args.hasArg(OPT_wholearchive_flag)` is the right check here, that asks if a whole archive flag appears anywhere on the command line, not if this particular flag is whole archive.
This expression correctly checks for that condition, no?


https://reviews.llvm.org/D47565