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

[Alp Toker]

On 04/01/2014 01:03, Nico Weber wrote:
> 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.)

That's a smart solution, but the patch I send gets the job done more 
consistently by ensuring the symbols are always there in builds where we 
need them.

Is there anything controversial about the patch I posted at this point? 
It's already got my thumbs up...

Alp.



>
> On Fri, Jan 3, 2014 at 2:59 PM, Nico Weber <thakis at chromium.org 
> <mailto:thakis at chromium.org>> wrote:
>
>     On Fri, Jan 3, 2014 at 2:11 PM, Chandler Carruth
>     <chandlerc at google.com <mailto:chandlerc at google.com>> wrote:
>
>         On Fri, Jan 3, 2014 at 4:08 PM, Alp Toker <alp at nuanti.com
>         <mailto: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>
>                 <mailto: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>
>                     <mailto: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
>

-- 
http://www.nuanti.com
the browser experts