<div dir="ltr"><div>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.</div><div><br></div><div>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.)</div>
<div><br></div>On Fri, Jan 3, 2014 at 2:59 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><div class="h5">On Fri, Jan 3, 2014 at 2:11 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.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 dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>On Fri, Jan 3, 2014 at 4:08 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">
<br>
On 03/01/2014 19:41, Chandler Carruth 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>
On Fri, Jan 3, 2014 at 2:03 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 10:15 AM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a><br></div><div>
<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 to have dump<br>
methods behind #ifdef !NDEBUG. Maybe we don't have to resolve<br>
that part in this thread<br>
<br>
Is there any opposition to replacing the attributes on just<br>
the dump methods with LLVM_DUMP_METHOD? That's a subset of<br>
Chandler's change and I think also what Alp's diff is.<br>
<br>
…which is effectively Alp's patch I suppose. So maybe we could<br>
land that for now and then discuss whether to put dump() methods<br>
in !NDEBUG at some other point?<br>
<br>
<br>
I'd like to actually see the patch rather than talk in the hypothetical.<br>
</div></blockquote>
<br>
The patch was in the original mail you replied to.</blockquote><div><br></div></div><div>Sorry, totally missed the attachments when ctatching up on this thread. Thanks.</div><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">
<div><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">
Maybe I misunderstood, but I thought it also moved the attribute to the definition? I don't understand why that's relevant yet (which is why maybe I've misunderstood what was happening).<br>
</blockquote>
<br></div>
Explained in the original mail.</blockquote><div><br></div></div><div>The first time I read it I got the impression there was some other bug besides the general desire to avoid NDEBUG in headers. Re-reading it, it seems that was all? Have I understood it correctly this time? <br>
</div><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"><div><br></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">
My reading of the spec for the attribute seems to indicate that there is no difference....<br>
<br>
</blockquote>
<br></div>
If there's no spec difference between the declaration and definition forms, that's a good reason to avoid the form that's problematic and go with the simple patch in the original mail.</blockquote><div><br></div>
</div><div>Again, I got the impression there was something more than moving NDEBUG references out of headers, but it seems that impression was wrong.</div><div><br></div><div><br></div><div>Anyways, I don't actually agree with the conclusion of the NDEBUG-in-headers discussion, but we can continue that discussion on its original thread. I've posted the primary reason why I don't see how to do this across the entire LLVM project in the thread.</div>
<div><br></div><div>My real motivation here is simple:</div><div>- I really like the helper functions used exclusively to support "dump" or "assert" (things we don't expect to generally ship in the released compiler) to be clearly separated from other functions. Even better if I get a compiler error when I misuse one from normal code.</div>
<div>- I don't see any good way to do this if the only marking of dump methods are conditional "used" attributes. We don't want to similarly mark the helper functions (no need to call them from the debugger) and even if we did it wouldn't get us warnings when we mix them up.</div>
<div>- If we put guards around the very declaration of these methods, then we'll get errors when we call helpers from non-assert contexts, and we'll get warnings when we fail to appropriately guard a method that is only used from assert contexts.</div>
<div>- But that runs afoul of the desire to have NDEBUG-free headers. I disagree with that general direction, but lets discuss that on its thread. (Note, I don't think your current patches meaningfully harm the NDEBUG issue -- they are strictly ABI neutral.)</div>
<div><br></div><div>Regarding your existing patch as an incremental step, I have some comments:</div><div><br></div><div>- I suspect these should be controlled by !NDEBUG || LLVM_ENABLE_DUMP</div></div></div></div></blockquote>
<div><br></div></div></div><div>The macro is controlled by this already.</div><div class="im"><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>- I don't know that "DEBUG" is the right word here, because of the ability to control them with the LLVM_ENABLE_DUMP build configuration option</div>
</div></div></div></blockquote><div><br></div></div><div>Went with your name suggestion.</div><div class="im"><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>- You don't remove NDEBUG || LLVM_ENABLE_DUMP complexity from any of the dump methods in LLVM... or apply this attribute to any of the existing dump methods that are missing it currently. I'm not sure we should, because I like the compile-away removal mechanism more than the link-away as outlined above. =/</div>
</div></div></div></blockquote><div><br></div></div><div>This can be done consistently one way or another after the discussion on the other thread is done.</div><div class="im"><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>- Why is noinline kept in the !NDEBUG case? Is that just for ABI neutrality?</div></div></div></div></blockquote><div><br></div></div><div>Kept it for now.</div><div class="im"><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">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>- Won't this cause warnings for unused private methods in !NDEBUG builds when the dump methods are not marked used? If we leave the methods in place, it would seem natural to mark them as unused in the !NDEBUG case to avoid this.</div>
</div></div></div>
</blockquote></div></div><br></div><div class="gmail_extra">All dump methods in llvm and clang with these attributes happen to be public at the moment.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Since we all seem to be in violent agreement about the basic approach, I went ahead and checked in the llvm bits in r198456. I'll do the s/LLVM_ATTRIBUTE_USED/LLVM_DUMP_METHOD/ bit in clang later today, and we can decide if we want to move the attributes from declaration to definition in a follow-up.</div>
<span class=""><font color="#888888">
<div class="gmail_extra"><br></div><div class="gmail_extra">Nico</div></font></span></div></blockquote></div></div></div>