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

[Alp Toker]

On 03/01/2014 10:08, Chandler Carruth wrote:
>
> On Fri, Jan 3, 2014 at 4:50 AM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>     HI Nico,
>
>     I've attached the fix we had for this in our product branch. As
>     far as I can tell the other posts on this thread are missing the
>     problem and just patching some of the symptoms.
>
>     The bug was that attribute 'used' was applied in public headers
>     instead of the function definitions, causing missed dead stripping
>     opportunities that were further compounded by inlining.
>
>
> I'm not sure why this is (necessarily) a bug? I can hypothesize 
> instances where it is a bug, but maybe you can explain the particular 
> pattern you're imagining?
>
>     The commit message says it resolves two issues, "Missing dump()
>     symbols embedding LLVM SDK" and "Debug functions included in
>     release" so I'm guessing your use case is covered:
>
>     Only link dump() functions in debug builds
>
>     LLVM_DEBUG_HELPER marks debug functions definitions as 'used' when
>     !NDEBUG or
>     LLVM_ENABLE_DUMP is defined, otherwise leaves them to be dead
>     stripped by the
>     linker.
>
>     The macro also unconditionally marks the functions 'noinline' to
>     (1) ensure
>     they're available to debuggers in debug builds and (2) assist
>     linker dead
>     stripping in release builds.
>
>     The patches haven't yet been pushed upstream because they
>     introduce an NDEBUG going against the LLVM 3.5 library-friendly
>     headers goal.
>
>
> I still don't understand or agree with trying to preclude the use of 
> NDEBUG in header files. While theoretically it would be nice for 
> NDEBUG to not be an ABI break for LLVM, that comes at a high cost and 
> I've not seen a compelling argument for paying that cost.

I'm not sure what the cost is that you're talking about.

clang has been free of NDEBUG in headers since r196739, and the commits 
were a neat reduction in complexity touching no more than a couple dozen 
lines of code. If you remember the original patch was thousands of lines 
it's remarkable how light this ended up being.

The plugin API can now be used independently of configuration with one 
last pending patch to config.h.in on the LLVM side. That's a massive 
step forward for getting documented and supported features usable in the 
real world.

Before this, you had to rebuild everything, even third party plugins, 
just to debug an LLVM issue and track down problems. That's not 
practical -- even if you have a build farm, sometimes you just don't 
have all plugins and their dependencies and original build environments 
available.

So, It's great if you yourself just use LLVM / clang internally and 
don't have to deal with end users, embedding and plugins but these are 
all supported clang use cases that were broken previously and now more 
or less fixed.

I don't use the Makefile build system or the Apple buildit script but I 
appreciate others use them and even try to help out with their 
maintenance from time to time, certainly never blocking the people who 
actually do the work in areas I'm not active in :-)

Besides which, the whole debate seems moot because I actually suggested 
introducing carefully considered NDEBUG violation that temporarily goes 
against the 3.5 goal.

> That said, I dislike this approach. I prefer actually *removing* the 
> functions in non-asserts builds because then we will get early 
> warnings if we miss some, and we don't have to rely on linker dead 
> stripping to remove all of them.

That's very intrusive and goes beyond the scope of Nico's original problem.

The patches I've posted are incremental, solve the original bug by 
moving attributes from declaration to definition which was just a bug. 
They lower the line count and binary size and eliminate ifdef tangle 
without changing the way everyone uses dump() functions day to day.

That sets the bar pretty high for an alternative solution.

Alp.

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