> On 2016-Jan-21, at 22:22, Eric Fiselier <e...@efcs.ca> wrote: > > > > On Thu, Jan 21, 2016 at 10:35 PM, Duncan P. N. Exon Smith > <dexonsm...@apple.com> wrote: > > > On 2016-Jan-21, at 17:59, Eric Fiselier <e...@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 > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
