Page MenuHomePhabricator

[ELF2] - Implemented --exclude-libs flag
AbandonedPublic

Authored by grimar on Oct 2 2015, 10:57 AM.

Details

Reviewers
ruiu
rafael
Summary

Specifies a list of archive libraries from which symbols should not be automatically exported. The library names may be delimited by commas. Specifying "--exclude-libs ALL" excludes symbols in all archive libraries from automatic export. For ELF targeted ports, symbols affected by this option will be treated as hidden.

Notes:

  • Original flag description contains "the library names may be delimited by commas or colons". This implemention handles only commas. Not sure do I need to add colons handling ? Never see them anywhere in use for any command line I think.

Diff Detail

Event Timeline

grimar updated this revision to Diff 36381.Oct 2 2015, 10:57 AM
grimar retitled this revision from to [ELF2] - Implemented --exclude-libs flag.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
grimar added inline comments.Oct 2 2015, 11:16 AM
ELF/Driver.cpp
9

Redundant declaration of Libs. Temporary code, will remove.

ruiu edited edge metadata.Oct 2 2015, 12:16 PM

I'm still reviewing this change, but I guess my question I have now is how important this feature is. Do you need this now, or is this high priority? New ELF LLD is still very early, and all code is likely to be refactored or modified, so I wouldn't want to add that much amount of code unless a feature is implemented pretty easily or important.

ELF/Config.h
2

Just use std::vector.

ELF/Driver.cpp
8–10
SmallVector<StringRef, 5> V;
Arg->getValue().split(V, ",", -1, false);
Config->ExcludeLibs.append(Config->ExcludeLibs.end(), V.begin(), V.end());
ELF/InputFiles.h
7

SymbolsHidden = false;

7–8

Do not define accessors if both are visible. They don't provide additional value. Just make the field public.

8

bool HideSymbols = true;

grimar updated this revision to Diff 36492.Oct 5 2015, 3:04 AM
grimar edited edge metadata.

Review comments addressed.

grimar added a comment.Oct 5 2015, 3:05 AM
In D13392#258851, @ruiu wrote:

I'm still reviewing this change, but I guess my question I have now is how important this feature is. Do you need this now, or is this high priority? New ELF LLD is still very early, and all code is likely to be refactored or modified, so I wouldn't want to add that much amount of code unless a feature is implemented pretty easily or important.

Not sure how this feature is important and I definitely dont need it right now.
I just thought it could be useful. At least from design side, before this patch there was no possibility to determine to which archive currently parced file belongs to.
Further modifications and refactorings at least should take care of that. All other changes performed seems minor and/or local for that flag to me.
Just in case I updated the diff to address all comments and be in touch with latest revision.
But If you think its too early fot that - please let me know and I will abandon the diff.

grimar abandoned this revision.Oct 6 2015, 1:07 AM
bero added a subscriber: bero.Jun 20 2017, 10:43 AM

This patch would be very useful for us: The Android build system relies on --exclude-libs ALL in several places (e.g. Bionic). We currently have an ongoing project to get AOSP to build entirely with lld (currently we're at a point where we use lld for binaries and keep using gold for shared libraries because of the --exclude-libs issue and a few problems we still have to hunt down).

ruiu edited edge metadata.Jun 20 2017, 12:01 PM

If Android needs the flag, I'm happy to re-implement this feature, but can you give me a pointer where the flag is used in Android? I want to understand how the flag is being used before implementing it.

bero added a comment.Jun 20 2017, 2:27 PM

Thanks! This is where it's currently being used (sometimes with a comment explaining the use):
https://android.googlesource.com/platform/bionic/+/master/libdl/Android.bp#18
https://android.googlesource.com/platform/bionic/+/master/libdl/Android.bp#37
https://android.googlesource.com/platform/bionic/+/master/libdl/Android.bp#43
https://android.googlesource.com/platform/bionic/+/master/libdl/Android.bp#86
https://android.googlesource.com/platform/bionic/+/master/libdl/Android.bp#91
https://android.googlesource.com/platform/bionic/+/master/libdl/Android.bp#94
https://android.googlesource.com/platform/bionic/+/master/linker/Android.bp#80
https://android.googlesource.com/platform/external/compiler-rt/+/master/Android.bp#376
https://android.googlesource.com/platform/external/compiler-rt/+/master/Android.bp#380
https://android.googlesource.com/platform/external/compiler-rt/+/master/Android.bp#384
https://android.googlesource.com/platform/external/compiler-rt/+/master/Android.bp#388
https://android.googlesource.com/platform/external/compiler-rt/+/master/Android.bp#392
https://android.googlesource.com/platform/external/compiler-rt/+/master/Android.bp#397
https://android.googlesource.com/platform/external/cblas/+/master/Android.bp#181
https://android.googlesource.com/platform/external/libcxx/+/master/Android.bp#89
https://android.googlesource.com/platform/frameworks/native/+/master/opengl/libs/Android.bp#110
https://android.googlesource.com/platform/frameworks/rs/+/master/support.bp#167
https://android.googlesource.com/platform/frameworks/rs/+/master/cpp/Android.bp#85
https://android.googlesource.com/platform/frameworks/rs/+/master/support/jni/Android.bp#33
https://android.googlesource.com/platform/frameworks/rs/+/master/support/jni/Android.bp#71

ruiu added a comment.Jun 20 2017, 2:47 PM

I re-implemented the feature in https://reviews.llvm.org/D34422. The new implementation uses a different strategy than this patch, and by utilizing the relationship of archive files and object files, the new implementation is much simpler and less intrusive than this one.

ruiu added a comment.Jun 21 2017, 8:39 AM

Bernhard,

I landed https://reviews.llvm.org/rL305920 to add the -exclude-libs flag.