Thanks Gábor. This looks great.
One last thing that occurred to me is that we should not create a ModelInjector
at all unless the model-path is specified. This allows us to preserve the
existing behavior in the analyzer while we continue to evolve this new
functionality.
Specifically:
std::unique_ptr<AnalysisASTConsumer>
-ento::CreateAnalysisConsumer(const Preprocessor &pp, const std::string &outDir,
- AnalyzerOptionsRef opts,
- ArrayRef<std::string> plugins) {
+ento::CreateAnalysisConsumer(CompilerInstance &CI) {
// Disable the effects of '-Werror' when using the AnalysisConsumer.
- pp.getDiagnostics().setWarningsAsErrors(false);
+ CI.getPreprocessor().getDiagnostics().setWarningsAsErrors(false);
- return llvm::make_unique<AnalysisConsumer>(pp, outDir, opts, plugins);
+ return llvm::make_unique<AnalysisConsumer>(
+ CI.getPreprocessor(), CI.getFrontendOpts().OutputFile,
+ CI.getAnalyzerOpts(), CI.getFrontendOpts().Plugins,
+ new ModelInjector(CI));
}
We can query 'opts' to see if model-path is empty; if it is we can pass nullptr
instead of 'new ModelInjector(CI)'.
> On Aug 12, 2014, at 1:22 AM, Gábor Horváth <[email protected]> wrote:
>
> Hi Ted,
>
> Thank you for the review, I have altered the patch accordingly, and also
> added the patch to follow up the API change in clang tidy.
>
>
> On 12 August 2014 08:44, Ted Kremenek <[email protected]
> <mailto:[email protected]>> wrote:
>
>> On Aug 1, 2014, at 1:44 AM, Gábor Horváth <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>> Hi Ted,
>>
>> Thank you for the review.
>>
>> On 1 August 2014 07:25, Ted Kremenek <[email protected]
>> <mailto:[email protected]>> wrote:
>> Hi Gábor,
>>
>> This is looking good to me. Some minor nits/comments:
>>
>> - Please add doxygen comments for the CodeInjector class.
>>
>> Done.
>>
>> - For the BugReporter patch, are there tests for that functionality change?
>> I saw tests in the other patch, but not that one. It's fine to separate the
>> review of that change before the primary change goes in, but I was curious.
>>
>> Well, it may be a bit complicated. I deleted some code in BugReporter to not
>> to discard bug reports that are in a model file, and the plist part of the
>> test case only pass if that patch is applied (if the patch is not applied
>> the nullpointer dereference warning that has the position in the modelfile
>> will be discarded. In the long term it would be better to report these
>> errors elsewhere but it is not supported yet by the bugreporter patch). I
>> can move the plist check into a separate testcase and add that case to the
>> BugReporter patch instead. The division by zero test should work without the
>> BugReporter patch.
>
> Ok, this make sense. Can you clarify what you mean by "better to report
> these errors elsewhere"?
>
>
> It might be confusing for the user, if the execution path of the bug report
> contains a location that is inside a model file so the report contains codes
> and files that are not present in the analysed project. So it might be more
> user friendly if the locations that are inside a model file would be excluded
> from the reported execution path, however if this is not an issue I am ok
> with reporting those locations as well.
>
>>
>> - As for breaking code in the 'extra' repository, LLVM-internal API is not
>> sacrosanct. If we break the 'extra' projects we just need to update them,
>> but I'm not certain if that is possible in this case.
>>
>> As far as I can remember it would be a straightforward fix in the extra
>> repository. Clang-tidy calls CreateAnalysisConsumer.
>
> Sounds good. Let's get the right API and just fix up clang-tidy.
>
>>
>> - For comments, please consistently use sentence casing and end with
>> periods, and for type names use the appropriate casing. For example:
>>
>> + // modules create a separate compilerinstance for parsing modules, maybe
>> it is
>> + // for reason so I mimic this behavior
>> + CompilerInstance Instance;
>> ...
>>
>> This comment looks a bit suspect, since it seems like a question to
>> yourself. Here you use the word "I"; who is "I" in the context of this
>> code? The comment also seems like an unanswered question. Is this a stale
>> comment?
>>
>>
>> Done, the comment was improved.
>>
>> Another example is this comment:
>>
>> + // FIXME: double memoization is redundant. Here and in bodyfarm.
>> + llvm::StringMap<Stmt *> Bodies;
>>
>> This can be made slightly cleaner. For example:
>>
>> + // FIXME: Double memorization is redundant, with
>> + /// memoization both here and in BodyFarm.
>> + llvm::StringMap<Stmt *> Bodies;
>>
>> Done.
>>
>> - Only use doxygen comments for documentation. For example:
>>
>> + if (notzero_notmodeled(p)) {
>> + /// There is no information about the value of p, because
>> + /// notzero_notmodeled is not modeled and the function definition
>> + /// is not available.
>> + int j = 5 / p; // expected-warning {{Division by zero}}
>> + }
>>
>> In this case we should use '//', not '///'. The former are true comments,
>> and the latter are candidates to be extracted for documentation.
>>
>>
>> Done.
>>
>> Overall, however, this is getting really close.
>>
>>
>> It is great.
>>
>> Thanks,
>> Gábor
>
> Wonderful. The rest of my comments are minor:
>
>> +/// \brief CodeInjector is an interface which is responsible forinjecting
>> AST of
>> +/// function definitions that may not be available in the original source.
>> +///
>> +/// The getBody function will be called each time the static analyzer
>> examines a
>> +/// function call that has no definition available in the current
>> translation
>> +/// unit. If the returned statement is not a nullpointer, it is assumed to
>> be
>> +/// the body of a function which will be used for the analysis. The source
>> of
>> +/// the body can be arbitrary, but it is advised to use memoization to avoid
>> +/// unnecessary reparsing of the external source that provides the body of
>> the
>> +/// functions.
>
>
> "forinjecting" -> "for injecting"
> "nullpointer" -> "null pointer"
>
>> +++ include/clang/StaticAnalyzer/Frontend/FrontendActions.h (working copy)
>> @@ -10,10 +10,16 @@
>> #ifndef LLVM_CLANG_GR_FRONTENDACTIONS_H
>> #define LLVM_CLANG_GR_FRONTENDACTIONS_H
>>
>> +#include <map>
>> +
>
>
> This "#include" of <map> doesn't seem needed. Neither is the one in
> ModelConsumer.h
>
>> +++ lib/StaticAnalyzer/Frontend/ModelConsumer.cpp (working copy)
>> @@ -0,0 +1,42 @@
>> +//===--- ModelConsumer.cpp - ASTConsumer for consuming model files
>> --------===//
>> +//
>> +// The LLVM Compiler Infrastructure
>> +//
>> +// This file is distributed under the University of Illinois Open Source
>> +// License. See LICENSE.TXT for details.
>> +//
>> +//===----------------------------------------------------------------------===//
>> +///
>> +/// \file
>> +/// \brief This file implements an ASTConsumer for consuming model files.
>> +///
>> +/// This ASTConsumer handles the AST of a parsed model file. All top level
>> +/// function definitions will be collected from that model file for later
>> +/// retrieval during the static analyzis. The body of these functions will
>> not
>> +/// be injected into the ASTUnit of the analyzed translation unit. It will
>> be
>> +/// available through the BodyFarm which is utilized by the
>> AnalysisDeclContext
>> +/// class.
>> +///
>
> "analyzis" -> "analysis"
>
>> + // The instance wants to take ownership, however disablefree frontend
>> option
>> + // is set to true to avoid double free issues
>
> Use the actual casing for the option for technical precision:
>
> DisableFree
>
>> + /// \brief Synthetize a body for a declaration
>> + ///
>> + /// This method first looks up the appropriate model file based on the
>> + /// model-path configuration option and the name of the declaration that
>> is
>> + /// looked up. If no model were synthetized yet for a function with that
>> name
>> + /// it will create a new compiler instance to parse the model file using
>> the
>> + /// ASTContext, Preprocessor, SourceManager of the original compiler
>> instance.
>> + /// The former resources are shared between the two compiler instance, so
>> the
>> + /// newly created instance have to "leak" these objects, since they are
>> owned
>> + /// by the original instance.
>
> Synthetize -> Synthesize
> synthetized -> synthesized
>
>> + std::vector<std::unique_ptr<ASTUnit> > ModelAsts;
>
> I'd prefer this to be "ModelASTs", as 'AST' is an acronym.
>
>
> It was an unused member (from an earlier implementation) that I forgot to
> remove, but done now.
>
> Otherwise, this all looks great to me.
>
>>
>> Cheers,
>> Ted
>>
>> On Jul 30, 2014, at 3:29 AM, Gábor Horváth <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>>> Hi Ted,
>>>
>>> Thank you for the review.
>>>
>>>
>>> On 30 July 2014 08:18, Ted Kremenek <[email protected]
>>> <mailto:[email protected]>> 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 <[email protected]
>>> <mailto:[email protected]>> wrote:
>>>
>>>>
>>>>
>>>>
>>>> On 14 July 2014 19:32, Anna Zaks <[email protected]
>>>> <mailto:[email protected]>> wrote:
>>>>
>>>>> On Jul 13, 2014, at 6:11 AM, Gábor Horváth <[email protected]
>>>>> <mailto:[email protected]>> 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 <[email protected]
>>>>> <mailto:[email protected]>> 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>
>>>
>>>
>>> <api_modeling.patch><bugreporter.patch>
>>
>>
>> <api_modeling.patch><bugreporter.patch>
>
>
>
> Thanks,
> Gábor
> <api_modeling.patch><bugreporter.patch><clangTidy.patch>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
