[libcxx] r215740 - Revert "Turn off extern templates for most uses."
Chandler Carruth
chandlerc at google.com
Sun Aug 17 01:39:06 PDT 2014
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/875c30c5/attachment.html>
More information about the cfe-commits
mailing list