This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mantognini on Apr 26 2022, 9:14 AM.

Details

Summary

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

Diff Detail

Event Timeline

mantognini created this revision.Apr 26 2022, 9:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2022, 9:14 AM
mantognini published this revision for review.Apr 27 2022, 12:41 AM
mantognini added reviewers: NoQ, xazax.hun.

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

Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2022, 12:41 AM

Could you please elaborate whats the motivation for this change?

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.

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

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!

Remove LLVM_DUMP_METHOD from definition

mantognini edited the summary of this revision. (Show Details)Apr 28 2022, 7:18 AM
mantognini marked an inline comment as done.

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

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.

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 accepted this revision.Apr 29 2022, 9:13 AM

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.

This revision is now accepted and ready to land.Apr 29 2022, 9:13 AM
This revision was automatically updated to reflect the committed changes.