[libcxx] r215740 - Revert "Turn off extern templates for most uses."
Eric Fiselier
eric at efcs.ca
Sun Aug 17 17:39:18 PDT 2014
Hi All,
I've attached a reproducer. just run `make` to build and execute it.
/Eric
On Sun, Aug 17, 2014 at 2:38 PM, Eric Fiselier <eric at efcs.ca> wrote:
> Hi Justin,
>
> > I'm pretty sure the extra false positives you're seeing here are the
> result of using an instrumented program with an uninstrumented shared
> library.
>
> That sounds like a likely cause. I still want to check out the
> "always_inline" attribute.
>
>
>
> > On the other hand, if I build libc++.so with -fsanitize=memory (via
> -DLLVM_USE_SANITIZER=Memory) and then run the tests against that with
> the memory sanitizer, the new errors in string go away. There are quite
> a few tests that seem to have other problems this way, but that's true
> with or without the template instantiations.
>
> Unfortunately I suspect building with -DLLVM_USE_SANITIZER only works
> when building in-tree.
> Something else we need to change to make libc++ work better with
> sanitizers.
> I'll try and get a patch out tonight that fixes most of these issues.
> I'll also have to look into the other errors.
>
> Chandler mentioned that MSAN isn't very useful without an instrumented
> standard library. This puts us in a pickle.
> Most of the time libc++ is shipped to users instrumented. I really don't
> want the end-user to see all these FPs.
> Without the extern template declarations they don't appear. For that
> reason I think we should keep them out for now.
>
> We could guard the definition of _LIBCPP_EXTERN_TEMPLATE with
> __has_feature(memory_sanitizer) (or w/e the correct name is).
> But this will only work on clang and not GCC or MSVC.
>
> Marshall is back tomorrow. I think he will have something to add to this
> as well.
>
> To the MSAN people: Can you offer any information on these false positives
> we are seeing?
>
>
> /Eric
>
>
>
>
>
> On Sun, Aug 17, 2014 at 2:39 AM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
>> It would be useful to include some folks who work on the sanitizers.
>>
>> MSan in particular is only really useful when linking against a sanitized
>> standard library.
>>
>>
>> On Sun, Aug 17, 2014 at 12:57 AM, Justin Bogner <mail at justinbogner.com>
>> wrote:
>>
>>> I'm pretty sure the extra false positives you're seeing here are the
>>> result of using an instrumented program with an uninstrumented shared
>>> library.
>>>
>>> I wasn't able to get any interesting results from your example, but I
>>> followed your instructions to run the libc++ tests with msan. That is, I
>>> built llvm and clang with libcxx and libcxxabi checked out, and added
>>> compile_flags=['-fsanitize=memory'] in libc++'s lit.cfg. This reproduces
>>> the false positives in string that you mentioned.
>>>
>>> On the other hand, if I build libc++.so with -fsanitize=memory (via
>>> -DLLVM_USE_SANITIZER=Memory) and then run the tests against that with
>>> the memory sanitizer, the new errors in string go away. There are quite
>>> a few tests that seem to have other problems this way, but that's true
>>> with or without the template instantiations.
>>>
>>> So it looks to me like the "there is no good way to run libc++ tests
>>> with sanitizers" claim is pretty accurate ;)
>>>
>>> Eric Fiselier <eric at efcs.ca> writes:
>>> > Oh jeez, I forgot a very important aspect to the reproducer. Last time
>>> I
>>> > looked into this it seemed that __attribute__ ((__always_inline__))
>>> also
>>> > played a part in this. In the last example "get_x()" should have this
>>> > attribute.
>>> >
>>> > On Sat, Aug 16, 2014 at 3:04 AM, Eric Fiselier <eric at efcs.ca> wrote:
>>> >
>>> > I could have been a lot clearer. It seems to be a bug in MSAN.
>>> I've run
>>> > into it before.
>>> > The code that causes it is something like:
>>> >
>>> > // TestClass.hpp
>>> > template <class T>
>>> > class TestClass
>>> > {
>>> > int x{0};
>>> > int get_x() { return x; }
>>> > };
>>> >
>>> > extern template class TestClass<int>;
>>> >
>>> > // TestClass.cpp
>>> > template class TestClass<int>;
>>> >
>>> > // main.cpp
>>> > #include "TestClass.hpp"
>>> > int main()
>>> > {
>>> > TestClass<int> t;
>>> > // MSAN fires on use of uninitialized value here.
>>> > int x = t.get_x();te
>>> > }
>>> >
>>> > That's by no means a reproducer, but it represents similar code to
>>> what
>>> > causes the test failures in "/test/string". MSAN seems to fire
>>> *any* time
>>> > a value is returned from a method of a class that has been
>>> instantiated
>>> > externally. I think the shared object boundary might be important.
>>> I
>>> > haven't had time to investigate.
>>> >
>>> > Unfortunately there is currently no good way to run the libc++
>>> tests with
>>> > sanitizers. The easiest way is to just insert "-fsanitize=memory"
>>> into the
>>> > `compile_flags` list in `test/lit.cfg'. Then just "cd test/string
>>> ; lit
>>> > -sv . "
>>> >
>>> > I hope that made it a little clearer.
>>> >
>>> > /Eric
>>> >
>>> > P.S. These failures arn't localized to "test/string". They are
>>> anywhere
>>> > templates are instantiated externally. String is just
>>> representative.
>>> >
>>> > On Sat, Aug 16, 2014 at 2:42 AM, Justin Bogner <
>>> mail at justinbogner.com>
>>> > wrote:
>>> >
>>> > Eric Fiselier <eric at efcs.ca> writes:
>>> > > Hi Justin,
>>> > >
>>> > > I'm not quite sure we are should do that right now.
>>> > > If your compiling with MSAN and you call a function in an
>>> extern
>>> > > template MSAN will fire.
>>> >
>>> > I don't think I understand what you mean here. Which function?
>>> Why
>>> > does
>>> > msan fire?
>>> >
>>> > > For example testing test/string with MSAN enabled causes 124
>>> > failures.
>>> > > Normally there are just 2.
>>> >
>>> > If there are invalid memory accesses we should fix them. I
>>> don't see
>>> > how
>>> > explicit template instantiations would cause memory errors
>>> though.
>>> >
>>> > Are there bots that show this issue? How can I reproduce it?
>>> >
>>> > > We could probably guard the _LIBCPP_EXTERN_TEMPLATE
>>> definition to
>>> > > check for MSAN but for now I don't think we are ready to
>>> re-enable
>>> > > external template definitions.
>>> > >
>>> > > /Eric
>>> > >
>>> > > On Fri, Aug 15, 2014 at 11:58 AM, Justin Bogner <
>>> > mail at justinbogner.com> wrote:
>>> > >
>>> > > Author: bogner
>>> > > Date: Fri Aug 15 12:58:56 2014
>>> > > New Revision: 215740
>>> > >
>>> > > URL:
>>> http://llvm.org/viewvc/llvm-project?rev=215740&view=rev
>>> > > Log:
>>> > > Revert "Turn off extern templates for most uses."
>>> > >
>>> > > Turning off explicit template instantiation leads to a
>>> pretty
>>> > > significant build time and code size cost. We're better
>>> off
>>> > dealing
>>> > > with ABI incompatibility issues that come up in a less
>>> heavy
>>> > handed
>>> > > way.
>>> > >
>>> > > This reverts commit r189610.
>>> > >
>>> > > Modified:
>>> > > libcxx/trunk/include/__config
>>> > > libcxx/trunk/src/algorithm.cpp
>>> > > libcxx/trunk/src/ios.cpp
>>> > > libcxx/trunk/src/locale.cpp
>>> > > libcxx/trunk/src/string.cpp
>>> > > libcxx/trunk/src/valarray.cpp
>>> > >
>>> > > Modified: libcxx/trunk/include/__config
>>> > > URL:
>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/
>>> > __config?rev
>>> > > =215740&r1=215739&r2=215740&view=diff
>>> > >
>>> ================================================================
>>> > ==========
>>> > > ====
>>> > > --- libcxx/trunk/include/__config (original)
>>> > > +++ libcxx/trunk/include/__config Fri Aug 15 12:58:56
>>> 2014
>>> > > @@ -608,7 +608,7 @@ template <unsigned> struct
>>> __static_asse
>>> > > #endif
>>> > >
>>> > > #ifndef _LIBCPP_EXTERN_TEMPLATE
>>> > > -#define _LIBCPP_EXTERN_TEMPLATE(...)
>>> > > +#define _LIBCPP_EXTERN_TEMPLATE(...) extern template
>>> > __VA_ARGS__;
>>> > > #endif
>>> > >
>>> > > #ifndef _LIBCPP_EXTERN_TEMPLATE2
>>> > >
>>> > > Modified: libcxx/trunk/src/algorithm.cpp
>>> > > URL:
>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/
>>> > algorithm.cpp?
>>> > > rev=215740&r1=215739&r2=215740&view=diff
>>> > >
>>> ================================================================
>>> > ==========
>>> > > ====
>>> > > --- libcxx/trunk/src/algorithm.cpp (original)
>>> > > +++ libcxx/trunk/src/algorithm.cpp Fri Aug 15 12:58:56
>>> 2014
>>> > > @@ -7,7 +7,6 @@
>>> > > //
>>> > > //===
>>> > >
>>> >
>>> ----------------------------------------------------------------------
>>> > ===/
>>> > > /
>>> > >
>>> > > -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template
>>> > __VA_ARGS__;
>>> > > #include "algorithm"
>>> > > #include "random"
>>> > > #include "mutex"
>>> > >
>>> > > Modified: libcxx/trunk/src/ios.cpp
>>> > > URL:
>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/
>>> > ios.cpp?rev=
>>> > > 215740&r1=215739&r2=215740&view=diff
>>> > >
>>> ================================================================
>>> > ==========
>>> > > ====
>>> > > --- libcxx/trunk/src/ios.cpp (original)
>>> > > +++ libcxx/trunk/src/ios.cpp Fri Aug 15 12:58:56 2014
>>> > > @@ -7,8 +7,6 @@
>>> > > //
>>> > > //===
>>> > >
>>> >
>>> ----------------------------------------------------------------------
>>> > ===/
>>> > > /
>>> > >
>>> > > -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template
>>> > __VA_ARGS__;
>>> > > -
>>> > > #include "ios"
>>> > > #include "streambuf"
>>> > > #include "istream"
>>> > >
>>> > > Modified: libcxx/trunk/src/locale.cpp
>>> > > URL:
>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/
>>> > locale.cpp?rev=
>>> > > 215740&r1=215739&r2=215740&view=diff
>>> > >
>>> ================================================================
>>> > ==========
>>> > > ====
>>> > > --- libcxx/trunk/src/locale.cpp (original)
>>> > > +++ libcxx/trunk/src/locale.cpp Fri Aug 15 12:58:56 2014
>>> > > @@ -7,8 +7,6 @@
>>> > > //
>>> > > //===
>>> > >
>>> >
>>> ----------------------------------------------------------------------
>>> > ===/
>>> > > /
>>> > >
>>> > > -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template
>>> > __VA_ARGS__;
>>> > > -
>>> > > // On Solaris, we need to define something to make the
>>> C99
>>> > parts of
>>> > > localeconv
>>> > > // visible.
>>> > > #ifdef __sun__
>>> > >
>>> > > Modified: libcxx/trunk/src/string.cpp
>>> > > URL:
>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/
>>> > string.cpp?rev=
>>> > > 215740&r1=215739&r2=215740&view=diff
>>> > >
>>> ================================================================
>>> > ==========
>>> > > ====
>>> > > --- libcxx/trunk/src/string.cpp (original)
>>> > > +++ libcxx/trunk/src/string.cpp Fri Aug 15 12:58:56 2014
>>> > > @@ -7,8 +7,6 @@
>>> > > //
>>> > > //===
>>> > >
>>> >
>>> ----------------------------------------------------------------------
>>> > ===/
>>> > > /
>>> > >
>>> > > -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template
>>> > __VA_ARGS__;
>>> > > -
>>> > > #include "string"
>>> > > #include "cstdlib"
>>> > > #include "cwchar"
>>> > >
>>> > > Modified: libcxx/trunk/src/valarray.cpp
>>> > > URL:
>>> http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/
>>> > valarray.cpp?rev
>>> > > =215740&r1=215739&r2=215740&view=diff
>>> > >
>>> ================================================================
>>> > ==========
>>> > > ====
>>> > > --- libcxx/trunk/src/valarray.cpp (original)
>>> > > +++ libcxx/trunk/src/valarray.cpp Fri Aug 15 12:58:56
>>> 2014
>>> > > @@ -7,8 +7,6 @@
>>> > > //
>>> > > //===
>>> > >
>>> >
>>> ----------------------------------------------------------------------
>>> > ===/
>>> > > /
>>> > >
>>> > > -#define _LIBCPP_EXTERN_TEMPLATE(...) extern template
>>> > __VA_ARGS__;
>>> > > -
>>> > > #include "valarray"
>>> > >
>>> > > _LIBCPP_BEGIN_NAMESPACE_STD
>>> > >
>>> > > _______________________________________________
>>> > > cfe-commits mailing list
>>> > > cfe-commits at cs.uiuc.edu
>>> > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140817/868955ac/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: test_case.tar.gz
Type: application/x-gzip
Size: 653 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140817/868955ac/attachment.bin>
More information about the cfe-commits
mailing list