Page MenuHomePhabricator

[analyzer] StdLibraryFunctionsChecker: Add option to display loaded summaries
ClosedPublic

Authored by martong on Apr 14 2020, 8:43 AM.

Diff Detail

Event Timeline

martong created this revision.Apr 14 2020, 8:43 AM

This is a great idea, but the tests just seem to, well, test the new functionality?

On a different issue, take a look at how certain help related frontend flags (not -analyzer-config ones!) skip the analysis, like -analyzer-checker-help, or -analyzer-list-enabled-checkers. If I were to set DisplayLoadedSummaries to true, most probably I'd only be interested in the capabilities of the checker, not the analysis itself. Though, I don't have an out-of-the-box solution that could work here, unless we outright emit an error in the checker registry function. That wouldn't be very nice, would it? :^) So, for the time being, regard this as me thinking aloud.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
299

I see what you mean, but "loaded" is a bit ambiguous unless you know how the checker operates.

The new option will simply list the (found) functions that have a summary based check enabled. (The term "Loaded" may be misleading in the current implementation, somebody can think if it is loaded from file?) A more detailed output would be better (display the signature too, or maybe the whole summary?). For simple purpose the current way may be enough but it may be useful (probably for a non-checker-developer too) to see the summary details.

martong updated this revision to Diff 260647.Apr 28 2020, 8:23 AM
  • Rebase to master
  • Add the option to the lit test analyzer-config.c
martong marked an inline comment as done.Apr 28 2020, 8:31 AM
martong added inline comments.
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
299

Yeah, okay, what about "the found summaries" or "applicable summaries" or "? This list can be different for every TU. So I am adding "... for the translation unit".

martong updated this revision to Diff 260651.Apr 28 2020, 8:31 AM
  • Better description for the option in Checkers.td

This is a great idea, but the tests just seem to, well, test the new functionality?

On a different issue, take a look at how certain help related frontend flags (not -analyzer-config ones!) skip the analysis, like -analyzer-checker-help, or -analyzer-list-enabled-checkers. If I were to set DisplayLoadedSummaries to true, most probably I'd only be interested in the capabilities of the checker, not the analysis itself. Though, I don't have an out-of-the-box solution that could work here, unless we outright emit an error in the checker registry function. That wouldn't be very nice, would it? :^) So, for the time being, regard this as me thinking aloud.

This new config option will simply list the (found) functions that have a summary based check enabled. This can be different for each TU. If a function cannot be looked up in a TU then we will never use its summary.
On the other hand, yes, it would be a good idea to have an option to list all the configured summaries. This list would be the same for each TU. Do you mind if I'd do that rather in another patch?

The new option will simply list the (found) functions that have a summary based check enabled. (The term "Loaded" may be misleading in the current implementation, somebody can think if it is loaded from file?) A more detailed output would be better (display the signature too, or maybe the whole summary?). For simple purpose the current way may be enough but it may be useful (probably for a non-checker-developer too) to see the summary details.

Yes, "loaded" might not be the best, I changed it to "found". About the more detailed output: is there a way to dump nicely the signature of a function decl? I don't want to dump the AST of it because that might be overkill. Could I just dump the content of the buffer that relates to the SourceRange of the FunctionDecl (if that does not have a definition)?

Harbormaster completed remote builds in B54973: Diff 260647.
martong updated this revision to Diff 260687.Apr 28 2020, 10:14 AM
  • Handle and test the display of signatures

The new option will simply list the (found) functions that have a summary based check enabled. (The term "Loaded" may be misleading in the current implementation, somebody can think if it is loaded from file?) A more detailed output would be better (display the signature too, or maybe the whole summary?). For simple purpose the current way may be enough but it may be useful (probably for a non-checker-developer too) to see the summary details.

Yes, "loaded" might not be the best, I changed it to "found". About the more detailed output: is there a way to dump nicely the signature of a function decl? I don't want to dump the AST of it because that might be overkill. Could I just dump the content of the buffer that relates to the SourceRange of the FunctionDecl (if that does not have a definition)?

I just have updated the patch so now the signature is printed.

This is a great idea, but the tests just seem to, well, test the new functionality?

I updated the patch so now we have a new RUN line that checks with FileCheck.

balazske added inline comments.Apr 29 2020, 3:32 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
651

Maybe the Decl::print method can be used? I pretty-prints the declaration (hopefully there is an option to print only the prototype), otherwise we can get the code as written in the source-file(?) that makes comparing the output more difficult.

664

I would prefer to display the function prototype in a separate line (if not only the name is included in the string). Or at least enclose the prototype with ' characters. Or the Loaded summary for: form.

martong marked 4 inline comments as done.Apr 29 2020, 8:46 AM
martong added inline comments.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
651

Yeah, thanks! I switched to use that!

664

Ok I use the Loaded summary for: form from now on.

martong updated this revision to Diff 260930.Apr 29 2020, 8:46 AM
martong marked 2 inline comments as done.
  • Use Decl::print and 'for: '

It would be nice to have a user-facing option that lists the functions this checker is capable of modeling in another patch :)

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
299

There is no need to keep this short :) Feel free to go on a bit of a tangent, if you prefer. But, to me, it seems like this isn't a user-facing option, this is a debug flag to check which functions are present in the TU, which is okay, but in that case we better make it Hidden, so it will only show up in -analyzer-checker-help-developer.

xazax.hun accepted this revision.May 6 2020, 1:39 AM

LGTM!

This revision is now accepted and ready to land.May 6 2020, 1:39 AM
This revision was automatically updated to reflect the committed changes.