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

[Rui Ueyama via llvm-commits]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160404/52ad535f/attachment.html>