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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: COFF/Driver.cpp:1212
 
+  if (Config->DLL && ((Config->MinGW && Config->Exports.empty()) ||
+                      Args.hasArg(OPT_export_all_symbols))) {
----------------
mstorsjo wrote:
> ruiu wrote:
> > 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.
> No, the mingw driver doesn't know whether the object files will contain any dllexport directives, so it can't take care of setting that flag.
Ok, will add such a comment.


================
Comment at: COFF/Driver.cpp:1214
+                      Args.hasArg(OPT_export_all_symbols))) {
+    std::vector<std::string> ExcludeSymbols = {
+        "_NULL_IMPORT_DESCRIPTOR",
----------------
zturner wrote:
> ruiu wrote:
> > Can you make this a separate function which returns a std::set?
> Can you make this a `vector<StringRef>`, but add an additional `_` to the beginning of each item?
> 
> Then, below you can write:
> 
> ```
> if (Config->Machine != I386) {
>   for (auto &Str : ExcludeSymbols) {
>     Str = Str.drop_front().substr(0, Str.find('@'));
>   }
> }
> ```
> 
> saving unnecessary allocations on all code paths.
Ok, that should be doable.


================
Comment at: COFF/Driver.cpp:1243-1244
+    }
+    std::set<StringRef> ExcludeSymbolSet(ExcludeSymbols.begin(),
+                                         ExcludeSymbols.end());
+
----------------
zturner wrote:
> Since you've explicitly defined this set above, and there is no way to get additional items into it, why do we need a set?  Seems like you can just `std::sort` the original vector and then `std::binary_search` for symbols.
Ok, changing this into a separate function that returns a sorted vector.


================
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())
----------------
ruiu wrote:
> We generally prefer early returns, so please flip the condition and return here.
Ok, will do


https://reviews.llvm.org/D38760