This is an archive of the discontinued LLVM Phabricator instance.

[LLD] [COFF] Exclude certain static libraries and object files when exporting all symbols
ClosedPublic

Authored by mstorsjo on Oct 15 2017, 2:11 PM.

Details

Summary

This more or less matches what GNU ld does.

The clang_rt.builtins-$arch cases are a bit problematic; an alternative would be to trim the library name at the first period, and just have it match against libclang_rt.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 15 2017, 2:11 PM
ruiu edited edge metadata.Oct 15 2017, 2:18 PM

At this point I feel like we should define a function bool shouldExport(Defined *Sym) or something, so that we can move the code to that function.

In D38937#898214, @ruiu wrote:

At this point I feel like we should define a function bool shouldExport(Defined *Sym) or something, so that we can move the code to that function.

Sounds like a good idea. How do you suggest to handle the exclude StringSets in that case - recreate them in each function invocation? Or have a static StringSet<> ExcludeSymbols in the global scope?

For future direction; GNU ld has got options for adding symbols and libraries to the exclude libs, so in case someone actually needs and wants that feature, we might need to add support for such options. I haven't felt a great need for it so far though, since I don't think I've ever seen it used.

ruiu added a comment.Oct 15 2017, 2:31 PM
In D38937#898214, @ruiu wrote:

At this point I feel like we should define a function bool shouldExport(Defined *Sym) or something, so that we can move the code to that function.

Sounds like a good idea. How do you suggest to handle the exclude StringSets in that case - recreate them in each function invocation? Or have a static StringSet<> ExcludeSymbols in the global scope?

I don't know if this is a good idea, but since the number of strings you need to check is small, you may be able to just iterate over the list like this?

for (StringRef S : {"libgcc", "libgcc_s", ...})
  if (LibName == S)
    return false;
In D38937#898216, @ruiu wrote:
In D38937#898214, @ruiu wrote:

At this point I feel like we should define a function bool shouldExport(Defined *Sym) or something, so that we can move the code to that function.

Sounds like a good idea. How do you suggest to handle the exclude StringSets in that case - recreate them in each function invocation? Or have a static StringSet<> ExcludeSymbols in the global scope?

I don't know if this is a good idea, but since the number of strings you need to check is small, you may be able to just iterate over the list like this?

for (StringRef S : {"libgcc", "libgcc_s", ...})
  if (LibName == S)
    return false;

I feel that would end up pretty unwieldy. Additionally, the set of symbols to exclude is different depending on the architecture.

The symbol/lib/object sets should be easy to encapsulate in a class though - I'll update the diff with such a suggestion.

mstorsjo updated this revision to Diff 119110.Oct 16 2017, 12:09 AM

Wrap up all the sets and logic for deciding what symbols to export into a class.

Any further comments on this version?

ruiu added a comment.Oct 18 2017, 12:07 PM

I don't see a reason to use OO over plain functions in this patch, but this is probably okay. We can revisit it later. But I feel we are having too many functions for MinGW in Driver.cpp, so I was thinking that it might be a good idea to great MinGW.{h,cpp} in the COFF directory to put MinGW-related stuff there. What do you think?

In D38937#901090, @ruiu wrote:

I don't see a reason to use OO over plain functions in this patch, but this is probably okay.

The reason for OO was (as you probably guess) keeping the sets around for each invocation. It might be a case of premature optimization. The alternative while keeping them around, with plain functions, would be to store the sets in the calling context and passing them to the function, and at that point, wrapping them up in a class seemed to simplify the code.

We can revisit it later. But I feel we are having too many functions for MinGW in Driver.cpp, so I was thinking that it might be a good idea to great MinGW.{h,cpp} in the COFF directory to put MinGW-related stuff there. What do you think?

That's probably sensible. Do you want a preparatory patch doing that, and then rebasing this one on top of that?

ruiu accepted this revision.Oct 18 2017, 12:21 PM

LGTM given that you are going to create a follow-up patch to move MinGW-related stuff to new files.

This revision is now accepted and ready to land.Oct 18 2017, 12:21 PM
This revision was automatically updated to reflect the committed changes.