<div dir="ltr">On Fri, Jan 3, 2014 at 10:15 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Ok, sounds like it's not clear if folks want to have dump methods behind #ifdef !NDEBUG. Maybe we don't have to resolve that part in this thread<div>
<br></div><div>Is there any opposition to replacing the attributes on just the dump methods with LLVM_DUMP_METHOD? That's a subset of Chandler's change and I think also what Alp's diff is.</div>
<div><br></div><div>(I don't understand the "This is a problem with 3.5's goal of not using NDEBUG in headers" – I read the thread about NDEBUG [1], and it seems to be mostly about abi compat – and a) clang itself isn't deadstripped itself because plugins b) if stuff in a core binary itself calls a dump method it won't be stripped anyhows c) the thread at [1] sounded more about abi stuff like mvars and virtual functions, there's no clear support for the assert stuff as far as I can tell. So just varying the presence or absence of attribute((used)) based on NDEBUG sounds fine to me.)</div>
</div></blockquote><div><br></div><div>…which is effectively Alp's patch I suppose. So maybe we could land that for now and then discuss whether to put dump() methods in !NDEBUG at some other point?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div><br></div><div>[1]: <a href="https://groups.google.com/forum/#!msg/llvm-dev/OvcDArJjrSw/teSzTMugoGcJ" target="_blank">https://groups.google.com/forum/#!msg/llvm-dev/OvcDArJjrSw/teSzTMugoGcJ</a><div><div class="h5"><br>
<div class="gmail_extra"><br><br><div class="gmail_quote">
On Fri, Jan 3, 2014 at 9:43 AM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:akyrtzi@gmail.com" target="_blank">akyrtzi@gmail.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>On Jan 3, 2014, at 2:08 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:<br>
<br>
><br>
> That said, I dislike this approach. I prefer actually *removing* the functions in non-asserts builds because then we will get early warnings if we miss some, and we don't have to rely on linker dead stripping to remove all of them.<br>


<br>
</div>The benefit of relying on the linker is that a project can easily use any particular dump/print function from the llvm/clang libraries that it sees fit (e.g. for logging purposes) and since the linker has the ultimate knowledge of what gets used or not, it "just works" without unnecessary code-bloat.<br>


For example, a project may just want to use "Decl::dump(raw_ostream &OS)" for logging; with your scheme it'd need to configure & build specially for this and accept the code bloat from _all_ dump functions across llvm/clang, just to use this one function.<br>


<br>
But we don't need to hypothesize, we can see a similar example in clang with '-ast-dump'.<br>
Do you really want to disable this option on release builds ? I would much prefer not, I've used it frequently enough and I'd like to continue to be able to use it from our release builds.<br>
<div><br>
><br>
> It also ensures that we don't grow accidental dependencies on functions that were only ever intended to be used inside of assert() and in dump() methods. Historically, this has happened quite a few times, and has taken someone some time to track down a compile time performance regression and separate out the functionality.<br>


<br>
</div>Could you elaborate with a specific example ? Was someone ever printing something by accident ?<br>
I think this could apply to invariant-checking functions, but for dump/print functionality I honestly don't think that there is much "danger" in practice.<br>
<br>
</blockquote></div><br></div></div></div></div></div>
</blockquote></div><br></div></div>