[PATCH] D18772: [lld] Implement --dynamic-list

[Rafael Espíndola via llvm-commits]

At least for a first patch I agree with Rui. Lets implement the
simplest thing: no c++, no glob.

Please add a few more test cases

* Executable with --export-dynamic.
* Shared library.
* Include trying to export a symbol that is not normally visible, like
a hidden one. Hopefully the expected semantics is that it stays
hidden.



On 4 April 2016 at 17:59, Rui Ueyama <ruiu at google.com> wrote:
> On Mon, Apr 4, 2016 at 2:57 PM, Adhemerval Zanella
> <adhemerval.zanella at linaro.org> wrote:
>>
>> zatrazz added inline comments.
>>
>> ================
>> Comment at: ELF/DynamicList.cpp:34
>> @@ +33,3 @@
>> +//
>> +//  { symbol1; symbol2; extern "C++" { symbol3; namespace::symbol4 }; }
>> +//
>> ----------------
>> ruiu wrote:
>> > zatrazz wrote:
>> > > ruiu wrote:
>> > > > I also don't like to support C++ mangled names now. There's a chance
>> > > > that we might want that later, but this patch is already too large, so
>> > > > please remove that feature.
>> > > Although llvm itself does not use, I see no direct reason to not
>> > > support. It does not interfere significantly with default matching rules
>> > > (for glob patterns it will first check the language mangling and only
>> > > applying the damangling in such case) if the script does not use it.
>> > Apparently this part of linker script was not designed with speed in
>> > mind, and I think the gnu linker went a bit too far on this front. How many
>> > users are really using this feature? Is this really what we want to have? We
>> > need to think carefully to decide whether we want to do the same thing as
>> > the gnu linker for mangled names.
>> I would say it was designed for flexibility, since the idea is to define
>> C++ (or any other supported language) symbols without tie to mangling reles
>> for an specific ABI. It is possible to just use the mangled name instead,
>> but since GNU ld supports multiples system with different mangling name
>> schemes, this seems the solution to avoid such trap.
>>
>> Anyway, I think performance-wise I it won't hurt default case, since
>> demangling will just be used if 'extern...' is actually issued in the
>> script. About the users, I am not sure since sanitizers itself does not make
>> use of this (since it exports its API as C). However I expect that C++
>> libraries may use this instead of using mangling names.
>>
>> ================
>> Comment at: ELF/ScriptParser.cpp:23-25
>> @@ -21,1 +22,5 @@
>>
>> +// Returns true if S matches T. S can contain glob meta-characters.
>> +// The asterisk ('*') matches zero or more characacters, and the question
>> +// mark ('?') matches one character.
>> +bool elf::matchGlobStr(StringRef S, StringRef T) {
>> ----------------
>> pcc wrote:
>> > ruiu wrote:
>> > > zatrazz wrote:
>> > > > ruiu wrote:
>> > > > > At least for now, I don't want to support glob patterns at all.
>> > > > > Glob pattern can have huge performance impact on the linking speed. In most
>> > > > > cases users pass a list of complete symbol names, so we should reject any
>> > > > > patterns that contains glob metacharacters and do simple hash table lookup
>> > > > > instead of glob pattern matching.
>> > > > As stated in http://llvm.org/pr26693 it is required for the
>> > > > sanitizers and at least from LLVm side it the main motivator for this patch.
>> > > > I think we can add an additional check on DynamicList to check if an dynamic
>> > > > list and fast return if it is not the case.
>> > > Do you need the glob pattern fro the sanitizer?
>> > FWIW, I think it should be pretty trivial to avoid using glob patterns
>> > in the sanitizer dynamic lists.
>> Yes, sanitizers uses dynamic list entries with '*' patterns.
>
>
> But if it is trivial to remove glob patterns, I'd do that on the sanitizer
> side. It is generally good to explicitly list all symbols rather than using
> wildcard.