[PATCH] Modeling APIs in the Static Analyzer
Gábor Horváth
xazax.hun at gmail.com
Wed Jul 30 03:29:08 PDT 2014
Hi Ted,
Thank you for the review.
On 30 July 2014 08:18, Ted Kremenek <kremenek at apple.com> wrote:
> Hi Gábor,
>
> Thanks for making progress on this very promising enhancement to the
> analyzer. I have an assortment of comments, in no particular order:
>
> - ModelInjector.h and ModelConsumer.h
>
> There is a comment at the top of these files, but I think a bit more
> explanation is needed. For example:
>
> MetaConsumer.cpp:
>
> +// "Meta" ASTConsumer for consuming model files.
>
> That doesn't really explain anything. What does "Meta" in quotes mean? I
> think an explanation here on what this does is helpful when someone
> discovers this code for the first time.
>
> Similarly, we should add some high-level comments for CodeInjector.h and
> ModelInjector.h. We have a good start in ModelInjector.h:
>
> +/// \file
> +/// \brief Defines the clang::ento::ModelInjector class which implements
> the
> +/// clang::CodeInjector interface. This class is responsible for injecting
> +/// function definitions that were synthetized from model files.
> +///
>
> Let's consider expanding it:
>
> /// \brief This file defines the clang::ento::ModelInjector class which
> implements the
> /// clang::CodeInjector interface. This class is responsible for injecting
> /// function definitions that were synthesized from model files.
>
> /// Model files allow definitions of functions to be lazily constituted
> for functions
> /// which lack bodies in the original source code. This allows the
> analyzer
> /// to more precisely analyze code that calls such functions, analyzing
> the
> /// artificial definitions (which typically approximate the semantics of
> the
> /// called function) when called by client code. These definitions are
> /// reconstituted lazily, on-demand, by the static analyzer engine.
>
> CodeInjector.h provides some information, but it is a bit vague:
>
> +///
> +/// \file
> +/// \brief Defines the clang::CodeInjector interface which is responsible
> for
> +/// injecting AST of function definitions from external source.
> +///
>
> It's a bit unclear how this gets used. I think a bit of prose here would
> help clarify its role in the static analyzer. I also think the
> CodeInjector interface is also more abstract than the prose describes.
> There's nothing about CodeInjector's interface that requires the injected
> definitions to come from an external source. That's an implementation
> detail of a concrete subclass. Instead, all CodeInjector does is provide
> an interface that lazily provides definitions for functions and methods
> that may not be present in the original source.
>
I have added some further documentation to address these issues.
>
> I'm also looking at the change to
> StaticAnalyzer/Frontend/FrontendActions.cpp, and wonder if we can simplify
> things:
>
> +++ lib/StaticAnalyzer/Frontend/FrontendActions.cpp (working copy)
> @@ -7,9 +7,11 @@
> //
>
> //===----------------------------------------------------------------------===//
>
> +#include "clang/Frontend/CompilerInstance.h"
> #include "clang/StaticAnalyzer/Frontend/FrontendActions.h"
> -#include "clang/Frontend/CompilerInstance.h"
> #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
> +#include "clang/StaticAnalyzer/Frontend/ModelConsumer.h"
> +#include "ModelInjector.h"
> using namespace clang;
> using namespace ento;
>
> @@ -18,6 +20,14 @@
> return CreateAnalysisConsumer(CI.getPreprocessor(),
> CI.getFrontendOpts().OutputFile,
> CI.getAnalyzerOpts(),
> - CI.getFrontendOpts().Plugins);
> + CI.getFrontendOpts().Plugins,
> + new ModelInjector(CI));
> }
>
>
>
> It looks like CreateAnalysisConsumer just continues to grow more
> arguments, all which derive from using 'CI'. This seems silly, since this
> function is called in one place. Instead of intro ducting a dependency on
> ModelInjector.h in this file, we can just sink these arguments into
> CreateAnalysisConsumer() itself, resulting in:
>
> return CreateAnalysisConsumer(CI);
>
> and let CreateAnalysisConsumer() do all that boilerplate.
>
That was my original idea as well but it broke the compilation of some code
in extra repository and I wasn't sure if it is ok to break the API with
this patch. But I find it cleaner this way so I modified it in this
iteration.
> Next, let's look at the change to FrontendAction:
>
> class FrontendAction {
> + /// Is this action invoked on a model file? Model files are incomplete
> + /// translation units that relies on type information from another
> translation
> + /// unit. Check ParseModelFileAction for details.
> + bool ModelFile;
>
>
> Perhaps "IsModelFile"? "ModelFile" sounds like it should be a reference
> to the file itself.
>
> FrontendInputFile CurrentInput;
> std::unique_ptr<ASTUnit> CurrentASTUnit;
> CompilerInstance *Instance;
> @@ -105,7 +109,11 @@
> /// @}
>
> public:
> - FrontendAction();
> + /// \brief Constructor
> + ///
> + /// \param modelFile determines whether the source files this action
> invoked
> + /// on should be treated as a model file. Defaults to false.
> + FrontendAction(bool modelFile = false);
>
>
> It seems suboptimal to modify the interface of FrontendAction just for
> this one edge case. Instead of modifying the constructor arguments, we
> could default initialize "IsModelFile" to false, and have a setter to
> change it. For example:
>
> ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *>
> &Bodies)
> : ASTFrontendAction(/*ModelFile=*/true), Bodies(Bodies) {}
>
> becomes:
>
> ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *>
> &Bodies)
> : Bodies(Bodies) {
> IsModelFile = true;
> }
>
> Looking at this more, I wonder if we should modify FrontendAction at all.
> The only place where isModelParsingAction() is called is in one spot in
> CompilerInstance.cpp:
>
> if (hasSourceManager() && !Act.isModelParsingAction())
>
> It *might* be cleaner to just have a virtual member function in
> FrontendAction, which defaults to returning false, but is generic for all
> subclasses to override. Then we don't need the "IsModelFile" field in
> FrontendAction at all, and we just have ParseModelFileAction override that
> single member function. We could then name that method to be something a
> bit more generic. That would allow us to not touch FrontendAction at all
> except for providing that single virtual method that can be overridden in
> subclasses. I somewhat prefer this approach because it provides a cleaner
> separation of concerns between FrontendAction (which is defined
> libFrontend) and the static analyzer. That would also allow you to get rid
> of isModelParsingAction() entirely (replacing it with something more
> generic).
>
>
You are right, it is much cleaner to use a virtual function, so I modified
the patch to use that approach. The new virtual function has the same name
because I have yet to find any better and more general name yet. Do you
have an idea for a better name?
> As for the test case:
>
> +typedef int* intptr;
> +
> +void modelled(intptr p);
> +
> +int main() {
> + modelled(0);
> + return 0;
> +}
>
>
> Please add some comments in this test file explaining what is happening.
> Also, it would be great if this both used FileCheck (which it does now)
> but also verified the diagnostics so we get cross-checking of the output
> (we see this in some analyzer tests). It also makes it easier to
> understand the test.
>
> Also, is there a reason to break up the tests
> between model-suppress-falsepos.cpp and model-file.cpp? It seems like one
> test file will do fine; just clearly comment on what is happening for each
> test. I also recommend called the modeled function "modeledFunction"
> instead of "modelled" (which according to my spell checker has an
> additional 'l').
>
I have merged the test files and also added some commets to explain what is
going on. I have fixed the misspelling as well. The nullpointer dereference
is only checked through plist because the point where the comment with the
expected warning should be added is inside the model file and it did not
work for me if the comment was in a separate file. If there is a different
way to verify the warnings that are in a separate file and I did not find
it, please let me know.
>
> As for the model files themselves:
>
> Index: test/Analysis/modelled.model
> ===================================================================
> --- test/Analysis/modelled.model (revision 0)
> +++ test/Analysis/modelled.model (working copy)
> @@ -0,0 +1,3 @@
> +void modelled(intptr p) {
> + ++*p;
> +}
> \ No newline at end of file
> Index: test/Analysis/notzero.model
> ===================================================================
> --- test/Analysis/notzero.model (revision 0)
> +++ test/Analysis/notzero.model (working copy)
>
>
> Let's put these in a separate subdirectory, for example, "models", instead
> of mixing them with the tests. This way they really serve as "inputs" to
> the analyzer.
>
I have moved the model files to tests/Inputs/Models.
>
> Overall this is looking good. I think the explanatory comments will
> really help people understand what this is doing, and I think changing how
> we thread the information through FrontendAction will help not introduce an
> artificial tainting of FrontendAction with concepts specific to the static
> analyzer.
>
> Cheers,
> Ted
>
>
Thanks,
Gábor
>
> On Jul 16, 2014, at 2:45 AM, Gábor Horváth <xazax.hun at gmail.com> wrote:
>
>
>
>
> On 14 July 2014 19:32, Anna Zaks <ganna at apple.com> wrote:
>
>>
>> On Jul 13, 2014, at 6:11 AM, Gábor Horváth <xazax.hun at gmail.com> wrote:
>>
>> Hi Anna,
>>
>> Thank you for the review. I have tweaked the test, so it no longer
>> requires the error reporting tweak that is not done yet to pass. I have
>> also added some high level comments to some files, if you think some
>> information is lacking I will add them in the next iteration as well. The
>> BugReporter patch is now separated into a different patch.
>>
>>
>> On 11 July 2014 18:02, Anna Zaks <ganna at apple.com> wrote:
>>
>>>
>>> For example, modeling functions should allow you to find bugs and
>>> suppress false positives outside of those functions. I would suggest adding
>>> a few of those tests first.
>>>
>>>
>> How are the false positives suppressed? I did not find any resource on
>> that. Found some analyzer attributes but I did not find them suitable for
>> this purpuse at the first glance. But I think once the locations that are
>> in a model file are omitted from the report path, the regular methods for
>> suppressing false positives should work (and I will definitely add test
>> case to ensure this once it is done).
>>
>>
>> What I meant is that it is possible to construct a test where ability to
>> model a function would eliminate a false positive. This would be another
>> way to test your patch without worrying about BugReporter.
>>
>
> I got it now, thansk. I have updated the patch with a test case where a
> false positive case is eliminated by a model file.
>
> Thanks,
> Gábor
>
>
>> Thanks,
>> Gábor
>> <api_modeling.patch><bugreporter.patch>
>>
>>
>>
> <api_modeling.patch><bugreporter.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140730/0cb77877/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: api_modeling.patch
Type: text/x-patch
Size: 44480 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140730/0cb77877/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bugreporter.patch
Type: text/x-patch
Size: 1728 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140730/0cb77877/attachment-0001.bin>
More information about the cfe-commits
mailing list