[PATCH] D37023: [analyzer] Fix bugreporter::getDerefExpr() again.
dcoughlin accepted this revision. dcoughlin added a comment. LGTM! Thanks Artem. https://reviews.llvm.org/D37023 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
dcoughlin added a comment.
This is such a nasty bug! It is great to see a fix. I have two comments inline,
one of which is just a nit.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1404
+ // When trying to dereference a void pointer, read the first byte.
+ T = Ctx.CharTy;
+}
Nit: It seems a bit odd to read the first byte here since (unless I'm
misunderstanding) this would never be triggered by actual C semantics, only by
a checker. Did you consider just returning UnknownVal() in this case?
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1408
}
+assert(!T.isNull() && "Unable to auto-detect binding type!");
+assert(!T->isVoidType() && "Attempted to retrieve a void value!");
I think you missed handling the AllocaRegion case from the old version in your
new version. This means the assert will fire on the following when core.alpha
is enabled:
```
void foo(void *dest) {
void *src = __builtin_alloca(5);
memcpy(dest, src, 1);
}
```
https://reviews.llvm.org/D38358
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.
dcoughlin added a comment.
Artem: Sorry it too me so long to review this! For CXXStdInitializerListExpr
this looks great. However, I don't think we want ObjCBoxedExprs to escape their
arguments. It looks to me like these expressions copy their argument values and
don't hold on to them.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1127
+// only consist of ObjC objects, and escapes of ObjC objects
+// aren't so important (eg., retain count checker ignores them).
+if (isa(Ex) ||
Note that we do have other ObjC checkers that rely on escaping of ObjC objects,
such as the ObjCLoopChecker and ObjCDeallocChecker. I think having the TODO is
great, but I'd like you to remove the the bit about "escapes of ObjC objects
aren't so important".
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1129
+if (isa(Ex) ||
+(isa(Ex) &&
+ cast(Ex)->getSubExpr()->getType()->isRecordType()))
In general, I don't think we want things passed to ObjCBoxedExpr to escape. The
boxed values are copied and encoded when they are boxed, so the boxed
expression doesn't take ownership of them.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1132
+ for (auto Child : Ex->children()) {
+if (!Child)
+ continue;
Is this 'if' needed?
Comment at: test/Analysis/objc-boxing.m:66
+ BoxableStruct bs;
+ bs.str = strdup("dynamic string"); // The duped string shall be owned by val.
+ NSValue *val = @(bs); // no-warning
In this case the duped string is not owned by `val`. NSValue doesn't take
ownership of the string, so this *will* leak and we should warn about it.
https://reviews.llvm.org/D35216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37478: [analyzer] Implement pointer arithmetic on constants
dcoughlin requested changes to this revision.
dcoughlin added a subscriber: zaks.anna.
dcoughlin added a comment.
This revision now requires changes to proceed.
Rafael: Thanks for the patch! @NoQ, @zaks.anna, and I spoke about this off-line
yesterday.
While this patch improves the modeling of pointer arithmetic, we're worried
about losing valuable alarms that rely on the existing behavior.
Here is a case where the analyzer would warn before your patch but doesn't with
it:
void foo() {
int *p = 0;
int q = *(p + 5); // expected-warning {{Dereference of null pointer}}
}
The existing diagnostic machinery relies on the fact that the analyzer treats
`p + 5` as 0 to report the bad dereference. This comes up more often than you
might think because the analyzer is sometimes quite aggressive about promoting
a symbol constrained to 0 to be a concrete value of 0. For example:
void bar(int *p) {
if (p)
return;
int q = *(p + 5); // expected-warning {{Dereference of null pointer}}
}
It would be good to add test cases for these diagnostics!
I think you can preserve the existing (although missing from the test suite)
good diagnostics and still improve the modeling by skipping the
addition/subtraction if the LHS is a concrete int with value 0. Doing so would
be a very minor change to this patch.
Modeling the pointer arithmetic when the LHS is 0 while still keeping the
diagnostics will likely be a more involved effort, with ramifications in
multiple parts of the analyzer. We could discuss that, if you'd like to tackle
it! But it would probably be good for you to get a couple more patches under
your belt before taking that on.
Comment at: test/Analysis/inlining/inline-defensive-checks.c:144
+
+// Intuitively the following tests should all warn about a null dereference,
+// since the object pointer the operations are based on can be null.
The analyzer doesn't warn on these on purpose.
Throughout the analyzer, we have a broad heuristic that says: "if the
programmer compares a pointer to NULL, then the analyzer should explicitly
consider the case that the pointer is NULL". It will perform a case split in
the symbolic execution: one case for when the value is definitely NULL and one
case for one it is definitely not NULL. As a heuristic this works reasonably
well: if the programmer bothered to add a check for NULL then they likely
though the value could be NULL.
However, when context-sensitive analysis via inlining was added, this heuristic
broke down for functions that called other functions with what we call "inlined
defensive checks" or null.
Here is an example:
```
void hasInlinedDefensiveCheck(int *p) {
if (!p)
return;
// Do something useful
}
void foo(int *param) {
hasInlinedDefensiveCheck(param);
*param = 7;
}
```
In this case the warning about `*param = 7` is a false positive from foo's
point of view because foo may have a strong invariant that param is not null;
it doesn't care that hasInlinedDefensiveCheck() may have other callers that
might call it with null. For this reason, we suppress reports about null
pointer dereferences when we can detect an inlined defense check.
https://reviews.llvm.org/D37478
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.
dcoughlin accepted this revision.
dcoughlin added inline comments.
This revision is now accepted and ready to land.
Comment at: test/Analysis/objc-boxing.m:66
+ BoxableStruct bs;
+ bs.str = strdup("dynamic string"); // The duped string shall be owned by val.
+ NSValue *val = @(bs); // no-warning
NoQ wrote:
> dcoughlin wrote:
> > In this case the duped string is not owned by `val`. NSValue doesn't take
> > ownership of the string, so this *will* leak and we should warn about it.
> I mean, the pointer to the raw string is stored inside the `NSValue`, and can
> be used or freed from there. The caller can free this string by looking into
> the `val`, even though `val` itself won't release the pointer (i guess i
> messed up the comment again). From MallocChecker's perspective, this is an
> escape and no-warning. If we free the string in this function, it'd most
> likely cause use-after-free in the caller.
>
> I tested that the string is indeed not strduped during boxing:
>
> **`$ cat test.m`**
> ```
> #import
>
> typedef struct __attribute__((objc_boxable)) {
> const char *str;
> } BoxableStruct;
>
> int main() {
> BoxableStruct bs;
> bs.str = strdup("dynamic string");
> NSLog(@"%p\n", bs.str);
>
> NSValue *val = @(bs);
>
> BoxableStruct bs2;
> [val getValue:&bs2];
> NSLog(@"%p\n", bs2.str);
>
> return 0;
> }
> ```
> **`$ clang test.m -framework Foundation`**
> **`$ ./a.out`**
> ```
> 2017-10-02 17:56:00.004 a.out[17933:1083757] 0x7ffd23407380
> 2017-10-02 17:56:00.004 a.out[17933:1083757] 0x7ffd23407380
> ```
> So it's possible to retrieve the exact same pointer from the boxed value. So
> if `val` is returned to the caller, like in the test, it shouldn't be freed.
>
> If the `NSValue` itself dies and never escapes, then of course it's a leak,
> but in order to see that we'd need to model contents of `NSValue`.
You're absolutely right about this. The documentation for NSValue even states:
"Important: The NSValue object doesn’t copy the contents of the string, but the
pointer itself. If you create an NSValue object with an allocated data item,
don’t free the data’s memory while the NSValue object exists."
Sorry! My mistake.
https://reviews.llvm.org/D35216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks great to me. I do wonder if long-term we should consider removing the auto-deduction when loading from the store. On the one hand it is really nice to avoid having to specify that at the call to getSVal(). On the other hand, this can lead to some really pathological weirdness and a bunch of corner-case code. For loads that result from actual program semantics the type of the loaded-from storage (from the loader's perspective) should always be easily available at the load site. There would still be cases where a checker might want to inspect what the store thinks is in memory, but I think that is a different function and in my opinion deserves a separate API. https://reviews.llvm.org/D38358 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.
dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1404 + // When trying to dereference a void pointer, read the first byte. + T = Ctx.CharTy; +} NoQ wrote: > dcoughlin wrote: > > Nit: It seems a bit odd to read the first byte here since (unless I'm > > misunderstanding) this would never be triggered by actual C semantics, only > > by a checker. Did you consider just returning UnknownVal() in this case? > `UnknownVal` seems to be quite similar to assertion failure: in both cases > we'd have to force the checker to specify `CharTy` explicitly. But assertions > are better because the author of the checker would instantly know that he > needs to specify the type, instead of never noticing the problem, or spending > a lot of time in the debugger trying to understand why he gets an > `UnknownVal`. I think having an assertion with a helpful message indicating how the checker author could fix the problem would be great! But you're not triggering an assertion failure here since you're changing T to be CharTy. Instead, you're papering over the problem by making up a fake value that doesn't correspond to the program semantics. If you're going to paper over the the problem and return a value, I think it is far preferable to use UnknownVal. This would signal "we don't know what is going here" rather than just making up a value that is likely wrong. Repository: rL LLVM https://reviews.llvm.org/D38358 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35216: [analyzer] Escape symbols when creating std::initializer_list.
dcoughlin added a comment. In https://reviews.llvm.org/D35216#888506, @NoQ wrote: > Do you think this patch should be blocked in favor of complete modeling? Please, let's get this landed. We can add more precise modeling when the need arises. https://reviews.llvm.org/D35216 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38589: [Analyzer] Run tests until the end, do not stop at the first failure
dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. This looks good to me. https://reviews.llvm.org/D38589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38589: [Analyzer] Run tests until the end, do not stop at the first failure
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Oops, wrong button! LGTM! https://reviews.llvm.org/D38589 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38702: [Analyzer] Do not segfault on unexpected call_once implementation
dcoughlin added a comment.
> @dcoughlin Any advice on how to handle different stdlib implementations?
> Can we conjure a separate symbol instead of relying on a particular struct
> layout?
> For now this implementation will simply not go inside a differently
> implemented call_once.
I think that for now your solution is the best to avoid the crashes. Let's see
what Alexander has to say about the standard library causing the crashes.
Ideally, we don't want to fall down too hard on libstdc++.
If we really need to handle a variety of standard libraries (or versions of
standard libraries) we'll probably want to to treat `std::call_once` more
abstractly and write a checker that models its behavior instead of body farming
it.
Comment at: lib/Analysis/BodyFarm.cpp:365
CXXRecordDecl *FlagCXXDecl = FlagType->getAsCXXRecordDecl();
+ if (FlagCXXDecl == nullptr) {
+DEBUG(llvm::dbgs() << "Flag field is not a CXX record: "
LLVM style is to write this null check as `if (!FlagCXXDecl)`.
Comment at: lib/Analysis/BodyFarm.cpp:369
+ << "Ignoring the call.\n");
+return nullptr;
+ }
This return will leak the allocated AST nodes (as will the return for
`__state__` below). Can you hoist the validation checks to above the AST
creation?
https://reviews.llvm.org/D38702
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38702: [Analyzer] Do not segfault on unexpected call_once implementation
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me! https://reviews.llvm.org/D38702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written
dcoughlin requested changes to this revision.
dcoughlin added a comment.
This revision now requires changes to proceed.
Apologies for the delay reviewing! As I noted inline, I'm pretty worried about
the performance impact of this. Is it possible to do the analysis in a single
traversal of the translation unit?
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:123
+ // Is variable changed anywhere in TU?
+ for (const Decl *D : AMgr.getASTContext().getTranslationUnitDecl()->decls())
{
+if (isChanged(D, VD))
Since you are calling `getInitialStateForGlobalStaticVar()` in
`getInitialState()` for each static variable declaration and
`getInitialState()` is called for each top-level function, you are doing an n^3
operation in the size of the translation unit, which is going to be very, very
expensive for large translation units.
Have you considered doing the analysis for static variables that are never
changed during call-graph construction? This should be a linear operation and
doing it during call-graph construction would avoid an extra walk of the entire
translation unit.
Repository:
rL LLVM
https://reviews.llvm.org/D37897
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37478: [analyzer] Implement pointer arithmetic on constants
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me! Thanks for adding this. Do you have commit access, or do you need someone to commit it for you? https://reviews.llvm.org/D37478 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38764: [Analyzer] Assume const string-like globals are non-null
dcoughlin added a comment.
Looks like a great start!
There are a bunch of minor nits inline.
The one big thing is that I think your handling of 'const char *' in
`typeIsConstString()` isn't quite right. 'const char *' means that the
pointed-to characters can't be modified but does allow modification of the
variable holding the pointer. I don't think we want to treat such variables as
holding non-null pointers, since anyone could assign them to NULL. On the other
hand, we do want to treat variables of type 'char * const' as holding non-null
pointers since the variable can't be reassigned and (presumably) it was
initialized to a non-null value. In this second case the characters could
potentially be modified, but that doesn't change whether the value of the
pointer itself.
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:1
+
+//
We need to have the license preamble here.
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:3
+//
+// This checker ensures that const string globals are assumed to be non-null.
+//
It would be good to include a motivation for why this is the right thing to do
and even an example of the declarations this will trigger on.
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:17
+
+class NonnilStringConstantsChecker : public Checker {
+ mutable IdentifierInfo *NSStringII = nullptr;
Since this applies to more than just Objective-C, I think it would be better to
use 'null' instead of 'nil' in the name of the checker. Or even remove the
'Nonnil' prefix. What about 'GlobalStringConstantsChecker'?
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:22
+public:
+ NonnilStringConstantsChecker(AnalyzerOptions &AO) {}
+
Does the constructor need to take an AnalyzerOptions?
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:53
+ SVal V = UnknownVal();
+ if (location.isValid()) {
+V = State->getSVal(location.castAs());
Should we just early return if `location` is not valid?
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:70
+
+bool NonnilStringConstantsChecker::isGlobalConstString(SVal val) const {
+ Optional regionVal = val.getAs();
Can you add doxygen-style comments to the implementations of these methods? I'd
like to break with tradition and have comments for all new code.
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:71
+bool NonnilStringConstantsChecker::isGlobalConstString(SVal val) const {
+ Optional regionVal = val.getAs();
+ if (!regionVal)
Note that we use capital letters for variables and parameters in Clang/LLVM.
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:74
+return false;
+ const VarRegion *region = dyn_cast(regionVal->getAsRegion());
+ if (!region)
The convention Clang uses with dyn_cast and friends to to use 'auto *' in the
variable declaration in order to not repeat the type name:
```
auto *Region = dyn_cast(RegionVal->getAsRegion());
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:93
+// to be classified const.
+hasOuterConst |= type.isConstQualified();
+if (typeIsConstString(type, hasOuterConst))
Do you really want a bit-wise or here?
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:113
+II = T->getDecl()->getIdentifier();
+ }
+
This will leave `II` as uninitialized if your `if/else if` is not exhaustive.
Are you sure that it is?
Comment at: test/Analysis/nonnil-string-constants.mm:41
+// For char* we do not require a pointer itself to be immutable.
+extern const char *CharStarConst;
+void charStarCheckGlobal() {
What is the rationale for treating `const char *v` as non null?
In this scenario `v` can be reassigned, right?
Comment at: test/Analysis/nonnil-string-constants.mm:45
+}
+
+// But the pointed data should be.
I'd also like to see a test for treating `char * const v;` as non null.
Comment at: test/Analysis/nonnil-string-constants.mm:55
+extern str globalStr;
+void charStarCheckTypedef() {
+ clang_analyzer_eval(globalStr); // expected-warning{{TRUE}}
```
typedef const char *str;
extern str globalStr;
```
allows the `globalStr` variable to be written to.
Did you mean:
```
typedef char * const str;
extern str globalStr;
```
https://reviews.llvm.org/D38764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38764: [Analyzer] Assume const string-like globals are non-null
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Looks good to me. Please fix the additional nits mentioned inline and commit!
Also, make sure to do a pass to update the capitalization of variables
throughout the file to match the LLVM coding standards.
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:1
+//==-- RetainCountChecker.cpp - Checks for leaks and other issues -*- C++
-*--//
+//
The file name and description here needs to be updated for this checker.
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:10
+//
+// Class definition for NonnullStringConstantsChecker.
+// This checker adds an assumption that constant string-like globals are
I don't think the comment in the first line here is needed.
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:22
+// Checker uses are defined in the test file:
+// - test/Analysis/nonnull-string-constants.mm
+//
We generally don't do this kind of cross-reference in comments since they tend
to get stale fast when things get moved around. There is no tool support to
keep them in sync.
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:26
+
+//
+// This checker ensures that const string globals are assumed to be non-null.
Can this comment go?
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:53
+private:
+ /// Lazily initialize cache for required identifier informations.
+ void initIdentifierInfo(ASTContext &Ctx) const;
We usually put the oxygen comments on checkers on the implementation and not
the interface since they aren't API and people reading the code generally look
at the implementations. Can you move them to the implementations?
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:114
+ // Look through the typedefs.
+ while (const TypedefType *T = dyn_cast(type)) {
+type = T->getDecl()->getUnderlyingType();
To match the coding standards this should be `auto *` as well.
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:131
+
+ if (const ObjCObjectPointerType *T = dyn_cast(type)) {
+return T->getInterfaceDecl()->getIdentifier() == NSStringII;
Similar comment about `auto *`
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:133
+return T->getInterfaceDecl()->getIdentifier() == NSStringII;
+ } else if (const TypedefType *T = dyn_cast(type)) {
+return T->getDecl()->getIdentifier() == CFStringRefII;
Similar comment about `auto *`.
Comment at: test/Analysis/nonnull-string-constants.mm:5
+// Relies on the checker defined in
+// lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp.
+
We generally don't put file paths like this in comments since they tend to go
stale. It is fine to to say that these are tests for the
NonnullStringConstantsChecker though.
https://reviews.llvm.org/D38764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38810: [Analyzer] Support bodyfarming std::call_once for libstdc++
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM with the dyn_cast mentioned inline changed. Comment at: lib/Analysis/BodyFarm.cpp:359 + ValueDecl *FieldDecl = dyn_cast(FoundDecl); bool isLambdaCall = CallbackType->getAsCXXRecordDecl() && Should this be a cast<>() rather than a dyn_cast<>()? Do you expect this to fail? If so then you should check the result for null. If not then the idiom is to use cast<>(). An alternative would be to change findMemberField() to return a FieldDecl. https://reviews.llvm.org/D38810 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38797: [analyzer] CStringChecker: pr34460: Admit that some casts are hard to model.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me. https://reviews.llvm.org/D38797 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23963: [analyzer] pr28449 - Move literal rvalue construction away from RegionStore.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. OK. Seems reasonable! https://reviews.llvm.org/D23963 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38877: [analyzer] RetainCount: Accept "safe" CFRetain wrappers.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me! https://reviews.llvm.org/D38877 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39015: [Analyzer] Always use non-reference types when creating expressions in BodyFarm, removes std::call_once crash
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. This comment is unrelated to this particular patch, but since I know you're doing other work in the area I think it would also be good to have test cases for the two other forms of call_once defined in . It looks to me like those will be used for projects that that use pre-C++11 -- so we should have test coverage for them. https://reviews.llvm.org/D39015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38986: [Analyzer] Better unreachable message in enumeration
dcoughlin added a comment. What is the workflow where this is needed? Is it when an end-user is running a build with assertions and can't provide a reproducer but can provide the console output? Does llvm_unreachable() guarantee that the string construction code is completely removed from release builds? https://reviews.llvm.org/D38986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38922: [analyzer] LoopUnrolling: check the bitwidth of the used numbers (pr34943)
dcoughlin accepted this revision. dcoughlin added a comment. Thanks for fixing this! https://reviews.llvm.org/D38922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39031: [Analyzer] Correctly handle parameters passed by reference when bodyfarming std::call_once
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Some nits inline, but looks good to me!
Comment at: lib/Analysis/BodyFarm.cpp:388
+ // reference.
+ for (unsigned int i = 2; i < D->getNumParams(); i++) {
+
Nit: 'i' doesn't match the naming conventions and it is not particularly
descriptive. Perhaps "ParamIndex"?
Comment at: lib/Analysis/BodyFarm.cpp:391
+const ParmVarDecl *PDecl = D->getParamDecl(i);
+QualType PTy = PDecl->getType().getNonReferenceType();
+Expr *ParamExpr = M.makeDeclRefExpr(PDecl);
Nit: You can move PTy inside the if block so it is not calculated when it is
not needed.
https://reviews.llvm.org/D39031
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39201: [Analyzer] Handle implicit function reference in bodyfarming std::call_once
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D39201 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Content-wise, LGTM. There is a style nit inline.
Also, can you avoid reformatting the lines that haven't changed? This will help
preserve the history of the file and make it clear what changes are related to
your intended change in functionality.
Comment at: lib/Analysis/AnalysisDeclContext.cpp:308
+BodyFarm *AnalysisDeclContextManager::getBodyFarm() {
+ if (BdyFrm == nullptr)
+BdyFrm = new BodyFarm(ASTCtx, Injector.get());
Nit: Style-wise this is idiomatically written as:
```
if (!BdyFrm)
```
https://reviews.llvm.org/D39208
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp
dcoughlin added a comment. I think a good strategy is to look at your diffs and consider whether the benefits of normalizing the style outweigh the cost of losing the history. In this case, I think keeping the history makes sense. (Imagine you are a future maintainer and want to know when and why a new option was added. Is is very helpful to use git annotate to understand why the code is the way it is.) https://reviews.llvm.org/D39208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39208: [Analyzer] Do not use static storage to for implementations created in BodyFarm.cpp
dcoughlin added a comment. That would require going into the past and requiring everyone back then to run clang-format as well. Unfortunately they didn't -- so human judgment is needed when modifying code that doesn't obey the guidelines. Repository: rL LLVM https://reviews.llvm.org/D39208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39220: [Analyzer] Store BodyFarm in std::unique_ptr
dcoughlin accepted this revision. dcoughlin added a comment. LGTM. https://reviews.llvm.org/D39220 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37470: [analyzer] Handle ObjC messages conservatively in CallDescription
dcoughlin added a comment. I think it would be better to add a new "test/Analysis/block-in-critical-section.m" file rather than enabling a random alpha checker in a file that tests the analyzer core. https://reviews.llvm.org/D37470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42779: [analyzer] NFC: Make sure we don't ever inline the constructor for which the temporary destructor is noreturn and missing.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me, but I think you should include your explanatory comments in the commit message to the comment itself to help future violators of the assertion out! Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:335 + // Otherwise there's nothing wrong with inlining such constructor. + assert(!DstEvaluated.empty() && + "We should not have inlined this constructor!"); Can you add to the comment the instructions from the commit message on what to do if the assertion fires? I..e, "If any new code violates this assertion, it should be fixed by not inlining this constructor when -analyzer-config cfg-temporary-dtors is set to false." This will give someone who violates the assertion concrete steps to take. https://reviews.llvm.org/D42779 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43104: [analyzer] Find correct region for simple temporary destructor calls and inline them if possible.
dcoughlin added inline comments. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:939 std::make_pair(D.getBindTemporaryExpr(), Pred->getStackFrame())); +// *MRPtr may still be null when the construction context for the temporary +// was not implemented. Can we add an assertion somewhere else (like when leaving a stack frame context) to make sure that the expected temporaries have all been removed from InitializedTemporaries? Repository: rC Clang https://reviews.llvm.org/D43104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43533: [CFG] [analyzer] NFC: Refactor ConstructionContext into a finite set of cases.
dcoughlin added a comment.
This looks great to me Artem! I'll be very curious to see how you extend this
to handle initializer lists.
Comment at: lib/Analysis/CFG.cpp:4737
+}
+case ConstructionContext::SimpleVariableKind: {
+ const auto *DSCC = cast(CC);
Eventually (not now) I think it would be great to include the construction
context kind in the printed CFG. This would make it easier to understand at a
glance the context.
Comment at: lib/Analysis/ConstructionContext.cpp:49
+ // patterns.
+ if (const Stmt *S = TopLayer->getTriggerStmt()) {
+if (const auto *DS = dyn_cast(S)) {
I like how this puts all the messy pattern matching in one place. The follows
the general LLVM guidelines of "if it has to be messy, put it all in one place
and hide the messiness from everything else".
Comment at: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:643
-if (ParentExpr && isa(ParentExpr) &&
+if (CC && isa(CC) &&
!Opts.mayInlineCXXAllocator())
This is much more semantically clear. Thank you!!
https://reviews.llvm.org/D43533
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43497: [analyzer] Introduce correct lifetime extension behavior in simple cases.
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Looks good to me. I have a comment about simplifying
createTemporaryRegionIfNeeded() (if possible) inline.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:281
+
+if (!TR) {
+ StorageDuration SD = MT->getStorageDuration();
Would it be safe for the body of the `if (!TR)` to be the else branch of `if
constCXXTempObjectionRegion *const *TRPtr = ...` rather then its own if
statement?
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:289
}
if (!TR)
TR = MRMgr.getCXXTempObjectRegion(Init, LC);
Would it be safe for `TR = MRMgr.getCXXTempObjectRegion(Init, LC);` to be the
else branch of `if (const MaterializeTemporaryExpr *MT =
dyn_cast(Result))` rather than its own `if` statement?
Ideally the number paths through this function on which we call
`MRMgr.getCXXTempObjectRegion()` would be small and very clear.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:490
+static void printTemporaryMaterializatinosForContext(
+raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep,
Nit: There is a typo in the name here ("Materializatinos"). I guess this is the
sub-atomic particle created as a byproduct of materializing a temporary! We
have a lot of materializatinos in Cupertino.
Comment at: test/Analysis/lifetime-extension.cpp:45
+ clang_analyzer_eval(z == 2);
+#ifdef TEMPORARIES
+ // expected-warning@-4{{TRUE}}
Yay!
Comment at: test/Analysis/lifetime-extension.cpp:78
+ {
+const C &c = C(1, &after, &before);
+ }
Nice.
https://reviews.llvm.org/D43497
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43428: [CFG] [analyzer] NFC: Allow more complicated construction contexts.
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
It seems kind of sketchy to me that we're recursing over an expression to find
construction contexts and then later doing it again for sub-expressions. I
guess there is precedent here with `VisitForTemporaryDtors()`, but we should
think about whether there is a better way to do this.
Comment at: lib/Analysis/CFG.cpp:1160
+ConstructionContextMap.lookup(CE)) {
+ // We might have visited this child when we were finding construction
+ // contexts within its parents.
This is kind of scary since it means we'll be recursing over subexpressions
repeatedly. I guess VisitForTemporaryDtors() does this too, but it sounds like
we're wasting a lot of computation visiting the subtrees over and over again.
https://reviews.llvm.org/D43428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D43477 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43483: [CFG] [analyzer] Add construction context when the constructor is on a branch of a ternary operator
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This seems reasonable to me. https://reviews.llvm.org/D43483 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43666: [analyzer] When constructing a temporary without construction context, track it for destruction anyway.
dcoughlin added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2234
+ // should go away, but the assertion should remain, without the
+ // "DidCacheOutOnCleanup" part, of course.
+ bool DidCacheOutOnCleanup = false;
Can you add a comment explaining the criteria under which the
DidCacheOutOnCleanup part can be removed?
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2257
+ } else {
+Pred = *CleanUpTemporaries.begin();
+ }
I realize this isn't new code in this patch, but can you use the result of
Bldr.generateNode() to determine whether we cached out and also to get the new
Pred? That seems cleaner than querying the ExplodedNodeSet.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2262
+ assert(DidCacheOutOnCleanup ||
+ areInitializedTemporariesClear(Pred->getState(),
Pred->getLocationContext(),
It sounds like this means if we did cache out then there are places where the
initialized temporaries are not cleared (that is, we have extra gunk in the
state that we don't want).
Is that true? If so, then this relaxation of the assertion doesn't seem right
to me.
Do we need to introduce a new program point when calling `Bldr.generateNode` on
the cleaned up state (for example, with a new tag or a new program point kind)?
This would make it so that when we cache out when generating a node for the
state with the cleaned up temporaries we know that it is safe to early return
from processEndOfFunction(). It would be safe because we would know that the
other node (the one we cached out because of) has already had its temporaries
cleared and notified the checkers about the end of the function, etc.
Repository:
rC Clang
https://reviews.llvm.org/D43666
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43689: [analyzer] Disable constructor inlining when lifetime extension through fields occurs.
dcoughlin accepted this revision.
dcoughlin added inline comments.
This revision is now accepted and ready to land.
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:70
+/// by binding a smaller object within it to a reference.
+bool IsTemporaryLifetimeExtendedViaSubobject = false;
Would you be willing to add a tiny code example to the comment? I.e., `const
int &x = C().x`
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:177
+ assert(VD->getType()->isReferenceType());
+ if (VD->getType()->getPointeeType().getCanonicalType() !=
+ MTE->GetTemporaryExpr()->getType().getCanonicalType()) {
I *think* this is safe. But it seems like it would be more direct to use
skipRValueSubobjectAdjustments() and check for sub-object adjustments since
ultimately that is what you care about, right?
Repository:
rC Clang
https://reviews.llvm.org/D43689
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43714: [analyzer] Don't do anything when trivial-copying an empty class object.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D43714 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33308: [analyzer]: Improve test handling with multiple constraint managers
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me. Sorry of the delay here -- and thanks for your patience. I will review the other patches this weekend. https://reviews.llvm.org/D33308 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40809: [WIP] [analyzer] Dump counterexample traces as C programs
dcoughlin added a comment. This is seems like a very useful visualization, *especially* for loops. Can we this patch get it into a state where it can be committed and used for debugging purposes? One possibility is to turn this into a debug checker similar to debug.ViewExplodedGraph. That checker registers for a checkEndAnalysis() callback and traverses the node graph (see DebugCheckers.cpp). Can you do the same here? It doesn't look like you really need this to be a BugReporterVisitor -- and making it a debug checker would avoid outputting multiple copies for each diagnostic consumer. You could also control file output with an analyzer-config option that takes an optional path. https://reviews.llvm.org/D40809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40841: [analyzer] Fix a crash on C++17 AST for non-trivial construction into a trivial brace initializer.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me for a quick fix that we plan to address in a more principled fashion later. However, I'd like you to add a comment at each of the places where you use the parent map to note something like "This is a quick dirty fix and we really shouldn't be using the parent map to model behavior." I understand why you're doing it, but I'd like to make sure that someone reading this code doesn't think that it is a good pattern to use the parent map and decide to use it elsewhere! Using the parent map is slow and is a sign that our reasoning isn't compositional. The analyzer should be able to figure out the correct behavior based on the expression and its state/context alone. The fact that we need the parent map here is an indication that we don't have the right representation for constructors in the analyzer. (I know you're planning on tackling this in the future!) https://reviews.llvm.org/D40841 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40809: [WIP] [analyzer] Dump counterexample traces as C programs
dcoughlin added a comment. In https://reviews.llvm.org/D40809#954890, @NoQ wrote: > In https://reviews.llvm.org/D40809#954858, @dcoughlin wrote: > > > One possibility is to turn this into a debug checker similar to > > debug.ViewExplodedGraph. That checker registers for a checkEndAnalysis() > > callback and traverses the node graph (see DebugCheckers.cpp). Can you do > > the same here? It doesn't look like you really need this to be a > > BugReporterVisitor -- and making it a debug checker would avoid outputting > > multiple copies for each diagnostic consumer. > > > These prints are only for actual bugs, not for the whole graph. Even if we > identify error nodes in the final graph, we're unlikely to identify which of > them are suppressed by visitors. As it stands, this is a debugging tool not a way to communicate error paths to end users. (I think that, too, would be very helpful but I'd like to see this as a debugging aid first.) The workflow for debugging would be more like viewing the exploded graph than (say) emitting html diagnostics. My point is this: this is valuable as a debugging tool, we should get it committed as such. We can work on making it user facing separately. https://reviews.llvm.org/D40809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
dcoughlin added a comment.
Some comments on the C++ inline.
Comment at: include/clang/AST/ASTContext.h:82
class APValue;
+class ASTImporter;
class ASTMutationListener;
Is this forward declaration needed?
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:63
private:
+ cross_tu::CrossTranslationUnitContext &CTU;
+
I don't think CrossTranslationUnitContext belongs in ExprEngine (it doesn't
really have much to do with transfer functions and graph construction.
Can you move it to AnalysisDeclContextManager instead?
Also, when you move it to AnalysisManager can you make it a pointer that is
NULL when naive cross-translation support is not enabled? This will make it
more clear that the cross-translation unit support will not always be available.
Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:400
+ return CTUDir.getValue();
+}
Can you also add an analyzer option that is something like
'enable-naive-cross-translation-unit-analysis' and defaults to false?
I'd like to avoid using the presence of 'ctu-dir' as an indication that the
analyzer should use the naive CTU analysis. This way when if add a less naive
CTU analysis we'll be able to the CTUDir for analysis artifacts (such as
summaries) for the less naive CTU analysis as well.
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:365
+
+ auto Engine = static_cast(
+ getState()->getStateManager().getOwningEngine());
This downcast is an indication that the CTUContext is living in the wrong class.
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:372
+
+ cross_tu::CrossTranslationUnitContext &CTUCtx =
+ Engine->getCrossTranslationUnitContext();
Can this logic be moved to AnalysisDeclContext->getBody()?
CallEvent::getRuntimeDefinition() is really all about modeling function
dispatch at run time. It seems odd to have the cross-translation-unit loading
(which is about compiler book-keeping rather than semantics) here.
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:382
+[&](const cross_tu::IndexError &IE) {
+ CTUCtx.emitCrossTUDiagnostics(IE);
+});
I don't think it makes sense to diagnose index errors here.
Doing it when during analysis means that, for example, the parse error could be
emitted or not emitted depending on whether the analyzer thinks a particular
call site is reached.
It would be better to validate/parse the index before starting analysis rather
than during analysis itself.
Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:197
- AnalysisConsumer(const Preprocessor &pp, const std::string &outdir,
- AnalyzerOptionsRef opts, ArrayRef plugins,
- CodeInjector *injector)
+ AnalysisConsumer(CompilerInstance &CI, const Preprocessor &pp,
+ const std::string &outdir, AnalyzerOptionsRef opts,
There is no need for AnalysisConsumer to take both a CompilerInstance and a
Preprocessor. You can just call `getPreprocessor()` to get the preprocessor
from the CompilerInstance
https://reviews.llvm.org/D30691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41258: [analyzer] trackNullOrUndefValue: deduplicate path pieces for each node.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. Repository: rC Clang https://reviews.llvm.org/D41258 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40939: [analyzer] Avoid element regions of void type.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me, although I have a super nit about wording in a comment. Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2181 + // One of the forbidden LValue types! We still need to have sensible + // symbolic lvalues to represent this stuff. + if (T->isVoidType()) Super nit: Since we can't actually have lvalues for a forbidden lvalue type, we should instead call it a 'location' in the comment. "We still need to have sensible locations to represent this stuff". https://reviews.llvm.org/D40939 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41478: [analyzer] Fix zero-initialization of stack VLAs under ARC.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. The tests are great!! Repository: rC Clang https://reviews.llvm.org/D41478 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41409: [analyzer] Fix intermediate diagnostics on paths that go through operator new().
dcoughlin added a comment. LGTM as well. Repository: rC Clang https://reviews.llvm.org/D41409 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41749: [analyzer] suppress nullability inference from a macro when result is used in another macro
dcoughlin accepted this revision. dcoughlin added a comment. Yes, this looks great! https://reviews.llvm.org/D41749 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41406: [analyzer] Add a new checker callback, check::NewAllocator.
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Looks good to me with some minor nits inside (and also a request to consider
factoring out common code).
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:311
+ ExprEngine &Eng,
+ bool wasInlined = false);
+
Does 'wasInlined' really need to have a default argument? It looks like there
are only two callers. (Also the capitalization is inconsistent).
Comment at: lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp:138
+ /// new-expression is this pointer. This callback is called between steps
+ /// (2) and (3). Post-call for the allocator is called before step (1).
+ /// Pre-statement for the new-expression is called on step (4) when the value
I find the following confusing: "Post-call for the allocator is called before
step (1)." Did you mean "after" step one?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:296
+ProgramStateRef State,
+Optional RetVal = None) const;
It is probably worth explaining what RetVal is and what it means if it is None
in the doxygen.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:324
+ AllocationFamily Family = AF_Malloc,
+ Optional RetVal = None);
Same comment about documenting RetVal here.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:964
+CheckerContext &C, const Expr *E, const unsigned AllocationSizeArg,
+ProgramStateRef State, Optional retVal) const {
if (!State)
Nit: Capitalization of "retVal".
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1289
+AllocationFamily Family,
+Optional retVal) {
if (!State)
Capitalization, again.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1302
+ SymbolRef Sym = retVal->getAsLocSymbol();
assert(Sym);
I think it is worth a comment here about why we expect this assert succeed: the
value will always be the result of a malloc function or an un-inlined call to
the new allocator (if I understand correctly).
Comment at: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:343
+if (const CXXNewExpr *CNE = dyn_cast_or_null(CE)) {
+ ExplodedNodeSet DstPostPostCallCallback;
+ getCheckerManager().runCheckersForPostCall(DstPostPostCallCallback,
Is it possible/desirable to factor out this logic (running the PostCall
callbacks and then the NewAllocator callbacks) into a helper method on
ExprEngine then call the helper from both VisitCXXNewAllocatorCall() and
processCallExit()?
https://reviews.llvm.org/D41406
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41408: [analyzer] NFC: Fix nothrow operator new definition in a test.
dcoughlin added a comment. LGTM as well. Repository: rC Clang https://reviews.llvm.org/D41408 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41800: [analyzer] Use a custom program point for the check::NewAllocator callback.
dcoughlin accepted this revision.
dcoughlin added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/StaticAnalyzer/Core/CheckerManager.cpp:491
NodeBuilder &Bldr, ExplodedNode *Pred) {
// TODO: Does this deserve a custom program point? For now we're re-using
// PostImplicitCall because we're guaranteed to use the non-implicit
Can we get rid of the TODO now?
Repository:
rC Clang
https://reviews.llvm.org/D41800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41266: [analyzer] With c++-allocator-inlining, fix memory space for operator new pointers.
dcoughlin added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:477
+bool ExprEngine::isStandardGlobalOperatorNewFunction(const CXXNewExpr *CNE) {
+ const FunctionDecl *FD = CNE->getOperatorNew();
+ if (FD && !isa(FD) && !FD->isVariadic()) {
I realize this isn't your code, but could we use
`FunctionDecl::isReplaceableGlobalAllocationFunction() here?`
https://reviews.llvm.org/D41266
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43798: [analyzer] UndefinedAssignment: Fix warning message on implicit copy/move constructors.
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM. I have a suggestion for a slight modification of the diagnostic text
inline.
Comment at: test/Analysis/implicit-ctor-undef-value.cpp:12
+// missing. Specify which field is being assigned.
+class C { // expected-warning{{Value assigned to field 'y' is garbage or
undefined}}
+ // expected-note@-1{{Value assigned to field 'y' is garbage or
undefined}}
I think it would be good to mention that this assignment is happening in an
implicit constructor, since that is probably not obvious to most users.
How about: "Value assigned to field 'y' in implicit constructor is garbage or
undefined".
https://reviews.llvm.org/D43798
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
dcoughlin accepted this revision. dcoughlin added a comment. Thanks Gabor, this looks good to me. Please commit! https://reviews.llvm.org/D30691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43804: [analyzer] Enable cfg-temporary-dtors by default?
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Yay! This looks good to me. https://reviews.llvm.org/D43804 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44120: [CFG] [analyzer] Heavier CFGValueTypedCall elements.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me. I'm not super happy with the name "CFGValueTypedCall" since it doesn't make it obvious that is reflects "a function call that returns a C++ object by value." Is "CFGCallReturningObjectByValue" too long? Repository: rC Clang https://reviews.llvm.org/D44120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44124: [analyzer] Support destruction and lifetime-extension of inlined function return values.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM! Comment at: test/Analysis/lifetime-extension.cpp:301 + } + // 1. Construct the original temporary within make(), + // 2. Construct return value of make() (elidable move) and lifetime-extend It might be easier to follow if you started from '0' so that steps here match offsets into `buf`. Repository: rC Clang https://reviews.llvm.org/D44124 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44059: [analyzer] AST-matching checker to detect global central dispatch performance anti-pattern
dcoughlin added a comment.
This looks good. Some minor post-commit review inline.
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:615
+
+def GCDAsyncSemaphoreChecker : Checker<"GCDAsyncSemaphore">,
+ HelpText<"Checker for performance anti-pattern when using semaphors from
async API">,
This should have a more general name so that we can add related checks to it in
the future. I suggest "GCDAntipattern".
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:616
+def GCDAsyncSemaphoreChecker : Checker<"GCDAsyncSemaphore">,
+ HelpText<"Checker for performance anti-pattern when using semaphors from
async API">,
+ DescFile<"GCDAsyncSemaphoreChecker.cpp">;
We don't use "checker" as a term in user-facing text. I suggest "Check for
performance anti-patterns when using Grand Central Dispatch".
Comment at: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp:12
+// antipattern when synchronous API is emulated from asynchronous callbacks
+// using a semaphor:
+//
Nit: missing "e" at end of "semaphor".
Comment at: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp:23
+// Such code is a common performance problem, due to inability of GCD to
+// properly handle QoS when a combination of queues and semaphors is used.
+// Good code would either use asynchronous API (when available), or perform
Nit: same here "semaphores"
Comment at: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp:92
+ if (const auto* ND = dyn_cast(D))
+if (ND->getName().startswith("test"))
+ return;
It would be good to look at both the method/function name and the class name
name for "test", "Test", "mock", and "Mock".
Comment at: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp:144
+ /*Category=*/"Performance",
+ "Possible semaphore performance anti-pattern: wait on a semaphore "
+ "signalled to in a callback",
We should tell the user more information about why they should believe this is
bad.
I suggest "Waiting on a semaphore with Grand Central Dispatch creates useless
threads and is subject to priority inversion"
Repository:
rC Clang
https://reviews.llvm.org/D44059
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit.
dcoughlin added a comment.
Looks good! Some suggested minor tweaks to diagnostic text inline.
Comment at: test/Analysis/use-after-move.cpp:146
+A b = std::move(a); // expected-note {{Object 'a' is moved}}
+b = a; // expected-warning {{Moved-from object is copied
'a'}} expected-note {{Moved-from object is copied 'a'}}
}
"Moved-from object is copied 'a'" doesn't read quite right. I think the object
name is in the wrong spot. Instead, I would suggest: "Moved-from object 'a' is
copied"
Comment at: test/Analysis/use-after-move.cpp:789
// in STL the object is left in "valid but unspecified state" after move.
-std::vector W = std::move(V); // expected-note{{'V' became
'moved-from' here}}
-V.push_back(123); // expected-warning{{Method call on a 'moved-from'
object 'V'}}
- // expected-note@-1{{Method call on a 'moved-from'
object 'V'}}
+std::vector W = std::move(V); // expected-note{{Object 'V' of type
'std::vector' is left in valid but unspecified state after move}}
+V.push_back(123); // expected-warning{{Method called on moved-from object
'V'}}
NoQ wrote:
> Note how when an object is both a local and an STL object, the bug is
> reported as an STL bug. Which is good because it's more dangerous to use an
> STL object after move than to use your average local variable after move.
I think the diagnostic text is missing an "a". It should be "... left in a
valid but ... "
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54560/new/
https://reviews.llvm.org/D54560
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59123: [analyzer] RetainCount: Fix a crash when a function follows retain/autorelease naming convention but takes no arguments.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Sigh. Looks good! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59123/new/ https://reviews.llvm.org/D59123 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59406: [analyzer] Teach scan-build to find /usr/bin/clang when installed in /usr/local/bin/
dcoughlin created this revision.
dcoughlin added a reviewer: NoQ.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus,
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.
Change scan-build to support the scenario where scan-build is installed in
/usr/local/bin/ but clang itself is installed in /usr/bin/.
rdar://problem/48914634
Repository:
rC Clang
https://reviews.llvm.org/D59406
Files:
tools/scan-build/bin/scan-build
Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1468,6 +1468,13 @@
$Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
+ if (!defined $Clang || ! -x $Clang) {
+# Look for a clang in the sibling bin of the parent of the bin
directory. So
+# if scan-build is at /usr/local/bin/scan-build look for clang at
/usr/bin/clang
+if (-f "$RealBin/../../bin/clang") {
+ $Clang = Cwd::realpath("$RealBin/../../bin/clang");
+}
+ }
}
if (!defined $Clang || ! -x $Clang) {
return "error: Cannot find an executable 'clang' relative to" .
Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1468,6 +1468,13 @@
$Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
+ if (!defined $Clang || ! -x $Clang) {
+# Look for a clang in the sibling bin of the parent of the bin directory. So
+# if scan-build is at /usr/local/bin/scan-build look for clang at /usr/bin/clang
+if (-f "$RealBin/../../bin/clang") {
+ $Clang = Cwd::realpath("$RealBin/../../bin/clang");
+}
+ }
}
if (!defined $Clang || ! -x $Clang) {
return "error: Cannot find an executable 'clang' relative to" .
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59406: [analyzer] Teach scan-build to find /usr/bin/clang when installed in /usr/local/bin/
dcoughlin added a comment. That's a good point. I've updated the patch to look for 'xcrun' CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59406/new/ https://reviews.llvm.org/D59406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59406: [analyzer] Teach scan-build to find /usr/bin/clang when installed in /usr/local/bin/
dcoughlin updated this revision to Diff 190931.
dcoughlin edited the summary of this revision.
dcoughlin added a comment.
Herald added a subscriber: jdoerfert.
Update to restrict the new behavior to when Xcode is present.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59406/new/
https://reviews.llvm.org/D59406
Files:
tools/scan-build/bin/scan-build
Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1459,6 +1459,16 @@
return $arg;
}
+####
+# FindXcrun - searches for the 'xcrun' executable. Returns "" if not found.
+####
+
+sub FindXcrun {
+ my $xcrun = `which xcrun`;
+ chomp $xcrun;
+ return $xcrun;
+}
+
####
# FindClang - searches for 'clang' executable.
####
@@ -1468,6 +1478,16 @@
$Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
+ if (!defined $Clang || ! -x $Clang) {
+# When an Xcode toolchain is present, look for a clang in the sibling
bin
+# of the parent of the bin directory. So if scan-build is at
+# $TOOLCHAIN/usr/local/bin/scan-build look for clang at
+# $TOOLCHAIN/usr/bin/clang.
+my $has_xcode_toolchain = FindXcrun() ne "";
+if ($has_xcode_toolchain && -f "$RealBin/../../bin/clang") {
+ $Clang = Cwd::realpath("$RealBin/../../bin/clang");
+}
+ }
}
if (!defined $Clang || ! -x $Clang) {
return "error: Cannot find an executable 'clang' relative to" .
@@ -1477,8 +1497,7 @@
}
else {
if ($Options{AnalyzerDiscoveryMethod} =~ /^[Xx]code$/) {
- my $xcrun = `which xcrun`;
- chomp $xcrun;
+ my $xcrun = FindXcrun();
if ($xcrun eq "") {
return "Cannot find 'xcrun' to find 'clang' for analysis.\n";
}
Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1459,6 +1459,16 @@
return $arg;
}
+####
+# FindXcrun - searches for the 'xcrun' executable. Returns "" if not found.
+####
+
+sub FindXcrun {
+ my $xcrun = `which xcrun`;
+ chomp $xcrun;
+ return $xcrun;
+}
+
####
# FindClang - searches for 'clang' executable.
####
@@ -1468,6 +1478,16 @@
$Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
+ if (!defined $Clang || ! -x $Clang) {
+# When an Xcode toolchain is present, look for a clang in the sibling bin
+# of the parent of the bin directory. So if scan-build is at
+# $TOOLCHAIN/usr/local/bin/scan-build look for clang at
+# $TOOLCHAIN/usr/bin/clang.
+my $has_xcode_toolchain = FindXcrun() ne "";
+if ($has_xcode_toolchain && -f "$RealBin/../../bin/clang") {
+ $Clang = Cwd::realpath("$RealBin/../../bin/clang");
+}
+ }
}
if (!defined $Clang || ! -x $Clang) {
return "error: Cannot find an executable 'clang' relative to" .
@@ -1477,8 +1497,7 @@
}
else {
if ($Options{AnalyzerDiscoveryMethod} =~ /^[Xx]code$/) {
- my $xcrun = `which xcrun`;
- chomp $xcrun;
+ my $xcrun = FindXcrun();
if ($xcrun eq "") {
return "Cannot find 'xcrun' to find 'clang' for analysis.\n";
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59406: [analyzer] Teach scan-build to find /usr/bin/clang when installed in /usr/local/bin/
This revision was automatically updated to reflect the committed changes.
Closed by commit rL356308: [analyzer] Teach scan-build to find clang when
installed in /usr/local/bin/ (authored by dcoughlin, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D59406?vs=190931&id=190932#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59406/new/
https://reviews.llvm.org/D59406
Files:
cfe/trunk/tools/scan-build/bin/scan-build
Index: cfe/trunk/tools/scan-build/bin/scan-build
===
--- cfe/trunk/tools/scan-build/bin/scan-build
+++ cfe/trunk/tools/scan-build/bin/scan-build
@@ -1460,6 +1460,16 @@
}
####
+# FindXcrun - searches for the 'xcrun' executable. Returns "" if not found.
+####
+
+sub FindXcrun {
+ my $xcrun = `which xcrun`;
+ chomp $xcrun;
+ return $xcrun;
+}
+
+####
# FindClang - searches for 'clang' executable.
####
@@ -1468,6 +1478,16 @@
$Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
+ if (!defined $Clang || ! -x $Clang) {
+# When an Xcode toolchain is present, look for a clang in the sibling
bin
+# of the parent of the bin directory. So if scan-build is at
+# $TOOLCHAIN/usr/local/bin/scan-build look for clang at
+# $TOOLCHAIN/usr/bin/clang.
+my $has_xcode_toolchain = FindXcrun() ne "";
+if ($has_xcode_toolchain && -f "$RealBin/../../bin/clang") {
+ $Clang = Cwd::realpath("$RealBin/../../bin/clang");
+}
+ }
}
if (!defined $Clang || ! -x $Clang) {
return "error: Cannot find an executable 'clang' relative to" .
@@ -1477,8 +1497,7 @@
}
else {
if ($Options{AnalyzerDiscoveryMethod} =~ /^[Xx]code$/) {
- my $xcrun = `which xcrun`;
- chomp $xcrun;
+ my $xcrun = FindXcrun();
if ($xcrun eq "") {
return "Cannot find 'xcrun' to find 'clang' for analysis.\n";
}
Index: cfe/trunk/tools/scan-build/bin/scan-build
===
--- cfe/trunk/tools/scan-build/bin/scan-build
+++ cfe/trunk/tools/scan-build/bin/scan-build
@@ -1460,6 +1460,16 @@
}
####
+# FindXcrun - searches for the 'xcrun' executable. Returns "" if not found.
+####
+
+sub FindXcrun {
+ my $xcrun = `which xcrun`;
+ chomp $xcrun;
+ return $xcrun;
+}
+
+####
# FindClang - searches for 'clang' executable.
####
@@ -1468,6 +1478,16 @@
$Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
if (!defined $Clang || ! -x $Clang) {
$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
+ if (!defined $Clang || ! -x $Clang) {
+# When an Xcode toolchain is present, look for a clang in the sibling bin
+# of the parent of the bin directory. So if scan-build is at
+# $TOOLCHAIN/usr/local/bin/scan-build look for clang at
+# $TOOLCHAIN/usr/bin/clang.
+my $has_xcode_toolchain = FindXcrun() ne "";
+if ($has_xcode_toolchain && -f "$RealBin/../../bin/clang") {
+ $Clang = Cwd::realpath("$RealBin/../../bin/clang");
+}
+ }
}
if (!defined $Clang || ! -x $Clang) {
return "error: Cannot find an executable 'clang' relative to" .
@@ -1477,8 +1497,7 @@
}
else {
if ($Options{AnalyzerDiscoveryMethod} =~ /^[Xx]code$/) {
- my $xcrun = `which xcrun`;
- chomp $xcrun;
+ my $xcrun = FindXcrun();
if ($xcrun eq "") {
return "Cannot find 'xcrun' to find 'clang' for analysis.\n";
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57858: [analyzer] Add a new frontend flag to display all checker options
dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. I'm pretty worried about exposing this flag to end users. - Almost none of the options you've listed are user facing. Many represent options intended for use by static analyzer developers: debugging options, feature flags, and checkers that were never finished. Others represent mechanisms for build systems to control the behavior of the analyzer. Even these are not meant for end users to interact with but rather for implementers of build systems and IDEs. I don't think end users should have to understand these options to use the analyzer. - The help text refers to analyzer implementation details (such as "SymRegion") that users won't have the context or knowledge to understand. - The help text also recommends invoking -cc1 directly or through the driver with -Xclang. Neither of these are supported end-user interfaces to the analyzer. Instead, users should use scan-build or another tool (such as CodeChecker) that was designed to be used by humans. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57858/new/ https://reviews.llvm.org/D57858 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57860: [analyzer] Validate checker option names and values
dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. I think it would be better if the default for compatibility mode were 'true'. That way this change will be backwards compatible and clients who want to enforce stricter checking could enable it. Setting compatibility mode to be true in the driver is not sufficient since many build systems call the frontend directly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57860/new/ https://reviews.llvm.org/D57860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57922: [analyzer] Insert checker options into AnalyzerOption::ConfigTable
dcoughlin added a comment. This looks good to me. It is great to see this tested! Did you consider separating the checker options from the non-checker options in the config dumper output? That would probably be easier to read. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57922/new/ https://reviews.llvm.org/D57922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59465: [analyzer] Add example plugin for checker option handling
dcoughlin requested changes to this revision. dcoughlin added a comment. This revision now requires changes to proceed. I'm not sure we really want to add any more examples of plugins. Analyzer plugins aren't supported and likely never will be. We probably shouldn't give people false hope that a plugin model is going to be possible any time soon. I'm worried that this will just encourage people to shoot themselves in the foot. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59465/new/ https://reviews.llvm.org/D59465 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60742: [analyzer] RegionStore: Enable loading default bindings from variables.
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM.
Comment at: clang/unittests/StaticAnalyzer/StoreTest.cpp:20
+// back from that variable. Test what happens if default bindings are used.
+class VariableBindConsumer : public ExprEngineConsumer {
+ void performTest(const Decl *D) {
It is awesome to see unit tests for this!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60742/new/
https://reviews.llvm.org/D60742
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60988: [analyzer] Fix crash when returning C++ objects from ObjC messages-to-nil.
dcoughlin added a comment. Looks good to me. This is a really interesting corner of Objective-C++! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60988/new/ https://reviews.llvm.org/D60988 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61161: [analyzer] RetainCount: Add a suppression for functions that follow "the Matching rule".
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61161/new/ https://reviews.llvm.org/D61161 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.
dcoughlin accepted this revision. dcoughlin added a comment. Looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61545/new/ https://reviews.llvm.org/D61545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57981: [analyzer] strlcat() syntax check: Fix an off-by-one error.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57981/new/ https://reviews.llvm.org/D57981 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58365: [attributes] Add a MIG server routine attribute.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me as long as Aaron is happy with it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58365/new/ https://reviews.llvm.org/D58365 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58366: [analyzer] MIGChecker: Make use of the server routine annotation.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/test/Analysis/mig.mm:2 +// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,alpha.osx.MIG\ +// RUN:-fblocks -verify %s Ooh, I didn't know you could split RUN lines like this. That is awesome! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58366/new/ https://reviews.llvm.org/D58366 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Looks good to me. I have some a minor diagnostic wording suggestion in line.
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:109
+llvm::raw_svector_ostream OS(Str);
+OS << "Deallocating object passed through parameter '" << PVD->getName()
+ << '\'';
Could we just have the note say "'name' is deallocated"?
Or "Value passed through parameter 'name' is deallocated"
The ".. is ... " construction matches our other checkers. (Like "Memory is
released" from the Malloc Checker.)
Comment at: clang/test/Analysis/mig.mm:52
+kern_return_t release_twice(mach_port_name_t port, vm_address_t addr1,
vm_address_t addr2, vm_size_t size) {
+ kern_return_t ret = KERN_ERROR; // expected-note{{'ret' initialized to 1}}
+ vm_deallocate(port, addr1, size); // expected-note{{Deallocating object
passed through parameter 'addr1'}}
A nice QoI improvement here (for a later patch, perhaps) would be to have this
note use the macro name: "'ret initialized to KERN_ERROR'".
Users probably won't know that KERN_ERROR is 1.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58368/new/
https://reviews.llvm.org/D58368
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58392: [analyzer] MIGChecker: Fix false negatives for releases in automatic destructors.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Oh, interesting. I'm glad you thought of this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58392/new/ https://reviews.llvm.org/D58392 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Aah, MIG_NO_REPLY.
LGTM.
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:42
+ std::vector> Deallocators = {
+#define CALL(required_args, deallocated_arg, ...)
\
+ {{{__VA_ARGS__}, required_args}, deallocated_arg}
Could you put a comment with an example indicating how CALL works for a
particular example function? I think that will make is easier for folks to add
support for new APIs in the future without getting it wrong.
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:125
+ // See if there's an annotated method in the superclass.
+ if (const auto *MD = dyn_cast(D))
Perhaps we could make it a Sema error if a method doesn't have a mig server
routine annotation but it overrides a method that does. From a
documentation/usability perspective it would be unfortunate if a human looking
at a method to determine its convention would have to look at its super classes
to see whether they have an annotation too.
Although, thinking about it more that might make it a source-breaking change if
an API author were to add annotation on a super class. Then API clients would
have to add the annotations to get their projects to build, which would not be
great..
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:162
+// Returns true if V can potentially represent a "successful" kern_return_t.
+static bool isSuccess(SVal V, CheckerContext &C) {
+ ProgramStateRef State = C.getState();
Could we rename this to "mayBeSuccess()" or something like that so that the
name of function indicate the "potentially" part of the comment.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58397/new/
https://reviews.llvm.org/D58397
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58529: [analyzer] MIGChecker: Enable by default.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58529/new/ https://reviews.llvm.org/D58529 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: Add preliminary Cross Translation Unit support library
dcoughlin added a comment.
Except for some drive-by nits, this is a high-level review.
I have some high-level questions about the design of the library:
1. **How do you intend to handle the case when there are multiple function
definitions with the same USR?** Whose responsibility it is to deal with this:
the client (for example, the static analyzer) or the index/database
(libCrossTU)? This seems to me to be a fundamental part of the design for this
feature that is missing. If you expect the client to handle this scenario (for
example, to pick a definition) I would expect the library API to return more
than a single potential result for a query. If you expect the the library to
pick a definition then at a minimum you should document how this will be done.
If the library will treat this as an error then you should communicate this to
the client so it can attempt to recover from it.
2. **Whose responsibility is it to handle errors that the library runs into?**
Right now several errors appear to reported to the end user as diagnostics. It
seems to me that the decision of how (or whether) to report these errors to the
end user should be up to a particular client. Are these errors even actionable
on the part of end users? My sense it that with the envisioned workflow users
would not know/care about the format of the database file.
3. **Where should the code that understands the database file format live?**
Right now the logic for the parsing of the index format is in libCrossTU and
the emission of the index is in the command-line tool. I think it would be
easier to maintain if there were a single place that understand the file
format. This way consumers and emitters of the index could be easily updated
when the format/representation changes. Additionally, I think it is important
to document this format.
Comment at: include/clang/Basic/DiagnosticCrossTUKinds.td:13
+def err_fnmap_parsing : Error<
+ "error parsing CrossTU index file: '%0' line: %1 'USR filename' format "
+ "expected">;
Is this a user-facing diagnostic? Do we expect users to know what 'CrossTU'
means? Or what 'USR' means?
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:60
+ ///
+ /// Note that the AST files should also be in the \p CrossTUDir.
+ const FunctionDecl *getCrossTUDefinition(const FunctionDecl *FD,
Can this document what happens when the function declaration cannot be found?
And what happens when multiple function declarations are found?
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:48
+ StringRef LookupFnName)
{
+ if (!DC)
+return nullptr;
Should this assert that DC is not null? What is the reason to accept null here?
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:81
+ if (!ExternalFnMapFile) {
+Context.getDiagnostics().Report(diag::err_fe_error_opening)
+<< ExternalFunctionMap << "required by the CrossTU functionality";
What is the rationale behind emitting this as a diagnostic? In general for a
library I would expect that errors would be communicated to the client, which
then would have responsibility for handling them, reporting them to the user,
or aborting.
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:92
+StringRef LineRef{Line};
+if (Pos > 0 && Pos != std::string::npos) {
+ FunctionName = LineRef.substr(0, Pos);
It looks like this is parsing. Is the file format documented anywhere? Also, it
would be good to keep the parsing and generation code in the same place so
that it is easy to figure out what to update when the file format changes.
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:86
+ if (SM.isInMainFile(Body->getLocStart()))
+DefinedFuncsStr << LookupName.str().str() << " " << CurrentFileName
+<< "\n";
It seems like the functionality that writes an entry and the functionality that
reads an entry should ideally be in the same place. This way when the format is
updated it is obvious what other places need to be updated. Can the code that
writes a mapping be moved to libCrossTU and can we have this tool link against
it?
https://reviews.llvm.org/D34512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37023: [analyzer] Fix bugreporter::getDerefExpr() again.
dcoughlin added a comment.
This seems like it is on the right track, but I think the emphasis here on
lvalue-to-rvalue casts is a bit misplaced. The logic for "what is the pointer
being dereferenced" and "what is the lvalue that holds the pointer being
dereferenced" is mixed together in a way that I find confusing.
I think an easier to understand approach is to first find the rvalue of the
pointer being dereferenced. Then, second, pattern match on that to find the
underlying lvalue the pointer was loaded from (when possible).
But first a couple of points just to make sure we're on the same page (with
apologies for the wall of text!). Suppose we have:
struct Y {
...
int z;
};
struct X {
...
Y y;
};
and a local 'x' of type `X *`.
For an access of the form `int l = x->y.z` the pointer being dereferenced is
the rvalue `x`. The dereference ultimately results in a load from memory, but
the address loaded from may be different from the the pointer being
dereferenced. Here, for example, the address loaded from is (the rvalue of) `x`
+ "offset of field y into X" + "offset of field z into Y".
Fundamentally, given an expression representing the lvalue that will be loaded
from, the way to find the rvalue of the pointer being dereferenced is to strip
off the parts of the expression representing addition of offsets and any
identity-preserving casts until you get to the expression that represents the
rvalue of the base address. (This is why stripping off unary `*` makes sense --
in an lvalue context it represents adding an offset of 0. This is also why
stripping off lvalue-to-rvalue casts doesn't make sense -- they do not preserve
identity nor add an offset)
For `int l = x->y.z,` the expression for the pointer being dereferenced is the
lvalue-to-rvalue cast that loads the value stored in local variable `x` from
the lvalue that represents the address of the local variable `x`. But note that
the pointer being dereferenced won't always be an lvalue-to-rvalue cast. For
example in `int l = returnsPointerToAnX()->y.z` the expression for the pointer
being dereferenced is the call expression `returnsPointerToAnX()`. There is no
lvalue in sight.
Now, the existing behavior of `getDerefExpr()` requires that it return the
lvalue representing the location containing the dereferenced pointer when
possible. This is required by `trackNullOrUndefValue()` (sigh). So I suggest,
for now, first finding the rvalue for the dereferenced value and then adding a
fixup at the end of `getDerefExpr()` that looks to see whether the rvalue for
the dereferenced pointer is an lvalue-to-rvalue cast. If so, the fixup will
return the sub expression representing the lvalue.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:46
+/// Given that expression S represents a pointer that would be dereferenced,
+/// try to find the immediate sub-expression that represents the pointer
+/// which is being dereferenced.
An interesting aspect of this function is that sometimes it returns a
sub-expression that represents the pointer rvalue and sometimes it returns a
sub-expression that represents the lvalue whose location contains the pointer
which is being dereferenced.
I believe the reason we need the lvalue is that trackNullOrUndef() tracks
lvalues better than rvalues. (That function loads from the lvalue to get the
symbol representing the dereferenced pointer value.)
This behavior is pretty confusing, so we should document it in the comment.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:48
+/// which is being dereferenced.
+/// For example, for 'x->y.z = 2' the answer would be 'x->y' (without the
+/// implicit lvalue-to-rvalue cast surrounding it); then, for 'x->y' (again,
This comment is not right. For `x->y.z = 2` the answer should be `x` and not
`x->y`.
https://reviews.llvm.org/D37023
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: Add preliminary Cross Translation Unit support library
dcoughlin added a comment. > The importing alters the ASTContext, so the only way to choose between the > definitions would be via a callback that is triggered before the import is > done. What do you think? I think that could work. Another possibility would be to have a two step process. The first step would return a representation of possible definitions to import; then the client would chose which one to important and call another API to perform the actual import. In either case, the important scenario I think we should support is choosing at a call site to a C function the most likely definition of the called function, based on number and type of parameters, from multiple possible definitions in other translation units. If the API is rich enough to support this then I think that is a good indication it will be useful for other scenarios as well. > The reason we introduced the diagnostic is that we assumed that the client > can not recover from a configuration error and will end up reporting the > problem to the user. For the static analyzer client, at least, one possible recovery is performing the existing invalidation that we do when calling a function defined in another translation unit. (I'm not really sure this a good idea; I just wanted to point out that the decision probably belongs with the client). I think it is reasonable to report an error as a diagnostic -- but I think this should be up to the client and I don't think it should show up in the index file itself. In an ideal world the user wouldn't even know that file existed. Further, for large projects/incremental rebuilds a text file is unlikely to be a suitable representation for the index. In the long term I doubt the file will be end-user editable/readable. https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16403: Add scope information to CFG for If/While/For/Do/Compound/CXXRangeFor statements
dcoughlin added a comment. Maxim, thanks for commandeering the patch. I apologize for the long delay reviewing! I really like the approach of retrying without scopes enabled when we hit a construct we can't handle yet. This will make is possible to turn the feature on by default (and get it running on large amounts of code) while still developing it incrementally! I'd like to suggest a change in the scope representation that will make it much easier to support all the corner cases and also re-use the existing logic for handling variable scopes. Right now the CFGScopeBegin and CFGScopeEnd elements use a statement (a loop or a CompoundStatement) to uniquely identify a scope. However, this is a problem because a `for` loop with an initializer may have two scopes that we really want to distinguish: for (int i = 0; i < 10; i++) int j = i; Here `i` and `j` really have different scopes, but with the current approach we won't be able tell them apart. My suggestion is to instead use the first VarDecl declared in a scope to uniquely identify it. This means, for example, that CFGScopeBegin's constructor will take a VarDecl instead of a statement. What's nice about this VarDecl approach is that the CFGBuilder already has a bunch of infrastructure to correctly track local scopes for variables (see the `LocalScope` class in CFG.cpp and the `addLocalScopeForStmt()` function.) Further, there are two functions, addLocalScopeAndDtors() and addAutomaticObjHandling() that are already called in all the places where a scope should end. This means you should only need to add logic to add an end of scope element in those two functions. What's more, future work to extend CFG construction to handle new C++ language features will also use these two functions -- so we will get support for those features for free. For an example of how to change those two functions, take a look at Matthias Gehre's commit in https://reviews.llvm.org/D15031. What you would need to do for CFGScopeEnd would be very similar to that patch. Using this approach should also make it possible to re-use the logic in that patch to eventually handle gotos. To handle CFGScopeBegin, you would only need to change CFGBuilder::VisitDeclSubExpr() to detect when the variable being visited is the first variable declared in its LocalScope. If so, you would append the CFGScopeBegin element. What's nice about this is that you won't have to handle all the different kinds of loops individually. What do you think? I'd be happy to have a Skype/Google hangouts talk about this if you want to have real-time discussion about it. Repository: rL LLVM https://reviews.llvm.org/D16403 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28955: [analyzer] Enable support for symbolic extension/truncation
dcoughlin added a comment. @ddcc : When I run this I get a bunch of assertion failures. Does this depend on https://reviews.llvm.org/D28953? (Which was reverted) Is it subsumed by https://reviews.llvm.org/D35450? Is this blocking on a review of another patch on our end? https://reviews.llvm.org/D28955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker
dcoughlin added a comment. I'm worried about doing this for main(). I think many people would find it surprising if analyzing main() were to behave differently than other functions. I'm particularly worried because I think it is quite common to create a small test program with only main() to understand the behavior of the analyzer. https://reviews.llvm.org/D36251 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36475: [analyzer] Add "create_sink" annotation support to MagentaHandleChecker
dcoughlin added a comment. I'm a bit surprised that this is needed for unit tests. Aren't unit tests supposed to clean up their own state? Leaking a scarce resource in one unit test can cause another unit test to fail, which can be hard to track down. Or is this for deliberately testing a scenario where a resource is leaked? If you're deliberately testing a leaking scenario, it seems to me that using #ifndef __clang_analyzer__ would probably be an easier suppression mechanism for the test author to understand and maintain. Can you elaborate on why you're running the analyzer on units tests and how reports from the analyzer on unit tests are used? I think it would be helpful for us to understand your work flow. Also, in general, my feeling is the exposing the internal mechanisms of the analyzer (here, the notion of a sink) in an attribute is not good for users. Ideally, attributes should be used to describe the behavior of code that they annotate. This means that they also serve as documentation for the user and not just as an ad hoc mechanism to communicate with the analyzer. https://reviews.llvm.org/D36475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: Add preliminary Cross Translation Unit support library
dcoughlin added a comment.
Thanks Gabor! Some additional comments in line.
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:118
+ ///
+ /// \return Returns a map with the loaded AST Units and the declarations
+ /// with the definitions.
Is this comment correct? Does it return a map?
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:128
+ /// \brief This function merges a definition from a separate AST Unit into
+ ///the current one.
+ ///
I found this comment confusing. Is 'Unit' the separate ASTUnit or it the
current one?
Comment at: include/clang/CrossTU/CrossTranslationUnit.h:134
+
+ std::string getLookupName(const NamedDecl *ND);
+
If this is public API, it deserves a comment. If not, can we make it private?
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:35
+namespace {
+// FIXME: This class is will be removed after the transition to llvm::Error.
+class IndexErrorCategory : public std::error_category {
Is this FIXME still relevant? What will need to be transitioned to llvm::Error
for it to be removed? Can you make the comment a bit more specific so that
future maintainers will know when/how to address the FIXME?
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:194
+handleAllErrors(IndexOrErr.takeError(), [&](const IndexError &IE) {
+ (bool)PropagatedErr;
+ PropagatedErr =
What is this cast for? Is it to suppress an analyzer dead store false positive?
I thought we got all those!
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:199
+ case index_error_code::missing_index_file:
+Context.getDiagnostics().Report(diag::err_fe_error_opening)
+<< ExternalFunctionMap
If I understand correctly, there is no way to call loadExternalAST() without
the potential for a diagnostic showing up to the user. Can this diagnostic
emission be moved to the client?
Comment at: lib/CrossTU/CrossTranslationUnit.cpp:249
+CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD,
+ ASTUnit *Unit) {
+ ASTImporter &Importer = getOrCreateASTImporter(Unit->getASTContext());
Does this method need to take the unit? Can it just get the ASTContext from
'FD'? If not, can we add an assertion that FD has the required relationship to
Unit? (And document it in the header doc.)
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:56
+private:
+ std::string getLookupName(const FunctionDecl *FD);
+ void handleDecl(const Decl *D);
Is getLookupName() here used?
Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:72
+SmallString<128> LookupName;
+bool Res = index::generateUSRForDecl(D, LookupName);
+assert(!Res);
Should this instead reuse the logic for generating a lookup name from
CrossTranslationUnitContext::getLookupName()? That way if we change how the
lookup name is generated we only have to do it in one place.
https://reviews.llvm.org/D34512
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: Add preliminary Cross Translation Unit support library
dcoughlin added a comment. I'm OK with this going into the repo a is (although it is light on tests!), as long as we have an agreement that you'll be OK with iteration on both the interface and the implementation to handle real-world projects. More specifically, for this to work well on large/complicated projects we'll need to: - Move conflict resolution from index generation where it is now to query time so that clients can pick the best implementation of a function when multiple are found. This is important for projects that have multiple functions with the same name or that build the same file in multiple ways. - Change function lookup from needing to traverse over the entire AST. - Move away from a linear, flat-file index to something that can handle larger projects more efficiently. For this last point, I suspect it will be good to adopt whatever Clangd ends up using to support indexing. (There has been some discussion of this relatively recently on the cfe-dev list.) https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34512: Add preliminary Cross Translation Unit support library
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. > But I also think it wouldn't be good to block this until ClanD indexing > reaching a mature state. I agree 100%. > All in all, we are willing to reuse as much of ClangD indexing as we can in > the future, but I think it is also more aligned with the LLVM Developer > Policy to have something working committed and do the development in the repo > rather than working separately on a fork. This sounds good to me. I just wanted to make sure that you were onboard with changing this index to use the common indexing infrastructure in the future. (Assuming it makes sense to do so, of course). https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35796: [analyzer] Delete with non-virtual destructor check
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This looks good to me! Do you have commit access, or do you need someone to commit it for you? https://reviews.llvm.org/D35796 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61925: [analyzer] MIGChecker: Add support for os_ref_retain().
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61925/new/ https://reviews.llvm.org/D61925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D61958: [analyzer] RetainCount: Fix a crash when os_returns_retained_on_zero (_nonzero) is applied to functions that return weird stuff.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good do me. Thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61958/new/ https://reviews.llvm.org/D61958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55671: [analyzer] Don't pretend that unknown block calls have one null parameter.
dcoughlin added a comment. Looks good to me! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55671/new/ https://reviews.llvm.org/D55671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55566: [analyzer] LiveVariables: Fix a zombie expression problem, add testing infrastructure.
dcoughlin added a comment. This looks good to me. It is great to see a dumper for this! Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55566/new/ https://reviews.llvm.org/D55566 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55804: [analyzer] C++17: Fix leak false positives when an object with destructor is returned from the top frame.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. This seems reasonable to me, although I have a question inline about why you are using `makeZeroElementRegion()`. Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:224 +bool IsArray = false; +V = makeZeroElementRegion(State, V, ReturnTy, IsArray); +assert(!IsArray && "Returning array from a function!"); I don't understand why you are using makeZeroElementRegion() here. Doesn't the assertion of !IsArray mean it must just return 'V'? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55804/new/ https://reviews.llvm.org/D55804 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55873: [analyzer] CStringChecker: Fix a crash when an argument of a weird type is encountered.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Comment at: test/Analysis/string.cpp:23 + +// A similarly weird override outside of the class. +void *memcpy(void *, const S &, size_t); I'm pretty sure you mean 'overload' instead of 'override' here and elsewhere. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55873/new/ https://reviews.llvm.org/D55873 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55875: [analyzer] pr38668: RegionStore: Do not attempt to cast loaded values of non-scalar types.
dcoughlin added a comment.
These seems reasonable, although it does also seem like there could be quite a
few unintended consequences that we haven't discovered yet.
I'm also a bit worried about the change in the analyzer's behavior on copy().
Do you have a sense of how much of an effect this will have and how easy
potential false positives from this will be to suppress?
Comment at: test/Analysis/bstring.cpp:47
+ // The TRUE warning shows up on the path on which the vector is empty.
clang_analyzer_eval(i == 66); // expected-warning {{UNKNOWN}}
This seems like it will be a big analysis policy change from the user's
perspective and is likely to generate a bunch of new reports.
Can the user add an assertion that v.size() > 0 to tell the analyzer that the
path on which the vector is empty is not feasible?
What are the diagnostic notes look like? Can the user tell that the the
analyzer is assuming that begin() == end() on that path?
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55875/new/
https://reviews.llvm.org/D55875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55875: [analyzer] pr38668: RegionStore: Do not attempt to cast loaded values of non-scalar types.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. Looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55875/new/ https://reviews.llvm.org/D55875 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
