Page MenuHomePhabricator

[analyzer] Create infrastructure for organizing and declaring analyzer configs.
AbandonedPublic

Authored by teemperor on Jul 30 2017, 1:49 PM.

Details

Summary

This patch is a first step towards restructuring the way we handle configs (i.e. the values we pass for example to -analyzer-config).

At the moment all configs are only implicitly defined by checking for them from some checker in the SA, which makes many aspects regarding the management of those options much harder than it should be. For example is it currently not really possible to recognize typos in the config keys that the user provided. We do some basic checking if the targeted checker is registered, but we don't do any checking if the given config is actually something that exists. This means that any typos in the -analyzer-config that aren't affecting the checker won't be reported or noticed by the SA. It also means that making a list of what configurations are available isn't possible at the moment without grepping the source code.

With this patch all configurations of the SA are moved to the Checkers.td and are emitted to the Checkers.inc. We also add support for adding/checking options to the CheckerRegistry and are using this to verify if the provided configuration values actually exist.

Diff Detail

Event Timeline

teemperor created this revision.Jul 30 2017, 1:49 PM

I'm open to suggestions how we should handle user-provided checkers that maybe come with their own configs. Should we add that if we have a custom checker in the registry, that we disable this check to keep backwards compability? Or do we demand users to register their own configs in the registry?

teemperor updated this revision to Diff 108883.Jul 31 2017, 2:01 AM
  • Remove some unneeded includes

I like the directions of this patch.
In general, I am in favor of explicitly registering the options from user defined checkers.
But changing a config option will now break the command line compatibility, so I wonder how do we want to handle this:

  • Have a list of no-op configs that we accept but warn that it has been replaced/removed?
  • Just do not care and break compatibility?
  • Something else?

I have a wishlist for this feature but I am perfectly fine to only address those in follow-up patches:

  • Ability to set descriptions and maybe default values in the Checkers.td
  • Command line argument to dump the list of options with descriptions (and defaults)

I like the directions of this patch.
In general, I am in favor of explicitly registering the options from user defined checkers.
But changing a config option will now break the command line compatibility, so I wonder how do we want to handle this:

  • Have a list of no-op configs that we accept but warn that it has been replaced/removed?
  • Just do not care and break compatibility?
  • Something else?

Don't have a preference here, but adding a forwarding config that also emits a warning should be easy to do.

I have a wishlist for this feature but I am perfectly fine to only address those in follow-up patches:

  • Ability to set descriptions and maybe default values in the Checkers.td
  • Command line argument to dump the list of options with descriptions (and defaults)

Yes, I would prefer having this in follow up patches. I tried to keep this as a minimal starting example because this currently blocks @yamaguchi 's GSoC project for bash completion. There we want to complete the values for -analyzer-config and we currently don't have a good way to get a complete list of available configs from the driver :).

zaks.anna edited edge metadata.Aug 1 2017, 2:31 PM
I tried to keep this as a minimal starting example because this currently blocks @yamaguchi 's GSoC project for bash completion. There we want to complete the values for -analyzer-config and we currently don't have a good way to get a complete list of available configs from the driver :).

It is not clear that we want to make the analyzer options user visible. These often used for staging purposes, where we develop a feature or a set of heuristics behind a flag. We do not support changing a lot of these options.

dcoughlin edited edge metadata.Aug 3 2017, 10:16 AM

I just want to second that we really don't want to treat the analyzer-config options as user visible. These are useful as staging flags for analyzer development/testing and occasionally as customization points for IDEs/build systems. But they do not expose a coherent (or even intelligible) model to the end user. In particular, this means that we should never, ever ask (or recommend) that the end user set these from the command line. They are also not API -- we make no promises of backward compatibility.

We also do sometimes use analyzer config flags as an escape hatch that lets us pass a flag to both a new clang (which understands the flag) and an old flag (which doesn't). It would be a shame if we lost that escape hatch.

teemperor abandoned this revision.Aug 3 2017, 2:07 PM

I see, thanks for the information! If we don't need to support this in the shell-completion and we shouldn't report invalid arguments, then it seems this review is stuck here. I'll abandon as I don't see any other use case for this code.

(Also, sorry for the late response).