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

[Chandler Carruth]

On Fri, Jan 3, 2014 at 12:43 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>wrote:

> On Jan 3, 2014, at 2:08 AM, Chandler Carruth <chandlerc at google.com> wrote:
>
> >
> > 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.
>
> The benefit of relying on the linker is that a project can easily use any
> particular dump/print function from the llvm/clang libraries that it sees
> fit (e.g. for logging purposes) and since the linker has the ultimate
> knowledge of what gets used or not, it "just works" without unnecessary
> code-bloat.
> For example, a project may just want to use "Decl::dump(raw_ostream &OS)"
> for logging; with your scheme it'd need to configure & build specially for
> this and accept the code bloat from _all_ dump functions across llvm/clang,
> just to use this one function.
>
> But we don't need to hypothesize, we can see a similar example in clang
> with '-ast-dump'.
> Do you really want to disable this option on release builds ? I would much
> prefer not, I've used it frequently enough and I'd like to continue to be
> able to use it from our release builds.
>

Sorry, terminology is getting a bit hard here. I wasn't at all referring to
the functionality of -ast-dump, or methods such as the one you mention
which might legitimately be part of the C++ API that a particular type
intends to expose. The AST is a great example here -- clearly it should
provide ways for library users to print it out in the same nice textual
form as used by -ast-dump.

I'm referring to the more narrow use of the term "dump method" which is
specifically a method that accepts no arguments and prints as much state to
stderr as might conceivably be useful from inside gdb. Decl::dump() is just
such a function, but Decl::dump(raw_ostream &OS) is different. The somewhat
consistent pattern in LLVM is to name the latter functions 'print' to avoid
confusion, but that's neither here nor there.

My stance is essentially that the things which are only used either a) in
GDB or b) inside of !NDEBUG regions (be they assert()s or the LLVM DEBUG
macro) are the things which should be compiled away when NDEBUG (or the
opt-in macro that several folks asked for in LLVM-land to get dump methods
in no-asserts builds) and should be marked as noinline and used to make
them easy to reach via the debugger.

When these routines can be implemented using the existing useful APIs like
in the case of Decl, awesome. In other cases, there are no real users of
the printing infrastructure and so all of it can be hidden in this way.


> >
> > 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.
>
> Could you elaborate with a specific example ? Was someone ever printing
> something by accident ?
> I think this could apply to invariant-checking functions, but for
> dump/print functionality I honestly don't think that there is much "danger"
> in practice.
>

It was a traversal function. Specifically, it looked like a nice constant
time query function, but actually did linear work under the hood. That was
totally fine (and better than the complexity of other alternatives) when it
was used as a helper to implement dump functionality. But it got re-used a
few times, and ended up inside of an existing linear algorithm and caused
us to have N^2 explosions every now and then.

Still, not the end of the world. It required a bug, some test cases, and
some easy time with the profiler. But when it was suggested to hide all of
the helper functions the same way as the dump methods in LLVM, making these
compile errors was a really nice result.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140103/3843f8a7/attachment.html>