[PATCH] D52905: CSA: fix accessing GDM data from shared libraries
jranieri-grammatech added a comment.
In https://reviews.llvm.org/D52905#1257040, @NoQ wrote:
> Hmmm, interesting. A checker doesn't usually need to access these specific
> static locals, at least not directly. These are usually accessed through
> functions in .cpp files that are supposed to be compiled with a pointer to
> the correct instance of the static local, and it doesn't seem to be necessary
> to expose them to plugins, simply because i don't immediately see why would a
> plugin want to use them. In this sense, i believe that the entire definition
> of these traits should be moved to .cpp files and be made private, accessed
> only via public methods of respective classes. But i guess it's more
> difficult and it's a separate chunk of work, so i totally approve this patch.
My current goal is to examine all of the information the analyzer has available
at callsites and extract the portions that the project I'm working on might be
interested in. I wouldn't disagree with your assessment in general, but it's
definitely not something I have time allocated towards doing.
> Also, i guess that's what they meant when they were saying that
> REGISTER_TRAIT_WITH_PROGRAMSTATE [and similar] macros shouldn't be used in
> headers (https://reviews.llvm.org/D51388?id=162968#inline-455495).
Yeah, I think so.
> Did you use any sort of tool to find those? I.e., are there more such bugs,
> and can we prevent this from regressing and breaking your workflow later?
I wrote a very quick and dirty clang plugin that has this AST matcher:
returnStmt(hasReturnValue(ignoringImplicit(unaryOperator(
hasOperatorName("&"),
hasUnaryOperand(declRefExpr(
to(varDecl(isStaticLocal(,
isExpansionInFileMatching("/include/.*\\.h"))
where `isStaticLocal` is defined as:
AST_MATCHER(VarDecl, isStaticLocal) {
return Node.isStaticLocal();
}
I've since adapted it to a clang-tidy checker under the llvm 'module', which
I'll aim at getting approval to open source as well. Do you know if there's a
way to run clang-tidy over all of clang+llvm automatically? My plugin approach
had the advantage of just needing to fiddle with CMAKE_CXX_FLAGS to run against
the whole codebase...
> You didn't add reviewers. Does it mean that you are still planning to work on
> this patch further, or do you want this patch to be committed in its current
> shape? Static Analyzer patches are usually prefixed with [analyzer] (a few
> people auto-subscribe to those) and please feel free to add me and
> @george.karpenkov as reviewers, and the code owner is @dcoughlin.
This is just my inexperience with the Phabricator patch submission process
showing through; I've traditionally emailed patches to the various -dev lists.
Repository:
rC Clang
https://reviews.llvm.org/D52905
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52905: CSA: fix accessing GDM data from shared libraries
NoQ added subscribers: george.karpenkov, dcoughlin, NoQ. NoQ added a comment. Hmmm, interesting. A checker doesn't usually need to access these specific static locals, at least not directly. These are usually accessed through functions in .cpp files that are supposed to be compiled with a pointer to the correct instance of the static local, and it doesn't seem to be necessary to expose them to plugins, simply because i don't immediately see why would a plugin want to use them. In this sense, i believe that the entire definition of these traits should be moved to .cpp files and be made private, accessed only via public methods of respective classes. But i guess it's more difficult and it's a separate chunk of work, so i totally approve this patch. Also, i guess that's what they meant when they were saying that REGISTER_TRAIT_WITH_PROGRAMSTATE [and similar] macros shouldn't be used in headers (https://reviews.llvm.org/D51388?id=162968#inline-455495). Did you use any sort of tool to find those? I.e., are there more such bugs, and can we prevent this from regressing and breaking your workflow later? You didn't add reviewers. Does it mean that you are still planning to work on this patch further, or do you want this patch to be committed in its current shape? Static Analyzer patches are usually prefixed with [analyzer] (a few people auto-subscribe to those) and please feel free to add me and @george.karpenkov as reviewers, and the code owner is @dcoughlin. Repository: rC Clang https://reviews.llvm.org/D52905 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52905: CSA: fix accessing GDM data from shared libraries
jranieri-grammatech created this revision.
Herald added subscribers: cfe-commits, mgorny.
The `GDMIndex` functions return a pointer that's used as a key for looking up
data, but addresses of local statics defined in header files aren't the same
across shared library boundaries and the result is that analyzer plugins can't
access this data.
Repository:
rC Clang
https://reviews.llvm.org/D52905
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
include/clang/StaticAnalyzer/Core/PathSensitive/TaintManager.h
lib/StaticAnalyzer/Core/CMakeLists.txt
lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
lib/StaticAnalyzer/Core/TaintManager.cpp
Index: lib/StaticAnalyzer/Core/TaintManager.cpp
===
--- lib/StaticAnalyzer/Core/TaintManager.cpp
+++ lib/StaticAnalyzer/Core/TaintManager.cpp
@@ -0,0 +1,23 @@
+//== TaintManager.cpp -- -*- C++ -*--=//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Core/PathSensitive/TaintManager.h"
+
+using namespace clang;
+using namespace ento;
+
+void *ProgramStateTrait::GDMIndex() {
+ static int index = 0;
+ return
+}
+
+void *ProgramStateTrait::GDMIndex() {
+ static int index;
+ return
+}
Index: lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
@@ -200,6 +200,11 @@
}
}
+void *ProgramStateTrait::GDMIndex() {
+ static int Index;
+ return
+}
+
} // end of namespace ento
} // end of namespace clang
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3106,3 +3106,8 @@
llvm::errs() << "Warning: dumping graph requires assertions" << "\n";
return "";
}
+
+void *ProgramStateTrait::GDMIndex() {
+ static int index = 0;
+ return
+}
Index: lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
===
--- lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
+++ lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
@@ -77,5 +77,10 @@
}
}
+void *ProgramStateTrait::GDMIndex() {
+ static int index = 0;
+ return
+}
+
} // namespace ento
} // namespace clang
Index: lib/StaticAnalyzer/Core/CMakeLists.txt
===
--- lib/StaticAnalyzer/Core/CMakeLists.txt
+++ lib/StaticAnalyzer/Core/CMakeLists.txt
@@ -52,6 +52,7 @@
Store.cpp
SubEngine.cpp
SymbolManager.cpp
+ TaintManager.cpp
WorkList.cpp
Z3ConstraintManager.cpp
Index: include/clang/StaticAnalyzer/Core/PathSensitive/TaintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/TaintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/TaintManager.h
@@ -34,10 +34,7 @@
template<> struct ProgramStateTrait
: public ProgramStatePartialTrait {
- static void *GDMIndex() {
-static int index = 0;
-return
- }
+ static void *GDMIndex();
};
/// The GDM component mapping derived symbols' parent symbols to their
@@ -49,10 +46,7 @@
template<> struct ProgramStateTrait
: public ProgramStatePartialTrait {
- static void *GDMIndex() {
-static int index;
-return
- }
+ static void *GDMIndex();
};
class TaintManager {
Index: include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
@@ -131,7 +131,7 @@
template <>
struct ProgramStateTrait
: public ProgramStatePartialTrait {
- static void *GDMIndex() { static int Index; return }
+ static void *GDMIndex();
};
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -832,7 +832,7 @@
template <>
struct ProgramStateTrait :
public ProgramStatePartialTrait {
- static void *GDMIndex() { static int index = 0; return }
+ static void *GDMIndex();
};
} // namespace
