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

[Chandler Carruth]

On Fri, Jan 3, 2014 at 4:50 AM, Alp Toker <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.


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.

It also ensures that we don't grow accidental dependencies on functions
that were only ever intended to be used inside of assert() and in dump()
methods. Historically, this has happened quite a few times, and has taken
someone some time to track down a compile time performance regression and
separate out the functionality.

So, I would still prefer the pattern I suggested earlier. Also, this was
discussed previously on the llvm mailing list and reached much the same
conclusion.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140103/1e092796/attachment.html>