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

[Nico Weber]

On Fri, Jan 3, 2014 at 2:11 PM, Chandler Carruth <chandlerc at google.com>wrote:

> On Fri, Jan 3, 2014 at 4:08 PM, Alp Toker <alp at nuanti.com> wrote:
>
>>
>> On 03/01/2014 19:41, Chandler Carruth wrote:
>>
>>> On Fri, Jan 3, 2014 at 2:03 PM, Nico Weber <thakis at chromium.org <mailto:
>>> thakis at chromium.org>> wrote:
>>>
>>>     On Fri, Jan 3, 2014 at 10:15 AM, Nico Weber <thakis at chromium.org
>>>     <mailto: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.
>>>
>>>     …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?
>>>
>>>
>>> I'd like to actually see the patch rather than talk in the hypothetical.
>>>
>>
>> The patch was in the original mail you replied to.
>
>
> Sorry, totally missed the attachments when ctatching up on this thread.
> Thanks.
>
>
>>
>>
>>  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).
>>>
>>
>> Explained in the original mail.
>
>
> 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?
>
>
>>
>>  My reading of the spec for the attribute seems to indicate that there is
>>> no difference....
>>>
>>>
>> 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.
>
>
> Again, I got the impression there was something more than moving NDEBUG
> references out of headers, but it seems that impression was wrong.
>
>
> 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.
>
> My real motivation here is simple:
> - 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.
> - 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.
> - 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.
> - 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.)
>
> Regarding your existing patch as an incremental step, I have some comments:
>
> - I suspect these should be controlled by !NDEBUG || LLVM_ENABLE_DUMP
>

The macro is controlled by this already.


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

Went with your name suggestion.


> - 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. =/
>

This can be done consistently one way or another after the discussion on
the other thread is done.


> - Why is noinline kept in the !NDEBUG case? Is that just for ABI
> neutrality?
>

Kept it for now.


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

All dump methods in llvm and clang with these attributes happen to be
public at the moment.

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.

Nico
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140103/7a8db284/attachment.html>