[PATCH] D12354: [libcxx] Add global assertion handler for debug mode.

[Eric Fiselier via cfe-commits]

On Fri, Jan 22, 2016 at 10:44 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2016-Jan-21, at 22:22, Eric Fiselier <eric at efcs.ca> wrote:
> >
> >
> >
> > On Thu, Jan 21, 2016 at 10:35 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >
> > > On 2016-Jan-21, at 17:59, Eric Fiselier <eric at efcs.ca> wrote:
> > >
> > > EricWF added a comment.
> > >
> > > In http://reviews.llvm.org/D12354#331776, @dexonsmith wrote:
> > >
> > >> This patch looks correct to me.  Is there any reason it wasn't
> committed?
> > >
> > >
> > > I was concerned about using a function-local static in the library
> headers, I don't think libc++ does that anywhere else and I wanted to make
> sure it was safe to do.
> > >
> > >
> > > http://reviews.llvm.org/D12354
> >
> > Ah, in the testing hook.  I feel like it's better than the current
> > situation, anyway.  Unless we have ABI guarantees for _LIBCPP_DEBUG
> > since this could lock us in somehow.  (Do we?  Should we?)
> >
> > The _LIBCPP_DEBUG mode should produce the same ABI as non-debug builds.
> > However I don't see how this patch would "lock us in" or change our ABI
> guarantees.
> > The function local static should have vague linkage and be emitted in
> every TU where it is ODR used.
> > TU's compiled with debug mode off will not contain or depend on the
> symbol.
> >
> > The libc++.dylib will contain it's own copy of the function local static
> since _LIBCPP_ASSERT
>
> If so, then the hook (as intended) doesn't actually allow testing
> the _LIBCPP_ASSERT()s in src/debug.cpp, unless I'm missing something?
>

Your right below. libc++.dylib will have it's own copy but the linker will
coalesce it
with the version emitted in the headers. So it will allow testing of
debug.cpp


>
> In that case, why not just do (2) and not worry about a global variable
> at all?  I.e., in test/support/debug.hpp, something like:
> --
> #define _LIBCPP_ASSERT(c, m) (c ? (void)0 : handleAssertion(m, __FILE__,
> ...))
> #include <__debug>
> --
>
> > Do you disagree? Do you have any concerns about this?
>
> As written, shouldn't the runtime linker coalesce these ODR functions
> (and the function-local static) with the version in the headers?  This
> isn't going to have hidden visibility AFAICT (maybe I missed something).
>


I agree with that 100%. Sorry if I was unclear. It's important that
the function has default visibility so that versions in shared libraries
are properly coalesced.


>
> > I want to be 100% sure about the ABI implications.
>
> Because these functions are inline (and not always_inline, or
> internal_linkage, or whatever the new thing is), ODR requires that every
> source file is compiled with the exact same version of them.  I think
> this affects the ABI.
>

I don't think ODR is a concern here. The assertion handler function has the
same definition in every TU where it is ODR used.
It's unlikely that the handler function body will need to change any time
soon. Can you clarify on how ODR affects the ABI?

The functions do have external "linkonce_odr" linkage and will be emited in
new libc++.dylib builds as weak symbols on ELF.
AFAIK it should be a ABI safe change to add/remove weak symbols on Linux.
For example we already get entirely different sets of weak symbols in
libc++.so between debug and release builds on linux.

I'm not sure how Darwin handles comdat linkonce_odr symbols but hopefully
it's semantically similar to linux.
@Duncan What are your main concerns about adding a new global variable and
forcing the linker to coalesce it?


> > Thinking through a couple of other options:
> >
> >  2. The test could provide its own version of _LIBCPP_ASSERT.  It's
> >     not obvious how to hook into the assertions in src/debug.cpp
> >     though.
> >
> >
> > This could work, but it feels more like a hack than a fix.
>
> IMO, it's no more hacky than adding function-locals.  The point of
> this part of the patch is to test calls to _LIBCPP_ASSERT, but
> approach (1) affects uses in non-trivial ways (adding global variables
> that need to be coalesced between all the affected TUs).  At least (2)
> limits the hook to the testing framework itself.
>


I think there will be a need to override _LIBCPP_ASSERT calls in the dylib
so I'm hesitant to use this approach.

1. We need to override _LIBCPP_ASSERT in src/debug.cpp in order to test a
lot of debug mode in the container library.

2. I would like to use _LIBCPP_ASSERT and the assertion handler to report
errors in src/mutex.cpp. Currently std::mutex uses "assert(ret == 0)" to
check that the pthread system calls succeed.
    Currently the user has no way to get the value of "ret" in the case of
failure. Using _LIBCPP_ASSERT will allow us to propagate "ret" to the
caller if they replace the assertion handler.
    (See https://llvm.org/bugs/show_bug.cgi?id=25982).


> >
> >  3. We could move __debug_assert() and __debug_assertion_handler()
> >     to the dylib (and use a static global variable there).
> >
> > This would be the *cleanest* solution but is actually the worst option
> IMHO.
> > We can't safely use the new dylib symbol in the headers without breaking
> ABI compatibility for older/system dylibs.
> > I think I'll try and move the symbol into the dylib for ABI v2 though.
> >
> > Maybe (3) is the best.  Is it important for these functions to be
> > inline, given that they're in the slow path anyway?
> >
> > The "inline" has more to do with linkage and ABI concerns. _LIBCPP_DEBUG
> isn't meant to be a "fast" path.
>
> I don't think making it inline really simplifies the ABI story
> though.
>

(1) results in a libc++.dylib which is backwards compatible with older
versions. A program compiled against a ToT libc++.dylib can still link to
older dylibs as well.
     The assertion handler symbols in the dylib should always be overridden
by the version emitted in "a.out". This means that "a.out" will continue to
work with dylib versions that don't provide the symbols.

(4) breaks compatibility with older dylibs. A program compiled against ToT
libc++ will fail to link with older dylib versions.


> >
> > I have a 4th possible option:
> >
> > 4. Make __debug_assertion_handler a weak symbol. By default it would not
> be defined, but users and tests could override it.
> > __debug_assert will use "__debug_assertion_handler" if it's defined,
> otherwise it will just abort.
> > However weak symbols are not a part of the language while function local
> statics are. I'm also concerned that weak symbols are not portable.
>
> Yeah, I had a similar thought and rejected it for the same reason.
> Portability probably matters here.
>
> > I've reached out to the libstdc++ maintainer about the ABI
> considerations of using function local statics. If I get a positive
> response from him I'm going to move ahead with plan #1.
> >
> > /Eric
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160122/e44d5150/attachment.html>