<div dir="ltr"><div>(As context, for me this is mostly about clang-format since I noticed AST and Sema code making it into its final binary, which I thought is a bit silly. This is also only about a few hundred kilobytes, so I'm happy with whatever outcome we arrive at. I consider this a minor cleanup and I'm not strongly arguing for anything.)</div>
<div><br></div><div>Chandler wrote:</div><div><div>> Linker dead stripping is only one of many ways dead stripping can and does occur. The used attribute is a much more stable way of ensuring that these debugging aids are available.</div>
<div><br></div><div>Oh? For example? (I'm curious, I didn't know this.)</div><div><br></div><div>> Similarly, I really want to be able to call dump methods from a Release+Asserts binary. The +Asserts means that the increased binary size shouldn't be a problem, and this can be a huge help when debugging bootstrap miscompiles and in-the-wild crashers.</div>
</div><div><br></div><div>You mostly diagnose crashes of clang in the wild, right? That's built without dead stripping anyways.</div><div><br></div><div><br></div><div>I tend to build everything in Release+Asserts, since I use trunk and want the internal sanity checks. I'm fine with the performance and size impact this has, but I don't agree that +Asserts necessarily implies "for debugging". (Again, I'm not strongly arguing for this, I'm just describing my perspective.)</div>
<div><br></div><div><br></div><div class="gmail_extra"><div class="gmail_quote">On Fri, Jan 3, 2014 at 5:09 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im"><br>
On 04/01/2014 01:03, Nico Weber wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Now that I played with this a bit, I realized that triggering this on NDEBUG means it's only stripped in -Asserts builds, independent of Debug and Release.<br>
<br>
How about something completely different: Let NO_DEAD_STRIP be 1 by default in debug builds, and remove used attribute completely? Then the dump functions won't be stripped in debug (since no stripping is done), and no special attribute is needed. (And the dump() functions get stripped in Release+Asserts mode too as long as nothing calls them.)<br>
</blockquote>
<br></div>
That's a smart solution, but the patch I send gets the job done more consistently by ensuring the symbols are always there in builds where we need them.<br></blockquote><div><br></div><div>What does "more consistently" mean?</div>
<div><br></div><div>It also means that they are always in +Assert builds, which I think is a minor bummer. (And it also requires additional source annotations.)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Is there anything controversial about the patch I posted at this point? It's already got my thumbs up...<br></blockquote><div><br></div><div>As posted upthread, the llvm part is already in ( <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131230/200338.html">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131230/200338.html</a> ) and I'm about to land the clang half. But when I measured the binary size win for the commit message, I noticed that there was none in +Asserts builds, hence me warming up this thread again :-)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Alp.<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
<br>
On Fri, Jan 3, 2014 at 2:59 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a> <mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>>> wrote:<br>
<br>
On Fri, Jan 3, 2014 at 2:11 PM, Chandler Carruth<br></div><div class="im">
<<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a> <mailto:<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>>> wrote:<br>
<br>
On Fri, Jan 3, 2014 at 4:08 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div><div class="im">
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
<br>
On 03/01/2014 19:41, Chandler Carruth wrote:<br>
<br>
On Fri, Jan 3, 2014 at 2:03 PM, Nico Weber<br>
<<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a> <mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>><br></div>
<mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a><div class="im"><br>
<mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>>>> wrote:<br>
<br>
On Fri, Jan 3, 2014 at 10:15 AM, Nico Weber<br>
<<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a> <mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>><br></div>
<mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a><div><div class="h5"><br>
<mailto:<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>>>> wrote:<br>
<br>
Ok, sounds like it's not clear if folks want<br>
to have dump<br>
methods behind #ifdef !NDEBUG. Maybe we don't<br>
have to resolve<br>
that part in this thread<br>
<br>
Is there any opposition to replacing the<br>
attributes on just<br>
the dump methods with LLVM_DUMP_METHOD? That's<br>
a subset of<br>
Chandler's change and I think also what Alp's<br>
diff is.<br>
<br>
…which is effectively Alp's patch I suppose. So<br>
maybe we could<br>
land that for now and then discuss whether to put<br>
dump() methods<br>
in !NDEBUG at some other point?<br>
<br>
<br>
I'd like to actually see the patch rather than talk in<br>
the hypothetical.<br>
<br>
<br>
The patch was in the original mail you replied to.<br>
<br>
<br>
Sorry, totally missed the attachments when ctatching up on<br>
this thread. Thanks.<br>
<br>
<br>
<br>
Maybe I misunderstood, but I thought it also moved the<br>
attribute to the definition? I don't understand why<br>
that's relevant yet (which is why maybe I've<br>
misunderstood what was happening).<br>
<br>
<br>
Explained in the original mail.<br>
<br>
<br>
The first time I read it I got the impression there was some<br>
other bug besides the general desire to avoid NDEBUG in<br>
headers. Re-reading it, it seems that was all? Have I<br>
understood it correctly this time?<br>
<br>
<br>
My reading of the spec for the attribute seems to<br>
indicate that there is no difference....<br>
<br>
<br>
If there's no spec difference between the declaration and<br>
definition forms, that's a good reason to avoid the form<br>
that's problematic and go with the simple patch in the<br>
original mail.<br>
<br>
<br>
Again, I got the impression there was something more than<br>
moving NDEBUG references out of headers, but it seems that<br>
impression was wrong.<br>
<br>
<br>
Anyways, I don't actually agree with the conclusion of the<br>
NDEBUG-in-headers discussion, but we can continue that<br>
discussion on its original thread. I've posted the primary<br>
reason why I don't see how to do this across the entire LLVM<br>
project in the thread.<br>
<br>
My real motivation here is simple:<br>
- I really like the helper functions used exclusively to<br>
support "dump" or "assert" (things we don't expect to<br>
generally ship in the released compiler) to be clearly<br>
separated from other functions. Even better if I get a<br>
compiler error when I misuse one from normal code.<br>
- I don't see any good way to do this if the only marking of<br>
dump methods are conditional "used" attributes. We don't want<br>
to similarly mark the helper functions (no need to call them<br>
from the debugger) and even if we did it wouldn't get us<br>
warnings when we mix them up.<br>
- If we put guards around the very declaration of these<br>
methods, then we'll get errors when we call helpers from<br>
non-assert contexts, and we'll get warnings when we fail to<br>
appropriately guard a method that is only used from assert<br>
contexts.<br>
- But that runs afoul of the desire to have NDEBUG-free<br>
headers. I disagree with that general direction, but lets<br>
discuss that on its thread. (Note, I don't think your current<br>
patches meaningfully harm the NDEBUG issue -- they are<br>
strictly ABI neutral.)<br>
<br>
Regarding your existing patch as an incremental step, I have<br>
some comments:<br>
<br>
- I suspect these should be controlled by !NDEBUG ||<br>
LLVM_ENABLE_DUMP<br>
<br>
<br>
The macro is controlled by this already.<br>
<br>
- I don't know that "DEBUG" is the right word here, because of<br>
the ability to control them with the LLVM_ENABLE_DUMP build<br>
configuration option<br>
<br>
<br>
Went with your name suggestion.<br>
<br>
- You don't remove NDEBUG || LLVM_ENABLE_DUMP complexity from<br>
any of the dump methods in LLVM... or apply this attribute to<br>
any of the existing dump methods that are missing it<br>
currently. I'm not sure we should, because I like the<br>
compile-away removal mechanism more than the link-away as<br>
outlined above. =/<br>
<br>
<br>
This can be done consistently one way or another after the<br>
discussion on the other thread is done.<br>
<br>
- Why is noinline kept in the !NDEBUG case? Is that just for<br>
ABI neutrality?<br>
<br>
<br>
Kept it for now.<br>
<br>
- Won't this cause warnings for unused private methods in<br>
!NDEBUG builds when the dump methods are not marked used? If<br>
we leave the methods in place, it would seem natural to mark<br>
them as unused in the !NDEBUG case to avoid this.<br>
<br>
<br>
All dump methods in llvm and clang with these attributes happen to<br>
be public at the moment.<br>
<br>
Since we all seem to be in violent agreement about the basic<br>
approach, I went ahead and checked in the llvm bits in r198456.<br>
I'll do the s/LLVM_ATTRIBUTE_USED/LLVM_<u></u>DUMP_METHOD/ bit in clang<br>
later today, and we can decide if we want to move the attributes<br>
from declaration to definition in a follow-up.<br>
<br>
Nico<br>
<br>
</div></div></blockquote><div class=""><div class="h5">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</div></div></blockquote></div><br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div></div>