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

[Alp Toker]

Hi Nico,

The current macro definition on the LLVM side looks fine to me because 
it's consistent with past dump() method ifdefs, particularly the 
existing convention followed in core LLVM code.

If however it needs tweaking, the change wouldn't affect the clang side 
anyway so I've gone ahead landed the follow up that makes use of the new 
macro in r198489.

If there's any major feature request or proposal to remove/change the 
way dump() functions are declared, that really needs to be a separate 
discussion at this point.

Thanks for holding on

Alp.



On 04/01/2014 04:11, Nico Weber wrote:
> On Fri, Jan 3, 2014 at 5:35 PM, Chandler Carruth <chandlerc at google.com 
> <mailto:chandlerc at google.com>> wrote:
>
>
>     On Fri, Jan 3, 2014 at 8:19 PM, Nico Weber <thakis at chromium.org
>     <mailto:thakis at chromium.org>> wrote:
>
>         (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.)
>
>
>     LTO, archive member dropping (hopefully not relevant here), and
>     some linkers that do minimal dead stripping by default.
>
>         > 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.
>
>
>     Not exclusively, no.
>
>         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.)
>
>
>     Sure. We could completely separate out the "asserts" knob from the
>     "dumping" knob. We already enable turning on dumping without
>     turning on asserts. Historically, asserts have carried much more
>     binary size and other impact than the dumping methods. There may
>     be a library layering issue if you're seeing dump methods drag in
>     large amounts of binary size into clang-format.
>
>     My preference is for fewer knobs in general, but if there really
>     are all four states here (+asserts +dumping, -asserts -dumping,
>     +asserts -dumping, -asserts +dumping) then we can support that I
>     guess. I just don't know of any real demand.
>
>
> I don't think this is worth adding another knob for.
>
>
> Not doing dead stripping in debug builds is something that might make 
> sense anyway though (if anyone disagrees, please let me know!), and 
> once that's done maybe not all dump methods would need to be marked 
> used in release builds (only the ones that folks consider so useful 
> that they need them in their release+asserts builds in addition to in 
> debug builds)?

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