<div dir="ltr">Hi Justin,<div><br></div><div>> <span style="font-family:arial,sans-serif;font-size:13px">I'm pretty sure the extra false positives you're seeing here are the</span></div><span style="font-family:arial,sans-serif;font-size:13px">result of using an instrumented program with an uninstrumented shared</span><br style="font-family:arial,sans-serif;font-size:13px">
<span style="font-family:arial,sans-serif;font-size:13px">library.</span><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><font face="arial, sans-serif">That sounds like a likely cause. I still want to check out the "always_inline" attribute.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">> </font><span style="font-family:arial,sans-serif;font-size:13px">On the other hand, if I build libc++.so with -fsanitize=memory (via</span></div>
<span style="font-family:arial,sans-serif;font-size:13px">-DLLVM_USE_SANITIZER=Memory) and then run the tests against that with</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">the memory sanitizer, the new errors in string go away. There are quite</span><br style="font-family:arial,sans-serif;font-size:13px">
<span style="font-family:arial,sans-serif;font-size:13px">a few tests that seem to have other problems this way, but that's true</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">with or without the template instantiations.</span><div>
<span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><font face="arial, sans-serif">Unfortunately</font><span style="font-family:arial,sans-serif;font-size:13px"> I suspect building with -DLLVM_USE_SANITIZER only works when building in-tree.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">Something else we need to change to make libc++ work better with sanitizers.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">I'll try and get a patch out tonight that fixes most of these issues.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">I'll also have to look into the other errors.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Chandler mentioned that MSAN isn't very useful without an instrumented standard library. This puts us in a pickle.</span></div>
<div><font face="arial, sans-serif">Most of the time libc++ is shipped to users instrumented. I really don't want the end-user to see all these FPs.</font></div><div><font face="arial, sans-serif">Without the extern template declarations they don't appear. For that reason I think we should keep them out  for now.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">We could guard the definition of _LIBCPP_EXTERN_TEMPLATE with __has_feature(memory_sanitizer) (or w/e the correct name is).</font></div>
<div><font face="arial, sans-serif">But this will only work on clang and not GCC or MSVC. </font></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">Marshall is back tomorrow. I think he will have something to add to this as well.</font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">To the MSAN people: Can you offer any information on these false positives we are seeing?</font></div><div><font face="arial, sans-serif"><br>
</font></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">/Eric</font></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif"><br></font></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Aug 17, 2014 at 2:39 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">It would be useful to include some folks who work on the sanitizers.<div><br></div><div>MSan in particular is only really useful when linking against a sanitized standard library.</div>
</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra">
<br><br><div class="gmail_quote">On Sun, Aug 17, 2014 at 12:57 AM, 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">

