[analyzer] The use of tblgen in the analyzer

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[analyzer] The use of tblgen in the analyzer

via cfe-dev
Hi!

I have two questions about this topic.

1. Checker registration:

I am in the process of updating clang-analyzer.llvm.org, and as I went through the checkers that were missing, I encountered this:

def NonnullGlobalConstantsChecker: Checker<"NonnilStringConstants">,
  HelpText<"Assume that const string-like globals are non-null">,
  DescFile<"NonilStringConstantsChecker.cpp">;

What is particularly interesting here (other than the mismatched names), is that NonilStringConstantsChecker.cpp doesn't exist, in fact it never did. To my greatest surprise, DescFile isn't used anywhere!

#define CHECKER(FULLNAME,CLASS,DESCFILE,HELPTEXT,GROUPINDEX,HIDDEN) \ registry.addChecker(register##CLASS, FULLNAME, HELPTEXT);

So, is there a point in keeping DescFile entries?

2. Use tblgen for AnalyzerOptions:

I dislike that I need to look at source code, or doxygen at best to know what kind -analyzer-config options are available. Would it be a good idea for me to refactor it with tblgen?

Cheers,
Kristóf

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] The use of tblgen in the analyzer

via cfe-dev
This is why i say that "i found a bug by looking at this report" is not a valid definition of a true positive. You can find bug in *any* code if you stare at it long enough.

I've no idea how DescFile can be useful. It might be that the old autotools-based build system was using it somehow. Finding file by checker name is a rare task that can usually be accomplished with grep, so no need to keep it for humans. Feel free to squash.

Our -analyzer-config is indeed pretty chaotic. I heard that it was designed for just one purpose: to not fail with an error when a flag is not available, which is good for backward and/or forward compatibility.

