[libcxx] r215740 - Revert "Turn off extern templates for most uses."
Eric Fiselier
eric at efcs.ca
Sun Aug 17 13:38:02 PDT 2014
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/f26ff23f/attachment.html>
More information about the cfe-commits
mailing list