[PATCH] D38760: [LLD] [COFF] Add support for automatically exporting all symbols

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Driver.cpp:1212
 
+  if (Config->DLL && ((Config->MinGW && Config->Exports.empty()) ||
+                      Args.hasArg(OPT_export_all_symbols))) {
----------------
Is it doable for the mingw driver to always add -export-all-symbols if -export is empty? If you can do this, you can remove `Config->MinGW && Config->Exports.empty()` from this expression.


================
Comment at: COFF/Driver.cpp:1212
 
+  if (Config->DLL && ((Config->MinGW && Config->Exports.empty()) ||
+                      Args.hasArg(OPT_export_all_symbols))) {
----------------
ruiu wrote:
> Is it doable for the mingw driver to always add -export-all-symbols if -export is empty? If you can do this, you can remove `Config->MinGW && Config->Exports.empty()` from this expression.
This needs comment to make it clear that this is for MinGW, and in MinGW, all symbols are automatically exported if no symbols are chosen to be exported.


================
Comment at: COFF/Driver.cpp:1214-1244
+    std::vector<std::string> ExcludeSymbols = {
+        "_NULL_IMPORT_DESCRIPTOR",
+        // Runtime pseudo-reloc.
+        "_pei386_runtime_relocator",
+        "do_pseudo_reloc",
+        // Global vars that should not be exported.
+        "impure_ptr",
----------------
Can you make this a separate function which returns a std::set?


================
Comment at: COFF/Driver.cpp:1248
+      auto *Def = dyn_cast<Defined>(S->body());
+      if (Def && Def->isLive() && Def->getChunk()) {
+        if (ExcludeSymbolSet.find(Def->getName()) != ExcludeSymbolSet.end())
----------------
We generally prefer early returns, so please flip the condition and return here.


================
Comment at: COFF/Driver.cpp:1249
+      if (Def && Def->isLive() && Def->getChunk()) {
+        if (ExcludeSymbolSet.find(Def->getName()) != ExcludeSymbolSet.end())
+          return;
----------------
`count` is more appropriate than `find`.


https://reviews.llvm.org/D38760