> On 2016-Jan-22, at 18:01, Eric Fiselier <e...@efcs.ca> wrote: > > > > On Fri, Jan 22, 2016 at 10:44 AM, Duncan P. N. Exon Smith > <dexonsm...@apple.com> wrote: > > > 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? > > 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.
Yes. > @Duncan What are your main concerns about adding a new global variable and > forcing the linker to coalesce it? The only concern is: this *is* an addition to the ABI, and it requires that the function doesn't change going forward. I'm not particularly concerned though. > > > > 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. Yup, doesn't work. From your previous mail I had misunderstood that you didn't expect this to work for dylib symbols. > > 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 don't see this as a major downside. Do users really have the expectation of using _LIBCPP_DEBUG with old versions of the library? At any rate, I don't think we support that on Darwin. Do other platforms? > > 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
