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

[Duncan P. N. Exon Smith via cfe-commits]

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

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

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

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

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