[cfe-dev] Should dump methods be LLVM_ATTRIBUTE_USED only in debug builds?

[Nico Weber]

On Fri, Jan 3, 2014 at 10:15 AM, Nico Weber <thakis at chromium.org> wrote:

> 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
>
> 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.
>
> (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.)
>

…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?


>
> [1]:
> https://groups.google.com/forum/#!msg/llvm-dev/OvcDArJjrSw/teSzTMugoGcJ
>
>
>
> On Fri, Jan 3, 2014 at 9:43 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com>wrote:
>
>> On Jan 3, 2014, at 2:08 AM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>>
>> >
>> > 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.
>>
>> 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.
>> 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.
>>
>> But we don't need to hypothesize, we can see a similar example in clang
>> with '-ast-dump'.
>> 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.
>>
>> >
>> > 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.
>>
>> Could you elaborate with a specific example ? Was someone ever printing
>> something by accident ?
>> 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.
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140103/437676d5/attachment.html>