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

[Nico Weber]

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.

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

On Fri, Jan 3, 2014 at 2:59 PM, Nico Weber <thakis at chromium.org> wrote:

> 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/731105f2/attachment.html>