[PATCH] D58380: [LLD] [COFF] Add -exclude-all-symbols for MinGW

Tue Feb 19 11:21:27 PST 2019

ruiu added inline comments.

================
Comment at: COFF/Driver.cpp:1613
+  // than MinGW in the case that nothing is explicitly exported.
+  if (Config->DLL && ((Config->MinGW && Config->Exports.empty() &&
+                       !Args.hasArg(OPT_exclude_all_symbols)) ||
----------------
mstorsjo wrote:
> ruiu wrote:
> > Shouldn't this be `Config->MinGW && Config->DLL && ...`? IIUC this is a mingw extension, and we don't want to run the code below if not mingw.
> We could do that as well. As far as I can see, it would only make a difference for the case when passing `-export-all-symbols` but not `-lldmingw`. The former is also an LLD private option only used by the MinGW frontend, so it doesn't really matter either way, but I can make it that way (I agree that the compound condition might be less confusing that way).
I think my preference is to make code that is expected to run only in MinGW mode to start with `if (Config->MinGW)` so that a reader can easily identify MinGW-specific features as such. Currently `-export-all-symbols` can be used without `-lldmingw`, but that's something no one cares, and I'd think for clarity `-export-all`-symbols` should be significant only when `-lldmingw` is also given.

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D58380