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

[Eric Fiselier via cfe-commits]

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

Do you disagree? Do you have any concerns about this?
I want to be 100% sure about the ABI implications.


>
> 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.


>  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 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.


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/20160121/f5958081/attachment.html>