[Analyzer] Fix clang::ento::taint::dumpTaint definition

[mantognini]

Summary

Ensure the definition is in the "taint" namespace, like its declaration.

[mantognini]

I'd appreciate reviews for this small patch. Thanks.

[steakhal]

Could you please elaborate whats the motivation for this change?

[mantognini]

In D124462#3476581, @steakhal wrote:

Could you please elaborate whats the motivation for this change?

Right, I could have been more explicit. Sorry about that.

dumpTaint is declared inside the taint namespace in the header file, hence why I'm adding taint::. I believe LLVM_DUMP_METHOD should also be applied to the function definition too.

[steakhal]

Although gcc (and probably clang too) allows specifying __attribute__((noinline)) at any declaration (by merging compatible attributes), I would prefer not to repeat ourselves.
The attribute must be present at the header to force all usages not to inline it, hence I would rather drop such attributes from the definition files.

Aside from that, looks good. Please update the summary of this patch accordingly.

clang/lib/StaticAnalyzer/Checkers/Taint.cpp

40

[mantognini]

In D124462#3477115, @steakhal wrote:

Although gcc (and probably clang too) allows specifying __attribute__((noinline)) at any declaration (by merging compatible attributes), I would prefer not to repeat ourselves.
The attribute must be present at the header to force all usages not to inline it, hence I would rather drop such attributes from the definition files.

On further inspection, I agree with you and will update the patch (+ description).

A few things misled me. First, attributes are complex, with standard and compiler-specific ones having different requirements. Then, Clang's doc about used talks about definitions, not declarations. GCC is clearer and always talks about declarations. Fortunately, https://godbolt.org/z/PnW7x9Ezz shows Clang inherits, as expected, the attributes of interest here.

But I was mainly misled because, specifically for LLVM_DUMP_METHOD, the trend in LLVM seems to apply it to definitions:

$ git grep -Ee '::dump\(\) const \{.*' | grep -ve LLVM_DUMP_METHOD | wc -l
92
$ git grep -Ee '::dump\(\) const \{.*' | grep -e LLVM_DUMP_METHOD | wc -l
184

Anyway, thanks for your feedback -- this fairly trivial patch turned out to reveal something interesting to me!

[mantognini]

Remove LLVM_DUMP_METHOD from definition

[steakhal]

Thanks for the stats.
@aaron.ballman WDYT, where should we put the LLVM_DUMP_METHOD ?

[aaron.ballman]

In D124462#3480219, @steakhal wrote:

Thanks for the stats.
@aaron.ballman WDYT, where should we put the LLVM_DUMP_METHOD ?

The documentation for the attribute says function definitions, but I don't think it matters in terms of the semantics of the attributes. I'd probably put it on the definition in this case because the goal is to keep the function around at runtime but it doesn't impact the interface of the call or how users would use it at compile time.

I noticed that we seem to be pretty bad about this advice however:

/// Note that you should also surround dump() functions with
/// `#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` so they do always
/// get stripped in release builds.

[mantognini]

In D124462#3480348, @aaron.ballman wrote:

In D124462#3480219, @steakhal wrote:

Thanks for the stats.
@aaron.ballman WDYT, where should we put the LLVM_DUMP_METHOD ?

The documentation for the attribute says function definitions, but I don't think it matters in terms of the semantics of the attributes. I'd probably put it on the definition in this case because the goal is to keep the function around at runtime but it doesn't impact the interface of the call or how users would use it at compile time.

I noticed that we seem to be pretty bad about this advice however:

/// Note that you should also surround dump() functions with
/// `#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` so they do always
/// get stripped in release builds.

I don't have a strong opinion on where LLVM_DUMP_METHOD should be and whether this means writing it twice as I can see arguments for both approaches.

In addition to not ifdef-out those functions, I note that many dump or dumpToStream functions in Analysis (and LLVM in general) don't have the LLVM_DUMP_METHOD macro at all.

So I'm tempted to say the overall situation should be improved in a separate effort, and this patch can focus only on the orthogonal linking issue. WDYT?

[steakhal]

In D124462#3482728, @mantognini wrote:

In D124462#3480348, @aaron.ballman wrote:

In D124462#3480219, @steakhal wrote:

Thanks for the stats.
@aaron.ballman WDYT, where should we put the LLVM_DUMP_METHOD ?

The documentation for the attribute says function definitions, but I don't think it matters in terms of the semantics of the attributes. I'd probably put it on the definition in this case because the goal is to keep the function around at runtime but it doesn't impact the interface of the call or how users would use it at compile time.

I noticed that we seem to be pretty bad about this advice however:

/// Note that you should also surround dump() functions with
/// `#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` so they do always
/// get stripped in release builds.

I don't have a strong opinion on where LLVM_DUMP_METHOD should be and whether this means writing it twice as I can see arguments for both approaches.

In addition to not ifdef-out those functions, I note that many dump or dumpToStream functions in Analysis (and LLVM in general) don't have the LLVM_DUMP_METHOD macro at all.

This is because in some cases it makes sense to preserve such dumps, e.g. the CFG dump might be useful not only for debugging but for others as well. Or the liveness analysis stuff and a lot more.

For this case specifically, one should not dump these taint metadata in release builds IMO. I've done some measurements to inspect the size increase for preserving some dump methods, and at the time I could not see much difference at all.
Check D124442, D124443. They have not landed yet, but I'm planning to land them in the close future.

So I'm tempted to say the overall situation should be improved in a separate effort, and this patch can focus only on the orthogonal linking issue. WDYT?

Definitely. We should consolidate the situation but at another time.
LGTM, given that you add the LLVM_DUMP_METHOD to any of the declarations.

clang/lib/StaticAnalyzer/Checkers/Taint.cpp

void taint::printTaint(ProgramStateRef State, raw_ostream &Out, const char *NL,
 
   if (!TM.isEmpty())
     Out << "Tainted symbols:" << NL;
 
   for (const auto &I : TM)
     Out << I.first << " : " << I.second << NL;
 }
 
-void dumpTaint(ProgramStateRef State) { printTaint(State, llvm::errs()); }
+LLVM_DUMP_METHOD void taint::dumpTaint(ProgramStateRef State) {
+  printTaint(State, llvm::errs());
+}
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, const Stmt *S,
                                 const LocationContext *LCtx,
                                 TaintTagType Kind) {
   return addTaint(State, State->getSVal(S, LCtx), Kind);
 }
 
 ProgramStateRef taint::addTaint(ProgramStateRef State, SVal V,