[cfe-commits] r164339 - in /cfe/trunk: include/clang/Analysis/ include/clang/StaticAnalyzer/Core/ include/clang/StaticAnalyzer/Core/BugReporter/ include/clang/StaticAnalyzer/Core/PathSensitive/ lib/Analysis/ lib/StaticAnalyzer/Core/ test/Analysis/
Jordan Rose
jordan_rose at apple.com
Thu Sep 20 19:24:49 PDT 2012
Cool! Some comments:
On Sep 20, 2012, at 17:09 , Ted Kremenek <kremenek at apple.com> wrote:
>
> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h?rev=164339&r1=164338&r2=164339&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h (original)
> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h Thu Sep 20 19:09:11 2012
> @@ -20,6 +20,7 @@
> #include "clang/AST/DeclCXX.h"
> #include "clang/AST/ExprCXX.h"
> #include "clang/AST/ExprObjC.h"
> +#include "clang/Analysis/AnalysisContext.h"
> #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
> #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
> #include "llvm/ADT/PointerIntPair.h"
> @@ -191,7 +192,8 @@
>
> /// \brief Returns the definition of the function or method that will be
> /// called.
> - virtual RuntimeDefinition getRuntimeDefinition() const = 0;
> + virtual RuntimeDefinition
> + getRuntimeDefinition(AnalysisDeclContextManager &M) const = 0;
This feels ugly to me. Is it okay if the CallEvent grovels through the state -> state manager -> etc chain to get it instead?
> +static BodyFarm &getBodyFarm(ASTContext &C) {
> + static BodyFarm *BF = new BodyFarm(C);
> + return *BF;
> +}
This seems like a disaster waiting to happen if we ever use the same process to run the analyzer over multiple translation units (i.e. multiple ASTContexts). Can this just live on the AnalysisDeclContextManager? That lasts for the entire analysis action and probably already implicitly depends on being per-ASTContext.
> Stmt *AnalysisDeclContext::getBody() const {
> - if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
> - return FD->getBody();
> + if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
> + Stmt *Body = FD->getBody();
> + if (!Body && Manager && Manager->synthesizeBodies())
> + return getBodyFarm(getASTContext()).getBody(FD);
> + return Body;
> + }
> else if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D))
> return MD->getBody();
> else if (const BlockDecl *BD = dyn_cast<BlockDecl>(D))
Is there any reason why this doesn't just use the BodyFarm for all bodies? We may not be using it now, but that's no reason not to make it flexible from the beginning.
> }
>
> AnalysisDeclContext *AnalysisDeclContextManager::getContext(const Decl *D) {
> + if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
> + FD->hasBody(FD);
> + D = FD;
> + }
So there is apparently a Decl::getBody that does the right thing (i.e. returns 0 if there is no body). Can we just use that?
> + // Everything checks out. Create a fake body that just calls the block.
> + // This is basically just an AST dump of:
> + //
> + // void dispatch_sync(dispatch_queue_t queue, void (^block)(void)) {
> + // block();
> + // }
> + //
> + DeclRefExpr *DR = DeclRefExpr::CreateEmpty(C, false, false, false, false);
> + DR->setDecl(const_cast<ParmVarDecl*>(PV));
> + DR->setValueKind(VK_LValue);
> + ImplicitCastExpr *ICE = ImplicitCastExpr::Create(C, Ty, CK_LValueToRValue,
> + DR, 0, VK_RValue);
> + CallExpr *CE = new (C) CallExpr(C, ICE, ArrayRef<Expr*>(), C.VoidTy,
> + VK_RValue, SourceLocation());
> + return CE;
> +}
The name for ArrayRef<Expr*>() is
It is probably not a good idea to make a function body that isn't a CompoundStmt or CXXTryStmt. Right now I don't think anything depends on it besides the diagnostics, which don't apply here, but still.
> +Stmt *BodyFarm::getBody(const FunctionDecl *D) {
> + D = D->getCanonicalDecl();
> +
> + llvm::Optional<Stmt *> &Val = Bodies[D];
> + if (Val.hasValue())
> + return Val.getValue();
> +
> + Val = 0;
Rather than using llvm::Optional<Stmt *>, which you immediately set to NULL, you can just use a map of Stmts and DenseMap::find(). That will still tell you if a value is present or not, and lets you insert the value efficiently if it isn't. It's a little clunkier code-wise, but it saves a bool in every value.
> + if (D->getIdentifier() == 0)
> + return 0;
> +
> + StringRef Name = D->getName();
> + if (Name.empty())
> + return 0;
This check isn't really necessary. Any function with a simple identifier name will actually have a non-empty name. Even if an empty function name happens to slip past, it still won't match in the StringSwitch.
> + FunctionFarmer FF =
> + llvm::StringSwitch<FunctionFarmer>(Name)
> + .Case("dispatch_sync", create_dispatch_sync)
> + .Default(NULL);
> +
> + if (FF) {
> + Val = FF(C, D);
> + }
Another early return here?
if (!FF)
return 0;
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp?rev=164339&r1=164338&r2=164339&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp Thu Sep 20 19:09:11 2012
> @@ -121,7 +121,8 @@
> /// Recursively scan through a path and prune out calls and macros pieces
> /// that aren't needed. Return true if afterwards the path contains
> /// "interesting stuff" which means it should be pruned from the parent path.
> -bool BugReporter::RemoveUneededCalls(PathPieces &pieces, BugReport *R) {
> +bool BugReporter::RemoveUneededCalls(PathPieces &pieces, BugReport *R,
> + PathDiagnosticCallPiece *CallWithLoc) {
> bool containsSomethingInteresting = false;
> const unsigned N = pieces.size();
>
> @@ -131,6 +132,11 @@
> IntrusiveRefCntPtr<PathDiagnosticPiece> piece(pieces.front());
> pieces.pop_front();
>
> + // Throw away pieces with invalid locations.
> + if (piece->getKind() != PathDiagnosticPiece::Call &&
> + piece->getLocation().asLocation().isInvalid())
> + continue;
This .asLocation().isInvalid() would be nice to have in a helper method. This isn't the last time you use it.
> switch (piece->getKind()) {
> case PathDiagnosticPiece::Call: {
> PathDiagnosticCallPiece *call = cast<PathDiagnosticCallPiece>(piece);
> @@ -142,8 +148,17 @@
> }
> // Recursively clean out the subclass. Keep this call around if
> // it contains any informative diagnostics.
> - if (!RemoveUneededCalls(call->path, R))
> + PathDiagnosticCallPiece *NewCallWithLoc =
> + call->getLocation().asLocation().isValid()
> + ? call : CallWithLoc;
I think there should actually be an assertion here -- only inlined functions can be synthesized, so there will always be a non-synthesized call with a valid location.
> + if (!RemoveUneededCalls(call->path, R, NewCallWithLoc))
> continue;
> +
> + if (NewCallWithLoc == CallWithLoc && CallWithLoc) {
> + call->callEnter = CallWithLoc->callEnter;
> + }
Extra braces? Also, when does callExit get set?
> Modified: cfe/trunk/test/Analysis/unix-fns.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/unix-fns.c?rev=164339&r1=164338&r2=164339&view=diff
> ==============================================================================
> --- cfe/trunk/test/Analysis/unix-fns.c (original)
> +++ cfe/trunk/test/Analysis/unix-fns.c Thu Sep 20 19:09:11 2012
> @@ -1,4 +1,5 @@
> -// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -analyze -analyzer-checker=unix.API,osx.API %s -analyzer-store=region -fblocks -verify
> +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -analyze -analyzer-checker=core,unix.API,osx.API %s -analyzer-store=region -analyzer-output=plist -analyzer-ipa=inlining -analyzer-eagerly-assume -analyzer-config faux-bodies=true -fblocks -verify -o %t.plist
> +// RUN: FileCheck --input-file=%t.plist %s
Please add an -analyzer-output=text version of this test, and use that for -verify. I know it's imprecise, but it's easier to tell what changes when we change things.
> struct _opaque_pthread_once_t {
> long __sig;
> @@ -15,9 +16,27 @@
> void *alloca(size_t);
> void *valloc(size_t);
>
> +typedef union {
> + struct _os_object_s *_os_obj;
> + struct dispatch_object_s *_do;
> + struct dispatch_continuation_s *_dc;
> + struct dispatch_queue_s *_dq;
> + struct dispatch_queue_attr_s *_dqa;
> + struct dispatch_group_s *_dg;
> + struct dispatch_source_s *_ds;
> + struct dispatch_source_attr_s *_dsa;
> + struct dispatch_semaphore_s *_dsema;
> + struct dispatch_data_s *_ddata;
> + struct dispatch_io_s *_dchannel;
> + struct dispatch_operation_s *_doperation;
> + struct dispatch_disk_s *_ddisk;
> +} dispatch_object_t __attribute__((__transparent_union__));
> +
> typedef void (^dispatch_block_t)(void);
> typedef long dispatch_once_t;
> +typedef struct dispatch_queue_s *dispatch_queue_t;
> void dispatch_once(dispatch_once_t *predicate, dispatch_block_t block);
> +void dispatch_sync(dispatch_queue_t queue, dispatch_block_t block);
>
> #ifndef O_CREAT
> #define O_CREAT 0x0200
> @@ -151,3 +170,1419 @@
> dispatch_once_t pred = 0;
> dispatch_once(&pred, ^(){}); // expected-warning {{Call to 'dispatch_once' uses the local variable 'pred' for the predicate value}}
> }
> +
> +// Test inlining of dispatch_sync.
> +void test_dispatch_sync(dispatch_queue_t queue, int *q) {
> + int *p = 0;
> + dispatch_sync(queue, ^(void){
> + if (q) {
> + *p = 1; // expected-warning {{null pointer}}
> + }
> + });
> +}
Very cool. :-)
More information about the cfe-commits
mailing list