<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Apr 4, 2016 at 2:57 PM, Adhemerval Zanella <span dir="ltr"><<a href="mailto:adhemerval.zanella@linaro.org" target="_blank">adhemerval.zanella@linaro.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">zatrazz added inline comments.<br>
<span class=""><br>
================<br>
Comment at: ELF/DynamicList.cpp:34<br>
@@ +33,3 @@<br>
+//<br>
+//  { symbol1; symbol2; extern "C++" { symbol3; namespace::symbol4 }; }<br>
+//<br>
</span><span class="">----------------<br>
ruiu wrote:<br>
> zatrazz wrote:<br>
> > ruiu wrote:<br>
</span><span class="">> > > 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.<br>
> > 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.<br>
> 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.<br>
</span>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.<br>
<br>
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.<br>
<span class=""><br>
================<br>
Comment at: ELF/ScriptParser.cpp:23-25<br>
@@ -21,1 +22,5 @@<br>
<br>
+// Returns true if S matches T. S can contain glob meta-characters.<br>
+// The asterisk ('*') matches zero or more characacters, and the question<br>
+// mark ('?') matches one character.<br>
+bool elf::matchGlobStr(StringRef S, StringRef T) {<br>
----------------<br>
</span><span class="">pcc wrote:<br>
> ruiu wrote:<br>
> > zatrazz wrote:<br>
> > > ruiu wrote:<br>
> > > > 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.<br>
> > > As stated in <a href="http://llvm.org/pr26693" rel="noreferrer" target="_blank">http://llvm.org/pr26693</a> 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.<br>
> > Do you need the glob pattern fro the sanitizer?<br>
> FWIW, I think it should be pretty trivial to avoid using glob patterns in the sanitizer dynamic lists.<br>
</span>Yes, sanitizers uses dynamic list entries with '*' patterns.</blockquote><div><br></div><div>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.</div></div></div></div>