<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 21, 2016 at 10:35 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2016-Jan-21, at 17:59, Eric Fiselier <<a href="mailto:eric@efcs.ca">eric@efcs.ca</a>> wrote:<br>
><br>
> EricWF added a comment.<br>
><br>
> In <a href="http://reviews.llvm.org/D12354#331776" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12354#331776</a>, @dexonsmith wrote:<br>
><br>
>> This patch looks correct to me. Is there any reason it wasn't committed?<br>
><br>
><br>
> 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.<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D12354" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12354</a><br>
<br>
</div></div>Ah, in the testing hook. I feel like it's better than the current<br>
situation, anyway. Unless we have ABI guarantees for _LIBCPP_DEBUG<br>
since this could lock us in somehow. (Do we? Should we?)<br></blockquote><div><br></div><div>The _LIBCPP_DEBUG mode should produce the same ABI as non-debug builds.</div><div>However I don't see how this patch would "lock us in" or change our ABI guarantees.</div><div>The function local static should have vague linkage and be emitted in every TU where it is ODR used.</div><div>TU's compiled with debug mode off will not contain or depend on the symbol.</div><div><br></div><div>The libc++.dylib will contain it's own copy of the function local static since _LIBCPP_ASSERT </div><div><br></div><div>Do you disagree? Do you have any concerns about this?</div><div>I want to be 100% sure about the ABI implications.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thinking through a couple of other options:<br>
<br>
2. The test could provide its own version of _LIBCPP_ASSERT. It's<br>
not obvious how to hook into the assertions in src/debug.cpp<br>
though.<br>
<br></blockquote><div><br></div><div>This could work, but it feels more like a hack than a fix. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3. We could move __debug_assert() and __debug_assertion_handler()<br>
to the dylib (and use a static global variable there).<br></blockquote><div><br></div><div>This would be the *cleanest* solution but is actually the worst option IMHO.</div><div>We can't safely use the new dylib symbol in the headers without breaking ABI compatibility for older/system dylibs.</div><div>I think I'll try and move the symbol into the dylib for ABI v2 though. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Maybe (3) is the best. Is it important for these functions to be<br>
inline, given that they're in the slow path anyway?</blockquote><div><br></div><div>The "inline" has more to do with linkage and ABI concerns. _LIBCPP_DEBUG isn't meant to be a "fast" path.</div><div><br></div><div>I have a 4th possible option:</div><div><br></div><div>4. Make __debug_assertion_handler a weak symbol. By default it would not be defined, but users and tests could override it.</div><div>__debug_assert will use "__debug_assertion_handler" if it's defined, otherwise it will just abort.</div><div>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.</div><div><br></div><div><br></div><div>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.</div><div><br></div><div>/Eric</div></div><br></div></div>