I'm pretty sure the extra false positives you're seeing here are the<br>
result of using an instrumented program with an uninstrumented shared<br>
library.<br>
<br>
I wasn't able to get any interesting results from your example, but I<br>
followed your instructions to run the libc++ tests with msan. That is, I<br>
built llvm and clang with libcxx and libcxxabi checked out, and added<br>
compile_flags=['-fsanitize=memory'] in libc++'s lit.cfg. This reproduces<br>
the false positives in string that you mentioned.<br>
<br>
On the other hand, if I build libc++.so with -fsanitize=memory (via<br>
-DLLVM_USE_SANITIZER=Memory) and then run the tests against that with<br>
the memory sanitizer, the new errors in string go away. There are quite<br>
a few tests that seem to have other problems this way, but that's true<br>
with or without the template instantiations.<br>
<br>
So it looks to me like the "there is no good way to run libc++ tests<br>
with sanitizers" claim is pretty accurate ;)<br>
<div><div><br>
Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> writes:<br>
> Oh jeez, I forgot a very important aspect to the reproducer. Last time I<br>
> looked into this it seemed that __attribute__ ((__always_inline__)) also<br>
> played a part in this. In the last example "get_x()" should have this<br>
> attribute.<br>
><br>
> On Sat, Aug 16, 2014 at 3:04 AM, Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> wrote:<br>
><br>
>     I could have been a lot clearer. It seems to be a bug in MSAN. I've run<br>
>     into it before. <br>
>     The code that causes it is something like:<br>
><br>
>     // TestClass.hpp<br>
>     template <class T><br>
>     class TestClass<br>
>     {<br>
>        int x{0};<br>
>        int get_x() { return x; }<br>
>     };<br>
><br>
>     extern template class TestClass<int>;<br>
><br>
>     // TestClass.cpp<br>
>     template class TestClass<int>;<br>
><br>
>     // main.cpp <br>
>     #include "TestClass.hpp"<br>
>     int main()<br>
>     {<br>
>        TestClass<int> t;<br>
>        // MSAN fires on use of uninitialized value here.<br>
>        int x = t.get_x();te <br>
>     }<br>
><br>
>     That's by no means a reproducer, but it represents similar code to what<br>
>     causes the test failures in "/test/string". MSAN seems to fire *any* time<br>
>     a value is returned from a method of a class that has been instantiated<br>
>     externally. I think the shared object boundary might be important. I<br>
>     haven't had time to investigate. <br>
><br>
>     Unfortunately there is currently no good way to run the libc++ tests with<br>
>     sanitizers. The easiest way is to just insert "-fsanitize=memory" into the<br>
>     `compile_flags` list in `test/lit.cfg'.  Then just "cd test/string ; lit<br>
>     -sv . "<br>
><br>
>     I hope that made it a little clearer.<br>
><br>
>     /Eric<br>
><br>
>     P.S. These failures arn't localized to "test/string". They are anywhere<br>
>     templates are instantiated externally. String is just representative. <br>
><br>
>     On Sat, Aug 16, 2014 at 2:42 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>><br>
>     wrote:<br>
><br>
>         Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>> writes:<br>
>         > Hi Justin,<br>
>         ><br>
>         > I'm not quite sure we are should do that right now. <br>
>         > If your compiling with MSAN and you call a function in an extern<br>
>         > template MSAN will fire. <br>
><br>
>         I don't think I understand what you mean here. Which function? Why<br>
>         does<br>
>         msan fire?<br>
><br>
>         > For example testing test/string with MSAN enabled causes 124<br>
>         failures.<br>
>         > Normally there are just 2.<br>
><br>
>         If there are invalid memory accesses we should fix them. I don't see<br>
>         how<br>
>         explicit template instantiations would cause memory errors though.<br>
><br>
>         Are there bots that show this issue? How can I reproduce it?<br>
><br>
>         > We could probably guard the _LIBCPP_EXTERN_TEMPLATE definition to<br>
>         > check for MSAN but for now I don't think we are ready to re-enable<br>
>         > external template definitions.<br>
>         ><br>
>         > /Eric<br>
>         ><br>
>         > On Fri, Aug 15, 2014 at 11:58 AM, Justin Bogner <<br>
>         <a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>> wrote:<br>
>         ><br>
>         >     Author: bogner<br>
>         >     Date: Fri Aug 15 12:58:56 2014<br>
>         >     New Revision: 215740<br>
>         ><br>
>         >     URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215740&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215740&view=rev</a><br>
>         >     Log:<br>
>         >     Revert "Turn off extern templates for most uses."<br>
>         ><br>
>         >     Turning off explicit template instantiation leads to a pretty<br>
>         >     significant build time and code size cost. We're better off<br>
>         dealing<br>
>         >     with ABI incompatibility issues that come up in a less heavy<br>
>         handed<br>
>         >     way.<br>
>         ><br>
>         >     This reverts commit r189610.<br>
>         ><br>
>         >     Modified:<br>
>         >         libcxx/trunk/include/__config<br>
>         >         libcxx/trunk/src/algorithm.cpp<br>
>         >         libcxx/trunk/src/ios.cpp<br>
>         >         libcxx/trunk/src/locale.cpp<br>
>         >         libcxx/trunk/src/string.cpp<br>
>         >         libcxx/trunk/src/valarray.cpp<br>
>         ><br>
>         >     Modified: libcxx/trunk/include/__config<br>
>         >     URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/</a><br>
>         __config?rev<br>
>         >     =215740&r1=215739&r2=215740&view=diff<br>
>         >     ================================================================<br>
>         ==========<br>
>         >     ====<br>
>         >     --- libcxx/trunk/include/__config (original)<br>
>         >     +++ libcxx/trunk/include/__config Fri Aug 15 12:58:56 2014<br>
>         >     @@ -608,7 +608,7 @@ template <unsigned> struct __static_asse<br>
>         >      #endif<br>
>         ><br>
>         >      #ifndef _LIBCPP_EXTERN_TEMPLATE<br>
>         >     -#define _LIBCPP_EXTERN_TEMPLATE(...)<br>
>         >     +#define _LIBCPP_EXTERN_TEMPLATE(...) extern template<br>
>         __VA_ARGS__;<br>
>         >      #endif<br>
>         ><br>
>         >      #ifndef _LIBCPP_EXTERN_TEMPLATE2<br>
>         ><br>
>         >     Modified: libcxx/trunk/src/algorithm.cpp<br>
>         >     URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/</a><br>
>         algorithm.cpp?<br>
>         >     rev=215740&r1=215739&r2=215740&view=diff<br>
>         >     ================================================================<br>
>         ==========<br>
>         >     ====<br>
>         >     --- libcxx/trunk/src/algorithm.cpp (original)<br>
>         >     +++ libcxx/trunk/src/algorithm.cpp Fri Aug 15 12:58:56 2014<br>
>         >     @@ -7,7 +7,6 @@<br>
>         >      //<br>
>         >      //===<br>
>         >   <br>
>          ----------------------------------------------------------------------<br>
>         ===/<br>
>         >     /<br>
>         ><br>
>         >     -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template<br>
>         __VA_ARGS__;<br>
>         >      #include "algorithm"<br>
>         >      #include "random"<br>
>         >      #include "mutex"<br>
>         ><br>
>         >     Modified: libcxx/trunk/src/ios.cpp<br>
>         >     URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/</a><br>
>         ios.cpp?rev=<br>
>         >     215740&r1=215739&r2=215740&view=diff<br>
>         >     ================================================================<br>
>         ==========<br>
>         >     ====<br>
>         >     --- libcxx/trunk/src/ios.cpp (original)<br>
>         >     +++ libcxx/trunk/src/ios.cpp Fri Aug 15 12:58:56 2014<br>
>         >     @@ -7,8 +7,6 @@<br>
>         >      //<br>
>         >      //===<br>
>         >   <br>
>          ----------------------------------------------------------------------<br>
>         ===/<br>
>         >     /<br>
>         ><br>
>         >     -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template<br>
>         __VA_ARGS__;<br>
>         >     -<br>
>         >      #include "ios"<br>
>         >      #include "streambuf"<br>
>         >      #include "istream"<br>
>         ><br>
>         >     Modified: libcxx/trunk/src/locale.cpp<br>
>         >     URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/</a><br>
>         locale.cpp?rev=<br>
>         >     215740&r1=215739&r2=215740&view=diff<br>
>         >     ================================================================<br>
>         ==========<br>
>         >     ====<br>
>         >     --- libcxx/trunk/src/locale.cpp (original)<br>
>         >     +++ libcxx/trunk/src/locale.cpp Fri Aug 15 12:58:56 2014<br>
>         >     @@ -7,8 +7,6 @@<br>
>         >      //<br>
>         >      //===<br>
>         >   <br>
>          ----------------------------------------------------------------------<br>
>         ===/<br>
>         >     /<br>
>         ><br>
>         >     -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template<br>
>         __VA_ARGS__;<br>
>         >     -<br>
>         >      // On Solaris, we need to define something to make the C99<br>
>         parts of<br>
>         >     localeconv<br>
>         >      // visible.<br>
>         >      #ifdef __sun__<br>
>         ><br>
>         >     Modified: libcxx/trunk/src/string.cpp<br>
>         >     URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/</a><br>
>         string.cpp?rev=<br>
>         >     215740&r1=215739&r2=215740&view=diff<br>
>         >     ================================================================<br>
>         ==========<br>
>         >     ====<br>
>         >     --- libcxx/trunk/src/string.cpp (original)<br>
>         >     +++ libcxx/trunk/src/string.cpp Fri Aug 15 12:58:56 2014<br>
>         >     @@ -7,8 +7,6 @@<br>
>         >      //<br>
>         >      //===<br>
>         >   <br>
>          ----------------------------------------------------------------------<br>
>         ===/<br>
>         >     /<br>
>         ><br>
>         >     -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template<br>
>         __VA_ARGS__;<br>
>         >     -<br>
>         >      #include "string"<br>
>         >      #include "cstdlib"<br>
>         >      #include "cwchar"<br>
>         ><br>
>         >     Modified: libcxx/trunk/src/valarray.cpp<br>
>         >     URL: <a href="http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/" target="_blank">http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/</a><br>
>         valarray.cpp?rev<br>
>         >     =215740&r1=215739&r2=215740&view=diff<br>
>         >     ================================================================<br>
>         ==========<br>
>         >     ====<br>
>         >     --- libcxx/trunk/src/valarray.cpp (original)<br>
>         >     +++ libcxx/trunk/src/valarray.cpp Fri Aug 15 12:58:56 2014<br>
>         >     @@ -7,8 +7,6 @@<br>
>         >      //<br>
>         >      //===<br>
>         >   <br>
>          ----------------------------------------------------------------------<br>
>         ===/<br>
>         >     /<br>
>         ><br>
>         >     -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template<br>
>         __VA_ARGS__;<br>
>         >     -<br>
>         >      #include "valarray"<br>
>         ><br>
>         >      _LIBCPP_BEGIN_NAMESPACE_STD<br>
>         ><br>
>         >     _______________________________________________<br>
>         >     cfe-commits mailing list<br>
>         >     <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
>         >     <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>