<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 13, 2015 at 4:38 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Alexey Samsonov <<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>> writes:<br>
> Hi Ryan,<br>
><br>
> On Sun, Apr 5, 2015 at 4:47 PM, Ryan Govostes <<a href="mailto:rzg@apple.com">rzg@apple.com</a>> wrote:<br>
>> The documentation for the sanitizer special case list format[0] says,<br>
>><br>
>> > The meanining of * in regular expression for entity names is different<br>
>> - it is treated as in shell wildcarding.<br>
>><br>
>> In SpecialCaseList::parse, we see that this is just replacing * with .*:<br>
>><br>
>> // Replace * with .*<br>
>> for (size_t pos = 0; (pos = Regexp.find("*", pos)) != std::string::npos;<br>
>> pos += strlen(".*")) {<br>
>> Regexp.replace(pos, strlen("*"), ".*");<br>
>> }<br>
>><br>
>> This seems to introduce more problems than it solves, since (i) this<br>
>> doesn’t really behave like a shell globbing wildcard as advertised, and<br>
>> (ii) if the user tries to use * as a regex quantifier, this will match<br>
>> incorrectly: A* matches the empty string and any number of As, while A.*<br>
>> matches all strings that start with at least one A.<br>
>><br>
>> If it’s forgivable to break compatibility here, we should do regular<br>
>> expressions _or_ shell globbing, and not a hybrid format.<br>
><br>
> I agree that the current format description is misleading, e.g. "foo*bar"<br>
> will also match "fooxxx/yyy/zzzbar", which might be unexpected for user<br>
> expecting a shell globbing. For now we should at least change documentation<br>
> to reflect that. In retrospective, replacing "*" with ".*" doesn't look like<br>
> a good idea at all, and for simplicity I'd prefer to just use regular<br>
> expressions. Adding special case for "file paths" isn't nice:<br>
> 1) as you mention, we have to do careful escaping<br>
> 2) at the moment special case list format is generic and is not tied to "src"<br>
> or "fun" entities, it's specific sanitizers that introduce logic on top of<br>
> it. I'd prefer to treat all special case list entries in a similar way.<br>
><br>
> However, I'm really afraid of breaking compatibility :( I know several users<br>
> of blacklist that already use "*" meaning ".*", and it would be challenging<br>
> to migrate them - e.g. you have to keep two different blacklist files for<br>
> older and newer Clang...<br>
<br>
</div></div>TBH, this doesn't seem like that big of a deal to me. The "*" behaviour<br>
is strange and confusing, and I'd expect most people to use sanitizers<br>
mostly from their newer compiler if they use multiple - they work<br>
better. Compiling with older compilers is important for a lot of use<br>
cases, but I can't see why people would run sanitizers from two<br>
different versions of the compiler at the same time.<br></blockquote><div><br></div><div>Well, suppose you build your project with Clang 3.6 and have my/project/asan_blacklist.txt</div><div>checked in your VCS. Now, if you will decide to switch to Clang 3.7 you'd have to *atomically*</div><div>upgrade the contents of my/project/asan_blacklist.txt. If some developers are still using Clang 3.6,</div><div>you'd have to keep the older version there, and do magic tricks in the build system so that it</div><div>would choose between two versions of asan_blacklist.txt. I'm not saying it's impossible</div><div>(things will be easier for you if you use default blacklist, for instance), it's just painful.</div><div><br></div><div>You will also not be able to see the problem instantly - your old blacklist will be treated</div><div>differently, and suddenly some functions blacklisted long time ago would be silently un-blacklisted,</div><div>triggering long-closed bugs.</div><div><br></div><div>-fsanitize-old-blacklist-format, which Nick suggests below, is an option. However, that would</div><div>also require passing different flags with different compiler versions. And, again, we could decide</div><div>to add more blacklist features in the future. Or GCC people could implement some version of it...</div><div><br></div><div>Another option is to add versioning to the file format (we had to do smth. similar for coverage</div><div>file format, for instance). E.g. if old file was:</div><div>----</div><div>fun:foo*bar</div><div>src:*baz.cpp</div><div>----</div><div><br></div><div>new file will be:</div><div><br></div><div>----</div><div>### 0002 ####</div><div># First line contains 4-digit string specifying the version number</div><div>fun:foo.*bar</div><div>src:.*baz.cpp</div><div>----</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> The only option I see is to introduce versioning to special case list format.<br>
> That's unfortunate and extra complexity, but can be useful if we decide to<br>
> make further changes.<br>
><br>
> I’d prefer shell globbing for paths in src entities, but that isn’t as<br>
> useful for function names. Most filenames will contain periods, which<br>
> also need to be escaped properly as regular expressions. (This also<br>
> limits the usefulness of treating literals separately.)<br>
><br>
> (Just a note: the way that regular expressions are concatenated in<br>
> ::parse appears to have a bug if a pattern contains a pipe.)<br>
><br>
> Patches welcome :) You can also open a bug against me, but I can't guarantee<br>
> I will get to this (or the suggestion above) in the nearest future.<br>
><br>
><br>
> Ryan<br>
><br>
> 0: <a href="http://clang.llvm.org/docs/SanitizerSpecialCaseList.html" target="_blank">http://clang.llvm.org/docs/SanitizerSpecialCaseList.html</a><br>
><br>
> diff --git a/lib/Support/SpecialCaseList.cpp b/lib/Support/<br>
> SpecialCaseList.cpp<br>
> index c312cc1..2972cb1 100644<br>
> --- a/lib/Support/SpecialCaseList.cpp<br>
> +++ b/lib/Support/SpecialCaseList.cpp<br>
> @@ -133,7 +133,7 @@ bool SpecialCaseList::parse(const MemoryBuffer *MB,<br>
> std::string &Error) {<br>
> // Add this regexp into the proper group by its prefix.<br>
> if (!Regexps[Prefix][Category].empty())<br>
> Regexps[Prefix][Category] += "|";<br>
> - Regexps[Prefix][Category] += "^" + Regexp + "$";<br>
> + Regexps[Prefix][Category] += "^(" + Regexp + ")$)";<br>
> }<br>
> return true;<br>
> }<br>
><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
><br>
> --<br>
> Alexey Samsonov<br>
> <a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a><br>
><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div></div>
</div></div>