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

[Chandler Carruth]

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
- 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
- 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. =/
- Why is noinline kept in the !NDEBUG case? Is that just for ABI neutrality?
- 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140103/d4d15368/attachment.html>