[cfe-dev] Should dump methods be LLVM_ATTRIBUTE_USED only in debug builds?
Nico Weber
thakis at chromium.org
Fri Jan 3 17:19:50 PST 2014
(As context, for me this is mostly about clang-format since I noticed AST
and Sema code making it into its final binary, which I thought is a bit
silly. This is also only about a few hundred kilobytes, so I'm happy with
whatever outcome we arrive at. I consider this a minor cleanup and I'm not
strongly arguing for anything.)
Chandler wrote:
> Linker dead stripping is only one of many ways dead stripping can and
does occur. The used attribute is a much more stable way of ensuring that
these debugging aids are available.
Oh? For example? (I'm curious, I didn't know this.)
> Similarly, I really want to be able to call dump methods from a
Release+Asserts binary. The +Asserts means that the increased binary size
shouldn't be a problem, and this can be a huge help when debugging
bootstrap miscompiles and in-the-wild crashers.
You mostly diagnose crashes of clang in the wild, right? That's built
without dead stripping anyways.
I tend to build everything in Release+Asserts, since I use trunk and want
the internal sanity checks. I'm fine with the performance and size impact
this has, but I don't agree that +Asserts necessarily implies "for
debugging". (Again, I'm not strongly arguing for this, I'm just describing
my perspective.)
On Fri, Jan 3, 2014 at 5:09 PM, Alp Toker <alp at nuanti.com> wrote:
>
> 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.
>
What does "more consistently" mean?
It also means that they are always in +Assert builds, which I think is a
minor bummer. (And it also requires additional source annotations.)
> Is there anything controversial about the patch I posted at this point?
> It's already got my thumbs up...
>
As posted upthread, the llvm part is already in (
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131230/200338.html)
and I'm about to land the clang half. But when I measured the binary
size
win for the commit message, I noticed that there was none in +Asserts
builds, hence me warming up this thread again :-)
>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140103/13e93ee4/attachment.html>
More information about the cfe-dev
mailing list