I guess maintaining a current list of flags will still be good, especially with longer descriptions, so you can try to do this. I highly favor .def files (that can be #included directly, eg. Analyses.def) over TableGen because TableGen code looked very hard to understand last time i tried to work with it.

Analyzer config flags are also very different, which suggests a UI distinction:
- A lot of them define "alpha" branches, so we shouldn't suggest users to use them unless they're developing the respective feature.
- Some define branches that were turned on by default so long ago that nobody remembers them, so i guess they can be removed (eg., c++-template-inlining).
- Some are thresholds or "temporary" hacks that are good to have as an option so that it was easy to experiment with them (adjust thresholds or remove hacks).
- Some are useful for debugging (eg., prune-paths).
- Some are actually important to maintain (eg., elide-constructors, because lack of copy elision is a valid behavior until C++17, and it's likely that some code is compiled without copy elision, and they may want to tell the Analyzer to match their behavior).

Supporting checker options (that are also passed as weird -analyzer-config flags) is going to be a separate challenge.


On 10/7/18 11:26 AM, Kristóf Umann via cfe-dev wrote:
Hi!

I have two questions about this topic.

1. Checker registration:

I am in the process of updating clang-analyzer.llvm.org, and as I went through the checkers that were missing, I encountered this:

def NonnullGlobalConstantsChecker: Checker<"NonnilStringConstants">,
  HelpText<"Assume that const string-like globals are non-null">,
  DescFile<"NonilStringConstantsChecker.cpp">;
What is particularly interesting here (other than the mismatched names), is that NonilStringConstantsChecker.cpp doesn't exist, in fact it never did. To my greatest surprise, DescFile isn't used anywhere!
#define CHECKER(FULLNAME,CLASS,DESCFILE,HELPTEXT,GROUPINDEX,HIDDEN) \ registry.addChecker(register##CLASS, FULLNAME, HELPTEXT);
So, is there a point in keeping DescFile entries?
2. Use tblgen for AnalyzerOptions:
I dislike that I need to look at source code, or doxygen at best to know what kind -analyzer-config options are available. Would it be a good idea for me to refactor it with tblgen?
Cheers,
Kristóf

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] The use of tblgen in the analyzer

via cfe-dev
Thanks for the clarification! I think I'll refactor the whole thing as you suggested, it's been bothering me for too long. I'll move the flags to a .def file and attempt to emit warnings on invalid input.

Artem Dergachev <[hidden email]> ezt írta (időpont: 2018. okt. 9., K, 3:24):
This is why i say that "i found a bug by looking at this report" is not a valid definition of a true positive. You can find bug in *any* code if you stare at it long enough.

I've no idea how DescFile can be useful. It might be that the old autotools-based build system was using it somehow. Finding file by checker name is a rare task that can usually be accomplished with grep, so no need to keep it for humans. Feel free to squash.

Our -analyzer-config is indeed pretty chaotic. I heard that it was designed for just one purpose: to not fail with an error when a flag is not available, which is good for backward and/or forward compatibility.

I guess maintaining a current list of flags will still be good, especially with longer descriptions, so you can try to do this. I highly favor .def files (that can be #included directly, eg. Analyses.def) over TableGen because TableGen code looked very hard to understand last time i tried to work with it.

Analyzer config flags are also very different, which suggests a UI distinction:
- A lot of them define "alpha" branches, so we shouldn't suggest users to use them unless they're developing the respective feature.
- Some define branches that were turned on by default so long ago that nobody remembers them, so i guess they can be removed (eg., c++-template-inlining).
- Some are thresholds or "temporary" hacks that are good to have as an option so that it was easy to experiment with them (adjust thresholds or remove hacks).
- Some are useful for debugging (eg., prune-paths).
- Some are actually important to maintain (eg., elide-constructors, because lack of copy elision is a valid behavior until C++17, and it's likely that some code is compiled without copy elision, and they may want to tell the Analyzer to match their behavior).

Supporting checker options (that are also passed as weird -analyzer-config flags) is going to be a separate challenge.


On 10/7/18 11:26 AM, Kristóf Umann via cfe-dev wrote:
Hi!

I have two questions about this topic.

1. Checker registration:

I am in the process of updating clang-analyzer.llvm.org, and as I went through the checkers that were missing, I encountered this:

def NonnullGlobalConstantsChecker: Checker<"NonnilStringConstants">,
  HelpText<"Assume that const string-like globals are non-null">,
  DescFile<"NonilStringConstantsChecker.cpp">;
What is particularly interesting here (other than the mismatched names), is that NonilStringConstantsChecker.cpp doesn't exist, in fact it never did. To my greatest surprise, DescFile isn't used anywhere!
#define CHECKER(FULLNAME,CLASS,DESCFILE,HELPTEXT,GROUPINDEX,HIDDEN) \ registry.addChecker(register##CLASS, FULLNAME, HELPTEXT);
So, is there a point in keeping DescFile entries?
2. Use tblgen for AnalyzerOptions:
I dislike that I need to look at source code, or doxygen at best to know what kind -analyzer-config options are available. Would it be a good idea for me to refactor it with tblgen?
Cheers,
Kristóf

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] The use of tblgen in the analyzer

via cfe-dev
In reply to this post by via cfe-dev


On Oct 7, 2018, at 11:26 AM, Kristóf Umann via cfe-dev <[hidden email]> wrote:

Hi!

I have two questions about this topic.

1. Checker registration:

I am in the process of updating clang-analyzer.llvm.org, and as I went through the checkers that were missing, I encountered this:

def NonnullGlobalConstantsChecker: Checker<"NonnilStringConstants">,
  HelpText<"Assume that const string-like globals are non-null">,
  DescFile<"NonilStringConstantsChecker.cpp">;

What is particularly interesting here (other than the mismatched names), is that NonilStringConstantsChecker.cpp doesn't exist, in fact it never did. To my greatest surprise, DescFile isn't used anywhere!

#define CHECKER(FULLNAME,CLASS,DESCFILE,HELPTEXT,GROUPINDEX,HIDDEN) \ registry.addChecker(register##CLASS, FULLNAME, HELPTEXT);

So, is there a point in keeping DescFile entries?

Just to give some context for ‘DescFile', the intention of this was to point to a file that we could use to add detailed documentation for a checker with a specific format, think of something like extensive doxygen documentation, and a tool would be able to extract such detailed documentation from the source files and render them in another format, like html pages for clang-analyzer.llvm.org.

However, this never materialized and it’s useless now.


2. Use tblgen for AnalyzerOptions:

I dislike that I need to look at source code, or doxygen at best to know what kind -analyzer-config options are available. Would it be a good idea for me to refactor it with tblgen?

Cheers,
Kristóf
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] The use of tblgen in the analyzer

via cfe-dev
+ Dániel Krupp

Another idea I was thinking is 

1. To be able to dump all analyzer configs in a JSON (or yaml, whatever) file, and similarly, be able to read such a config file.

2. The use of Optional is impactical. We lazily evaluate every option the user supplied only when is asked for. Instead, we could do the evaluation when the cmd line options are parsed, at which point we can also check for wrong input (like whether string options have typos, or flag X won't do a thing when flag Y isn't set) since we have access to a DiagnosticsEngine.

3. Remove getBooleanOption, getOptionAsString, etc, and only provide general option getters for checkers, in order to support shared lib checkers. This would force anyone who'd like add a flag to actually register it properly. I'm already trying to come up a neat solution to neatly handle checker options.

I've wasted more time on making a typos in config flags than I'd like to admit, so I'm super motivated to make a much stricter version of AnalyzerOptions. While keeping backward compatibility, of course.

Argyrios Kyrtzidis <[hidden email]> ezt írta (időpont: 2018. okt. 13., Szo, 0:45):


On Oct 7, 2018, at 11:26 AM, Kristóf Umann via cfe-dev <[hidden email]> wrote:

Hi!

I have two questions about this topic.

1. Checker registration:

I am in the process of updating clang-analyzer.llvm.org, and as I went through the checkers that were missing, I encountered this:

def NonnullGlobalConstantsChecker: Checker<"NonnilStringConstants">,
  HelpText<"Assume that const string-like globals are non-null">,
  DescFile<"NonilStringConstantsChecker.cpp">;

What is particularly interesting here (other than the mismatched names), is that NonilStringConstantsChecker.cpp doesn't exist, in fact it never did. To my greatest surprise, DescFile isn't used anywhere!

#define CHECKER(FULLNAME,CLASS,DESCFILE,HELPTEXT,GROUPINDEX,HIDDEN) \ registry.addChecker(register##CLASS, FULLNAME, HELPTEXT);

So, is there a point in keeping DescFile entries?

Just to give some context for ‘DescFile', the intention of this was to point to a file that we could use to add detailed documentation for a checker with a specific format, think of something like extensive doxygen documentation, and a tool would be able to extract such detailed documentation from the source files and render them in another format, like html pages for clang-analyzer.llvm.org.

However, this never materialized and it’s useless now.
Thanks! That actually sounds neat, we had a few conversations in the office about auto-generating the website, like clang-tidy, so I'm not a hurry to remove these just yet.


2. Use tblgen for AnalyzerOptions:

I dislike that I need to look at source code, or doxygen at best to know what kind -analyzer-config options are available. Would it be a good idea for me to refactor it with tblgen?

Cheers,
Kristóf
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [analyzer] The use of tblgen in the analyzer

via cfe-dev
Argyrios, thanks for the archeological insight! I wouldn't have guessed :)

Hmm, what if we make analyzer options verified by default, but have such verification disabled by a flag (eg. `-analyzer-config-compatibility-mode`) that's enabled by default *by* the Clang Driver?

Analyzer GUIs/wrappers usually work through the Driver by appending `--analyze` to the build command. It might look like scan-build is an exception as it runs `clang -cc1` directly, but before that it anyway dumps the default driver flags by running appending `--analyze -###` to the (modified) build command. So this might be both safe for backward/forward compatibility purposes *and* helpful for developers.


On 10/17/18 12:57 PM, Kristóf Umann via cfe-dev wrote:
+ Dániel Krupp

Another idea I was thinking is 

1. To be able to dump all analyzer configs in a JSON (or yaml, whatever) file, and similarly, be able to read such a config file.

2. The use of Optional is impactical. We lazily evaluate every option the user supplied only when is asked for. Instead, we could do the evaluation when the cmd line options are parsed, at which point we can also check for wrong input (like whether string options have typos, or flag X won't do a thing when flag Y isn't set) since we have access to a DiagnosticsEngine.

3. Remove getBooleanOption, getOptionAsString, etc, and only provide general option getters for checkers, in order to support shared lib checkers. This would force anyone who'd like add a flag to actually register it properly. I'm already trying to come up a neat solution to neatly handle checker options.

I've wasted more time on making a typos in config flags than I'd like to admit, so I'm super motivated to make a much stricter version of AnalyzerOptions. While keeping backward compatibility, of course.

Argyrios Kyrtzidis <[hidden email]> ezt írta (időpont: 2018. okt. 13., Szo, 0:45):


On Oct 7, 2018, at 11:26 AM, Kristóf Umann via cfe-dev <[hidden email]> wrote:

Hi!

I have two questions about this topic.

1. Checker registration:

I am in the process of updating clang-analyzer.llvm.org, and as I went through the checkers that were missing, I encountered this:

def NonnullGlobalConstantsChecker: Checker<"NonnilStringConstants">,
  HelpText<"Assume that const string-like globals are non-null">,
  DescFile<"NonilStringConstantsChecker.cpp">;
What is particularly interesting here (other than the mismatched names), is that NonilStringConstantsChecker.cpp doesn't exist, in fact it never did. To my greatest surprise, DescFile isn't used anywhere!
#define CHECKER(FULLNAME,CLASS,DESCFILE,HELPTEXT,GROUPINDEX,HIDDEN) \ registry.addChecker(register##CLASS, FULLNAME, HELPTEXT);
So, is there a point in keeping DescFile entries?

Just to give some context for ‘DescFile', the intention of this was to point to a file that we could use to add detailed documentation for a checker with a specific format, think of something like extensive doxygen documentation, and a tool would be able to extract such detailed documentation from the source files and render them in another format, like html pages for clang-analyzer.llvm.org.

However, this never materialized and it’s useless now.
Thanks! That actually sounds neat, we had a few conversations in the office about auto-generating the website, like clang-tidy, so I'm not a hurry to remove these just yet.

2. Use tblgen for AnalyzerOptions:
I dislike that I need to look at source code, or doxygen at best to know what kind -analyzer-config options are available. Would it be a good idea for me to refactor it with tblgen?
Cheers,
Kristóf
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev