Page MenuHomePhabricator

[analyzer] Emit an error for invalid -analyzer-config inputs
ClosedPublic

Authored by Szelethus on Oct 15 2018, 3:45 AM.

Details

Summary

I'm in the process of refactoring AnalyzerOptions. The main motivation behind here is to emit warnings if an invalid -analyzer-config option is given from the command line, and be able to list them all.

In this patch, I'll implement the warning.

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Oct 15 2018, 3:45 AM
Szelethus added a comment.EditedOct 15 2018, 3:56 AM

@whisperity @xazax.hun A worry of mine is shared libraries, for example, we've got an array of Ericsson-specific checkers that we load run-time. Do we (or should we) support acquiring non-checker -analyzer-config options?

Szelethus updated this revision to Diff 169693.Oct 15 2018, 6:29 AM
NoQ added a comment.Oct 19 2018, 6:45 PM
lib/Frontend/CompilerInvocation.cpp
353

typo: "config"

I agree with NoQ. Forward and backward compatibility might be important for CodeChecker as well.
But I wonder if it make sense to have analyzer-config compatibility mode on a per config basis?
E.g., if we have two configs:

  • One did not exist in earlier clang versions, but a tool (like CodeChecker) would like to pass this to the analyzer without doing a version check first. Passing this in a compatibility mode makes sense. This could be the regural -analyzer-config
  • The second option also did not exist in earlier clang versions, but we do not want to support those versions. In the case passing this config in a more strict mode makes sense. This could be something like -analyzer-config-strict.

Or we could choose the deafults the other way around.

What do you think? Does it make sense to have the compatibility mode on a per config bases or too much code for too little gain?

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
322

I wonder if it is worth to store this in a static object, have it sorted and use binary search for the warning. If we do not expect to have a lot more options in the future I am fine with the current solution.

Szelethus planned changes to this revision.Oct 24 2018, 2:00 PM

Since I've dug myself quite deep into the refactoring effort, I realized that there are more elegant ways of achieving this, should I hammer AnalyzerOptions for long enough. I'll probably change everything but the revision title.

Szelethus updated this revision to Diff 174495.Nov 16 2018, 8:32 PM
Szelethus retitled this revision from [analyzer] Emit a warning for unknown -analyzer-config options to [analyzer] Emit an error for invalid -analyzer-config inputs.
Szelethus set the repository for this revision to rC Clang.
  • Added the discussed compatibility flag that's off by default, but is enabled by default by the driver.
  • Now emitting errors instead of warnings.
  • For the first time, introduce errors rather then asserts for invalid input.

Polite ping. :) I'd be most comfortable landing D53692 together with this patch.

xazax.hun accepted this revision.Nov 27 2018, 8:51 AM

Overall looks good to me, some minor comments inline.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
232

I wonder if it is a good idea to put the comment inside the macro.

lib/Driver/ToolChains/Clang.cpp
2372

I wonder if it makes more sense to not add this here but rather make the default option to be true.

lib/Frontend/CompilerInvocation.cpp
367–370

Do we actually need the branching here? It would be perfectly fine to always pass a pointer to Diags but sometimes just ignore it.

This revision is now accepted and ready to land.Nov 27 2018, 8:51 AM

Thanks! I'll commit around friday (with the requested fixes) to leave enough time for everyone to object.

Szelethus marked 2 inline comments as done.Nov 28 2018, 3:11 PM
Szelethus added inline comments.
lib/Driver/ToolChains/Clang.cpp
2372

We resolved this IRL, but just for the record, this line will run when the analyzer is invoked with --analyze (essentially, in driver mode), in which case we wouldn't like to receive errors on invalid input, but with -cc1 (frontend mode), it will be disabled by default .

NoQ accepted this revision.Nov 29 2018, 11:57 AM

Thanks, this looks great!

lib/Frontend/CompilerInvocation.cpp
363

Maybe we can eventually turn this into an array and address by index that's auto-generated from the options table.

But that's pretty unimportant; even though we can query options pretty often, i don't think they are a performance bottleneck.

Szelethus marked 2 inline comments as done.Nov 29 2018, 4:16 PM

Thanks!

lib/Frontend/CompilerInvocation.cpp
363

Actually, ConfigTable will probably end up on the chopping board by the end of the refactoring.

Szelethus marked 6 inline comments as done.Nov 30 2018, 1:26 PM
Szelethus added inline comments.
lib/Frontend/CompilerInvocation.cpp
367–370

I'm not sure what would be the point of that, if we don't branch here, we'll have to somewhere else, and I'm not sure whether polluting parseAnalyzerConfigs with that would help on readability.

Or, if you mean that I should just check Opts.ShouldEmitErrorsOnInvalidConfigValue inside there, that could work, but I think it makes more sense that if we don't to validate configs, don't even supply the tool for it. Less room for error.

This revision was automatically updated to reflect the committed changes.
Szelethus marked an inline comment as done.
NoQ added a comment.Dec 4 2018, 4:16 PM

Ugh. We broke backwards compatibility when an invalid -analyzer-config is specified but -analyze isn't specified. Yes, people are accidentally relying on that :/ :/ :/

In this case the driver doesn't know that it needs to add analyzer-config-compatibility-mode=true (or, even better, outright ignore all analyzer options) but the frontend doesn't know that it doesn't need to parse them.

In D53280#1319446, @NoQ wrote:

Ugh. We broke backwards compatibility when an invalid -analyzer-config is specified but -analyze isn't specified. Yes, people are accidentally relying on that :/ :/ :/

In this case the driver doesn't know that it needs to add analyzer-config-compatibility-mode=true (or, even better, outright ignore all analyzer options) but the frontend doesn't know that it doesn't need to parse them.

Oh my god. Can you go a little more in depth? Was the problem something along the lines of treu typo, or a non-existent flag? Is that at all salvageable? I mean, losing the entire patch would be super bad imo.

Also, I can't attend to this issue immediately -- replying on the phone is one thing, but almost any other action is out of my reach for the next couple days/weeks.

Szelethus marked an inline comment as done.Dec 4 2018, 4:45 PM
Szelethus added inline comments.
lib/Driver/ToolChains/Clang.cpp
3307–3309

Hmmm. If we just moved the CmdArgs.push_back("-analyzer-config-compatibility-mode=true"); line to this function (which actually invokes RenderAnalyzerOptions), that would solve the issue, wouldn't it? Or somewhere in an even more general driver function.

NoQ added a comment.Dec 4 2018, 4:51 PM

It's a flag that was removed but old soft doesn't know that it was removed, and it won't be fixed because it's old. I guess a typo would have caused the same problem.

Not urgent, no pressure. Yes, i believe this patch is good and should stay there. At the same time, by the looks of it, coming up with a fix for this "problem" may be annoying.

lib/Driver/ToolChains/Clang.cpp
3307–3309

Yeah, but it's not going to be an AnalyzeJobAction in the problematic case. So you'd have to add this flag to every single clang invocation.

Szelethus marked an inline comment as done.EditedDec 4 2018, 5:02 PM
In D53280#1319503, @NoQ wrote:

It's a flag that was removed but old soft doesn't know that it was removed, and it won't be fixed because it's old.

We could just add the flag back, and do nothing with it. We have already mentioned converting some frontend flags to analyzer configs, and removing some long enabled-by-default options while still leaving them in the code in some form or another for backward compatibility, so some carefully documented litter in AnalyzerOptions here and there wouldn't be the end of the world.

Edit: I just realized that this could easily be an issue for other users of the static analyzer (or clang in general) as well.

lib/Driver/ToolChains/Clang.cpp
3307–3309

Aaaaand I bet that's something we can't do just like that.

I guess the solution would be to check whether there are any user supplied flags with "analyze" substring, and add the compatibility flag then. It is possible if not probable that a non-static-analyzer related flag with a name like that will eventually be added, but I guess we can live with it.

The deadline is around the creation of the 8.0.0. release branch, right?

NoQ added a comment.Dec 12 2018, 1:35 PM

I guess the solution would be to check whether there are any user supplied flags with "analyze" substring, and add the compatibility flag then. It is possible if not probable that a non-static-analyzer related flag with a name like that will eventually be added, but I guess we can live with it.

Yeah, that's pretty much the easiest solution that i see (i guess we need to look for -analyzer-config specifically), but it's still pretty gross.

The deadline is around the creation of the 8.0.0. release branch, right?

I'd feel slightly more comfortable if some sort of fix gets in before the Christmas break.

We could just add the flag back, and do nothing with it. (...) Edit: I just realized that this could easily be an issue for other users of the static analyzer (or clang in general) as well.

It doesn't sound appealing to bring back all the flags that we've removed recently, but i guess it's not that hard to track back all the options that we removed since, say, 6.0.0 and add them back, marked as "deprecated" somehow, and remove them later when either we have a solution for the general case or we don't care anymore :)

The deadline is around the creation of the 8.0.0. release branch, right?

I'd feel slightly more comfortable if some sort of fix gets in before the Christmas break.

Sure! :)