[PATCH] D48941: [ASTImporter] import FunctionDecl end locations
a_sidorin accepted this revision. a_sidorin added a comment. LGTM too. Thank you! Repository: rC Clang https://reviews.llvm.org/D48941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48628: [AST] Structural equivalence of methods
a_sidorin added inline comments.
Comment at: unittests/AST/StructuralEquivalenceTest.cpp:489
+
+TEST_F(StructuralEquivalenceRecordTest, DISABLED_Methods) {
+ auto t = makeNamedDecls(
balazske wrote:
> a_sidorin wrote:
> > Could you add a comment why this test is disabled?
> Methods are not checked, there was no decision about to include this check or
> not. The problem was related to performance overhead and if order-independent
> check of methods is needed. (ASTImporter should keep order of imported fields
> and methods.) (This test is about equivalence of `foo`.)
You mean that imported decl have other order of methods? Do you mean implicit
methods (because I see only a single method here)? If so, could you please note
this in the comment?
Repository:
rC Clang
https://reviews.llvm.org/D48628
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47632: [ASTImporter] Refactor Decl creation
a_sidorin added a comment.
Hi Gabor,
I like the new syntax. There are some comments inline; most of them are just
stylish.
Comment at: lib/AST/ASTImporter.cpp:94
+ void updateAttrAndFlags(const Decl *From, Decl *To) {
+// check if some flags or attrs are new in 'From' and copy into 'To'
+// FIXME: other flags or attrs?
Comments should be complete sentences: start with a capital and end with a
period.
Comment at: lib/AST/ASTImporter.cpp:114
+
+// Always use theses functions to create a Decl during import. There are
+// certain tasks which must be done after the Decl was created, e.g. we
these?
Comment at: lib/AST/ASTImporter.cpp:161
+ ToD->IdentifierNamespace = FromD->IdentifierNamespace;
+ if (FromD->hasAttrs())
+for (const Attr *FromAttr : FromD->getAttrs())
Should we move the below operations into `updateAttrAndFlags()` and use it
instead?
Comment at: lib/AST/ASTImporter.cpp:1587
StructuralEquivalenceContext Ctx(
Importer.getFromContext(), Importer.getToContext(),
+ Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
(Thinking out loud) We need to introduce some method to return
`StructuralEquivalenceContext` in ASTImporter. But this is an item for a
separate patch, not this one.
Comment at: lib/AST/ASTImporter.cpp:1890
TypedefNameDecl *ToTypedef;
- if (IsAlias)
-ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC, StartL, Loc,
- Name.getAsIdentifierInfo(), TInfo);
- else
-ToTypedef = TypedefDecl::Create(Importer.getToContext(), DC,
-StartL, Loc,
-Name.getAsIdentifierInfo(),
-TInfo);
+ if (IsAlias && GetImportedOrCreateDecl(
+ ToTypedef, D, Importer.getToContext(), DC, StartL, Loc,
This is not strictly equivalent to the source condition. If
GetImportedOrCreateDecl returns false, we'll fall to the `else` branch, and it
doesn't seem correct to me.
Comment at: lib/AST/ASTImporter.cpp:4274
+ TemplateParams))
+return ToD;
+ return ToD;
Can we just ignore the return value by casting it to void here and in similar
cases?
Comment at: lib/AST/ASTStructuralEquivalence.cpp:895
+ // If any of the records has external storage and we do a minimal check (or
+ // AST import) we assmue they are equivalent. (If we didn't have this
+ // assumption then `RecordDecl::LoadFieldsFromExternalStorage` could trigger
assume
Repository:
rC Clang
https://reviews.llvm.org/D47632
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49059: [MinGW] Skip adding default win32 api libraries if -lwindowsapp is specified
mstorsjo created this revision.
mstorsjo added reviewers: rnk, martell, compnerd, smeenai.
In this setup, skip adding all the default windows import libraries, if linking
to windowsapp (which replaces them, when targeting the windows store/UWP api
subset).
With GCC, the same is achieved by using a custom spec file, but since clang
doesn't use spec files, we have to allow other means of overriding what default
libraries to use (without going all the way to using -nostdlib, which would
exclude everything). The same approach, in detecting certain user specified
libraries and omitting others from the defaults, was already used in SVN
r314138.
Repository:
rC Clang
https://reviews.llvm.org/D49059
Files:
lib/Driver/ToolChains/MinGW.cpp
test/Driver/mingw-windowsapp.c
Index: test/Driver/mingw-windowsapp.c
===
--- /dev/null
+++ test/Driver/mingw-windowsapp.c
@@ -0,0 +1,6 @@
+// RUN: %clang -v -target i686-pc-windows-gnu -### %s 2>&1 | FileCheck
-check-prefix=CHECK_DEFAULT %s
+// RUN: %clang -v -target i686-pc-windows-gnu -### %s -lwindowsapp 2>&1 |
FileCheck -check-prefix=CHECK_WINDOWSAPP %s
+
+// CHECK_DEFAULT: "-lmsvcrt" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32"
"-lmingw32"
+// CHECK_WINDOWSAPP: "-lwindowsapp" "-lmingw32"
+// CHECK_WINDOWSAPP-SAME: "-lmsvcrt" "-lmingw32"
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -201,6 +201,11 @@
CmdArgs.push_back("-Bdynamic");
}
+ bool HasWindowsApp = false;
+ for (auto Lib : Args.getAllArgValues(options::OPT_l))
+if (Lib == "windowsapp")
+ HasWindowsApp = true;
+
if (!Args.hasArg(options::OPT_nostdlib)) {
if (!Args.hasArg(options::OPT_nodefaultlibs)) {
if (Args.hasArg(options::OPT_static))
@@ -223,15 +228,17 @@
if (Args.hasArg(options::OPT_pthread))
CmdArgs.push_back("-lpthread");
- // add system libraries
- if (Args.hasArg(options::OPT_mwindows)) {
-CmdArgs.push_back("-lgdi32");
-CmdArgs.push_back("-lcomdlg32");
+ if (!HasWindowsApp) {
+// add system libraries
+if (Args.hasArg(options::OPT_mwindows)) {
+ CmdArgs.push_back("-lgdi32");
+ CmdArgs.push_back("-lcomdlg32");
+}
+CmdArgs.push_back("-ladvapi32");
+CmdArgs.push_back("-lshell32");
+CmdArgs.push_back("-luser32");
+CmdArgs.push_back("-lkernel32");
}
- CmdArgs.push_back("-ladvapi32");
- CmdArgs.push_back("-lshell32");
- CmdArgs.push_back("-luser32");
- CmdArgs.push_back("-lkernel32");
if (Args.hasArg(options::OPT_static))
CmdArgs.push_back("--end-group");
Index: test/Driver/mingw-windowsapp.c
===
--- /dev/null
+++ test/Driver/mingw-windowsapp.c
@@ -0,0 +1,6 @@
+// RUN: %clang -v -target i686-pc-windows-gnu -### %s 2>&1 | FileCheck -check-prefix=CHECK_DEFAULT %s
+// RUN: %clang -v -target i686-pc-windows-gnu -### %s -lwindowsapp 2>&1 | FileCheck -check-prefix=CHECK_WINDOWSAPP %s
+
+// CHECK_DEFAULT: "-lmsvcrt" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32" "-lmingw32"
+// CHECK_WINDOWSAPP: "-lwindowsapp" "-lmingw32"
+// CHECK_WINDOWSAPP-SAME: "-lmsvcrt" "-lmingw32"
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -201,6 +201,11 @@
CmdArgs.push_back("-Bdynamic");
}
+ bool HasWindowsApp = false;
+ for (auto Lib : Args.getAllArgValues(options::OPT_l))
+if (Lib == "windowsapp")
+ HasWindowsApp = true;
+
if (!Args.hasArg(options::OPT_nostdlib)) {
if (!Args.hasArg(options::OPT_nodefaultlibs)) {
if (Args.hasArg(options::OPT_static))
@@ -223,15 +228,17 @@
if (Args.hasArg(options::OPT_pthread))
CmdArgs.push_back("-lpthread");
- // add system libraries
- if (Args.hasArg(options::OPT_mwindows)) {
-CmdArgs.push_back("-lgdi32");
-CmdArgs.push_back("-lcomdlg32");
+ if (!HasWindowsApp) {
+// add system libraries
+if (Args.hasArg(options::OPT_mwindows)) {
+ CmdArgs.push_back("-lgdi32");
+ CmdArgs.push_back("-lcomdlg32");
+}
+CmdArgs.push_back("-ladvapi32");
+CmdArgs.push_back("-lshell32");
+CmdArgs.push_back("-luser32");
+CmdArgs.push_back("-lkernel32");
}
- CmdArgs.push_back("-ladvapi32");
- CmdArgs.push_back("-lshell32");
- CmdArgs.push_back("-luser32");
- CmdArgs.push_back("-lkernel32");
if (Args.hasArg(options::OPT_static))
CmdArgs.push_back("--end-group");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
NoQ added a comment.
Much symbols!
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+State = State->set(ObjRegion, NewSet);
rnkovacs wrote:
> xazax.hun wrote:
> > Is it possible here the set to be empty? We just added a new element to it
> > above. Maybe turn this into an assert or just omit this if it is impossible?
> I'm not sure whether `add()` can fail. I turned it into an assert now and
> will see if it ever fails.
It totally can't. We're just adding an object to a set. What could possibly go
wrong?
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:32-40
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+ : public ProgramStatePartialTrait {
+ static void *GDMIndex() {
+static int Index = 0;
+return
Please add a comment on how this template is useful. This trick is used by some
checkers, but it's a vry unobvious trick. We should probably add a macro
for that, i.e. `REGISTER_FACTORY_WITH_PROGRAMSTATE` or something like that.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:113
- const SymbolRef *StrBufferPtr = State->get(TypedR);
- // FIXME: What if Origin is null?
const Expr *Origin = Call.getOriginExpr();
Maybe turn the FIXME into a NOTE or something like that. It indeed seems fine,
but let's still make sure the reader realizes that there's a subtle potential
problem here.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:115
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
+ // Start tracking this raw pointer by adding it to the set of symbols
I'd much rather unwrap the symbol here, i.e. `if (SymbolRef Sym =
RawPtr.getAsSymbol())`. A lot of other cornercases may occur if the
implementation is accidentally inlined (`Undefined`, concrete regions). Also,
speculatively, `.getAsSymbol(/* search for symbolic base = */ true)` for the
same reason//*//.
If we didn't care about inlined implementations, the `Unknown` check would have
been redundant. So it should also be safe to straightforwardly ignore inlined
implementations by consulting `C.wasInlined`, then the presence of the symbol
can be asserted. But i'd rather speculatively care about inlined
implementations as long as it seems easy.
__
//*// In fact your code relies on a very wonky implementation detail of our
`SVal` hierarchy: namely, pointer-type return values of conservatively
evaluated functions are always expressed as `{conj_$N}`
and never as `{SymRegion{conj_$N}, 0 S32b, pointee
type}`. Currently nobody knows the rules under which zero element regions are
added in different parts of the analyzer, i.e. what is the "canonical"
representation of the symbolic pointer, though i made a few attempts to
speculate.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:119
+ PtrSet::Factory = State->getStateManager().get_context();
+ PtrSet NewSet = F.getEmptySet();
+ if (const PtrSet *OldSet = State->get(ObjRegion)) {
My favorite idiom for such cases, looks a bit cleaner:
```
const PtrSet *SetPtr = State->get(ObjRegion)
PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet()
```
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:122
+NewSet = *OldSet;
+if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
This `contains` check is very unlikely to yield true because if the
implementation is not inlined, the symbol is newly born. I'd much rather skip
the check; it doesn't sound like it'd make things any faster.
Even better, let's assert that: `assert(C.wasInlined || !Set.contains(Sym))`.
It'll probably help us catch some "reincarnated symbol" bugs in the core.
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49058: [analyzer] Move DanglingInternalBufferChecker out of alpha
rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet,
baloghadamsoftware, whisperity.
Repository:
rC Clang
https://reviews.llvm.org/D49058
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -276,6 +276,11 @@
let ParentPackage = Cplusplus in {
+def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
+ HelpText<"Reports the usage of internal raw pointers of C++ containers "
+ "after invalidation.">,
+ DescFile<"DanglingInternalBufferChecker.cpp">;
+
def NewDeleteChecker : Checker<"NewDelete">,
HelpText<"Check for double-free and use-after-free problems. Traces memory
managed by new/delete.">,
DescFile<"MallocChecker.cpp">;
@@ -300,11 +305,6 @@
let ParentPackage = CplusplusAlpha in {
-def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
- HelpText<"Check for internal raw pointers of C++ standard library containers
"
- "used after deallocation">,
- DescFile<"DanglingInternalBufferChecker.cpp">;
-
def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
HelpText<"Reports destructions of polymorphic objects with a non-virtual "
"destructor in their base class">,
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -276,6 +276,11 @@
let ParentPackage = Cplusplus in {
+def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
+ HelpText<"Reports the usage of internal raw pointers of C++ containers "
+ "after invalidation.">,
+ DescFile<"DanglingInternalBufferChecker.cpp">;
+
def NewDeleteChecker : Checker<"NewDelete">,
HelpText<"Check for double-free and use-after-free problems. Traces memory managed by new/delete.">,
DescFile<"MallocChecker.cpp">;
@@ -300,11 +305,6 @@
let ParentPackage = CplusplusAlpha in {
-def DanglingInternalBufferChecker : Checker<"DanglingInternalBuffer">,
- HelpText<"Check for internal raw pointers of C++ standard library containers "
- "used after deallocation">,
- DescFile<"DanglingInternalBufferChecker.cpp">;
-
def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
HelpText<"Reports destructions of polymorphic objects with a non-virtual "
"destructor in their base class">,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:121
+ if (State->contains(ObjRegion)) {
+NewSet = *State->get(ObjRegion);
+if (NewSet.contains(RawPtr.getAsSymbol()))
xazax.hun wrote:
> Oh, there is also a double lookup here, sorry I did not spot it in the
> initial review.
Thanks for noticing. I should have seen it in the first place :)
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs updated this revision to Diff 154520.
rnkovacs marked an inline comment as done.
https://reviews.llvm.org/D49057
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -108,6 +108,30 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
+void multiple_symbols(bool cond) {
+ const char *c1, *d1;
+ {
+std::string s1;
+c1 = s1.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+d1 = s1.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+const char *local = s1.c_str();
+consume(local); // no-warning
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
+ // expected-note@-1 {{Internal buffer is released because the object was destroyed}}
+ std::string s2;
+ const char *c2 = s2.c_str();
+ if (cond) {
+// expected-note@-1 {{Assuming 'cond' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'cond' is 0}}
+// expected-note@-4 {{Taking false branch}}
+consume(c1); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d1); // expected-warning {{Use of memory after it is freed}}
+ } // expected-note@-1 {{Use of memory after it is freed}}
+}
+
void deref_after_scope_cstr_ok() {
const char *c;
std::string s;
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -24,10 +24,22 @@
using namespace clang;
using namespace ento;
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+using PtrSet = llvm::ImmutableSet;
+
+// Associate container objects with a set of raw pointer symbols.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
+
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+ : public ProgramStatePartialTrait {
+ static void *GDMIndex() {
+static int Index = 0;
+return
+ }
+};
+} // end namespace ento
+} // end namespace clang
namespace {
@@ -61,7 +73,7 @@
bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
RawPtrMapTy Map = State->get();
for (const auto Entry : Map) {
-if (Entry.second == Sym)
+if (Entry.second.contains(Sym))
return true;
}
return false;
@@ -88,32 +100,43 @@
return;
SVal Obj = ICall->getCXXThisVal();
- const auto *TypedR = dyn_cast_or_null(Obj.getAsRegion());
- if (!TypedR)
+ const auto *ObjRegion = dyn_cast_or_null(Obj.getAsRegion());
+ if (!ObjRegion)
return;
- auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+ auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
if (TypeDecl->getName() != "basic_string")
return;
ProgramStateRef State = C.getState();
if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
- State = State->set(TypedR, RawPtr.getAsSymbol());
+ // Start tracking this raw pointer by adding it to the set of symbols
+ // associated with this container object in the program state map.
+ PtrSet::Factory = State->getStateManager().get_context();
+ PtrSet NewSet = F.getEmptySet();
+ if (const PtrSet *OldSet = State->get(ObjRegion)) {
+NewSet = *OldSet;
+if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
+ }
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ assert(!NewSet.isEmpty());
+ State = State->set(ObjRegion, NewSet);
C.addTransition(State);
}
return;
}
if (isa(ICall)) {
-if (State->contains(TypedR)) {
- const SymbolRef *StrBufferPtr = State->get(TypedR);
- // FIXME: What if Origin is null?
+if (const PtrSet *PS = State->get(ObjRegion)) {
+ // Mark all pointer symbols associated with the deleted object released.
const Expr *Origin = Call.getOriginExpr();
- State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
- State = State->remove(TypedR);
+ for (const auto Symbol : *PS)
+State = allocation_state::markReleased(State, Symbol, Origin);
+ State = State->remove(ObjRegion);
C.addTransition(State);
return;
}
@@ -123,15
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
xazax.hun added a comment.
Thanks! The changes look good, I forgot to mark one double lookup though in my
previous review.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:121
+ if (State->contains(ObjRegion)) {
+NewSet = *State->get(ObjRegion);
+if (NewSet.contains(RawPtr.getAsSymbol()))
Oh, there is also a double lookup here, sorry I did not spot it in the initial
review.
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+State = State->set(ObjRegion, NewSet);
xazax.hun wrote:
> Is it possible here the set to be empty? We just added a new element to it
> above. Maybe turn this into an assert or just omit this if it is impossible?
I'm not sure whether `add()` can fail. I turned it into an assert now and will
see if it ever fails.
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs updated this revision to Diff 154519.
rnkovacs marked 5 inline comments as done.
rnkovacs edited the summary of this revision.
rnkovacs added a comment.
Addressed comments.
https://reviews.llvm.org/D49057
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -108,6 +108,30 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
+void multiple_symbols(bool cond) {
+ const char *c1, *d1;
+ {
+std::string s1;
+c1 = s1.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+d1 = s1.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+const char *local = s1.c_str();
+consume(local); // no-warning
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
+ // expected-note@-1 {{Internal buffer is released because the object was destroyed}}
+ std::string s2;
+ const char *c2 = s2.c_str();
+ if (cond) {
+// expected-note@-1 {{Assuming 'cond' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'cond' is 0}}
+// expected-note@-4 {{Taking false branch}}
+consume(c1); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d1); // expected-warning {{Use of memory after it is freed}}
+ } // expected-note@-1 {{Use of memory after it is freed}}
+}
+
void deref_after_scope_cstr_ok() {
const char *c;
std::string s;
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -24,10 +24,22 @@
using namespace clang;
using namespace ento;
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+using PtrSet = llvm::ImmutableSet;
+
+// Associate container objects with a set of raw pointer symbols.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
+
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+ : public ProgramStatePartialTrait {
+ static void *GDMIndex() {
+static int Index = 0;
+return
+ }
+};
+} // end namespace ento
+} // end namespace clang
namespace {
@@ -60,8 +72,8 @@
// lookup by region.
bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
RawPtrMapTy Map = State->get();
- for (const auto Entry : Map) {
-if (Entry.second == Sym)
+ for (const auto : Map) {
+if (Entry.second.contains(Sym))
return true;
}
return false;
@@ -88,32 +100,43 @@
return;
SVal Obj = ICall->getCXXThisVal();
- const auto *TypedR = dyn_cast_or_null(Obj.getAsRegion());
- if (!TypedR)
+ const auto *ObjRegion = dyn_cast_or_null(Obj.getAsRegion());
+ if (!ObjRegion)
return;
- auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+ auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
if (TypeDecl->getName() != "basic_string")
return;
ProgramStateRef State = C.getState();
if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
- State = State->set(TypedR, RawPtr.getAsSymbol());
+ // Start tracking this raw pointer by adding it to the set of symbols
+ // associated with this container object in the program state map.
+ PtrSet::Factory = State->getStateManager().get_context();
+ PtrSet NewSet = F.getEmptySet();
+ if (State->contains(ObjRegion)) {
+NewSet = *State->get(ObjRegion);
+if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
+ }
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ assert(!NewSet.isEmpty());
+ State = State->set(ObjRegion, NewSet);
C.addTransition(State);
}
return;
}
if (isa(ICall)) {
-if (State->contains(TypedR)) {
- const SymbolRef *StrBufferPtr = State->get(TypedR);
- // FIXME: What if Origin is null?
+if (const PtrSet *PS = State->get(ObjRegion)) {
+ // Mark all pointer symbols associated with the deleted object released.
const Expr *Origin = Call.getOriginExpr();
- State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
- State = State->remove(TypedR);
+ for (const auto : *PS)
+State =
[PATCH] D48712: [X86] Lowering integer truncation intrinsics to native IR
craig.topper accepted this revision. craig.topper added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D48712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48712: [X86] Lowering integer truncation intrinsics to native IR
RKSimon added a comment. LGTM - @craig.topper any comments? https://reviews.llvm.org/D48712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.
Overall looks good, some nits inline. Let's run it on some projects to exercise
this change.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:27
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+typedef llvm::ImmutableSet PtrSet;
+
I think we should use `using` now instead of `typedef`.
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+State = State->set(ObjRegion, NewSet);
Is it possible here the set to be empty? We just added a new element to it
above. Maybe turn this into an assert or just omit this if it is impossible?
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:138
const Expr *Origin = Call.getOriginExpr();
- State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
- State = State->remove(TypedR);
+ const PtrSet *PS = State->get(ObjRegion);
+ for (const auto : *PS)
Using both contains and get will result in two lookups. Maybe it would be
better to just use get and check if the result is nullptr?
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:159
}
+if (State->contains(Entry.first)) {
+ const PtrSet *OldSet = State->get(Entry.first);
Same comment as above.
Comment at: test/Analysis/dangling-internal-buffer.cpp:111
+void multiple_symbols(bool Cond) {
+ const char *c1, *d1;
We started to use lowercase letters for variable names. Maybe this is not the
best, since it is not following the LLVM coding guidelines. So I think it would
be better to rename this `Cond` to start with a lowercase letter in this patch
for consistency, and update the tests to follow the LLVM coding styleguide in a
separate patch later.
Repository:
rC Clang
https://reviews.llvm.org/D49057
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker
rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet,
baloghadamsoftware, whisperity.
Previously, the checker only tracked one raw pointer symbol for each container
object. But member functions returning a pointer to the object's inner buffer
may be called on the object several times.
These pointer symbols are now collected in a set inside the program state map
and thus all of them is checked for use-after-free problems.
Repository:
rC Clang
https://reviews.llvm.org/D49057
Files:
lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
test/Analysis/dangling-internal-buffer.cpp
Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -108,6 +108,28 @@
// expected-note@-1 {{Use of memory after it is freed}}
}
+void multiple_symbols(bool Cond) {
+ const char *c1, *d1;
+ {
+std::string s1;
+c1 = s1.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+d1 = s1.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+const char *local = s1.c_str();
+consume(local); // no-warning
+ } // expected-note {{Internal buffer is released because the object was destroyed}}
+ // expected-note@-1 {{Internal buffer is released because the object was destroyed}}
+ std::string s2;
+ const char *c2 = s2.c_str();
+ if (Cond) {
+// expected-note@-1 {{Assuming 'Pred' is not equal to 0}} // expected-note@-1 {{Taking true branch}}
+// expected-note@-2 {{Assuming 'Pred' is 0}} // expected-note@-2 {{Taking false branch}}
+consume(c1); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+ } else {
+consume(d1); // expected-warning {{Use of memory after it is freed}}
+ } // expected-note@-1 {{Use of memory after it is freed}}
+}
+
void deref_after_scope_cstr_ok() {
const char *c;
std::string s;
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -24,10 +24,22 @@
using namespace clang;
using namespace ento;
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+typedef llvm::ImmutableSet PtrSet;
+
+// Associate container objects with a set of raw pointer symbols.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
+
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+ : public ProgramStatePartialTrait {
+ static void *GDMIndex() {
+static int Index = 0;
+return
+ }
+};
+} // end namespace ento
+} // end namespace clang
namespace {
@@ -60,8 +72,8 @@
// lookup by region.
bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
RawPtrMapTy Map = State->get();
- for (const auto Entry : Map) {
-if (Entry.second == Sym)
+ for (const auto : Map) {
+if (Entry.second.contains(Sym))
return true;
}
return false;
@@ -88,32 +100,45 @@
return;
SVal Obj = ICall->getCXXThisVal();
- const auto *TypedR = dyn_cast_or_null(Obj.getAsRegion());
- if (!TypedR)
+ const auto *ObjRegion = dyn_cast_or_null(Obj.getAsRegion());
+ if (!ObjRegion)
return;
- auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+ auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
if (TypeDecl->getName() != "basic_string")
return;
ProgramStateRef State = C.getState();
if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
SVal RawPtr = Call.getReturnValue();
if (!RawPtr.isUnknown()) {
- State = State->set(TypedR, RawPtr.getAsSymbol());
- C.addTransition(State);
+ // Start tracking this raw pointer by adding it to the set of symbols
+ // associated with this container object in the program state map.
+ PtrSet::Factory = State->getStateManager().get_context();
+ PtrSet NewSet = F.getEmptySet();
+ if (State->contains(ObjRegion)) {
+NewSet = *State->get(ObjRegion);
+if (NewSet.contains(RawPtr.getAsSymbol()))
+ return;
+ }
+ NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+ if (!NewSet.isEmpty()) {
+State = State->set(ObjRegion, NewSet);
+C.addTransition(State);
+ }
}
return;
}
if (isa(ICall)) {
-if (State->contains(TypedR)) {
- const SymbolRef *StrBufferPtr =
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added inline comments.
Comment at: test/SemaObjC/property-in-class-extension-1.m:18
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note
{{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //
expected-note {{property declared here}}
QF5690 wrote:
> rjmccall wrote:
> > Whoa, why is this test case using `-Weverything`? That seems unnecessarily
> > fragile.
> Do you think it should be relaxed only to warnings that are appearing here?
Yeah, tests should generally only be testing specific categories. Opt-out
warnings are different, of course, but it's understood that adding a new
opt-out warning is an ambitious change.
Repository:
rC Clang
https://reviews.llvm.org/D44539
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
