[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Csaba Dabis via Phabricator via cfe-commits]

Charusso resigned from this revision.
Charusso added a comment.

@NoQ, what do you think?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97699/new/

https://reviews.llvm.org/D97699

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thank you guys for investigating it!

In D69726#2671734 , @vsavchenko wrote:

> 2. The analyzer doesn't explain why it thinks that `guc_malloc` returns null 
> pointer.  I find it alarming that it might assume it for all the wrong 
> reasons.



In D69726#2673178 , @NoQ wrote:

> We should still investigate the tracking bug though, i.e. why path in 
> `guc_malloc()` isn't explained.

What I have seen back in the days is that: uninitialized variables and 
variables storing `NULL` are not attached to regions so we cannot map notes to 
their origin region and we cannot track them. That is where 
`trackExpressionValue()` tries to attach notes based on changes in the Store 
and with full of heuristics. The nature of heuristics and the fight of 
note-creation and note-suppression Reporters what you see.

If we would prioritize to massage the `trackExpressionValue()` framework, count 
me in: I have half year of pai- programming in it, but I am out of the office.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D69726#2671526 , @vsavchenko wrote:

> @Charusso 
> It looks like this patch introduced a some weird false positive on PostgreSQL 
> F16161734: report-guc.c-ParseLongOption-13-1.html 
> 
> I'll try to look at it myself and minimize it, but maybe you can get an idea 
> from a full report.

Could you supply us with all the divergence please? If there is only one case, 
may we can ignore it by appending it to @NoQ's `extremely-weird-bugs.txt` so 
people can focus on more important stuff.

What I see in the bug report is that: `line 3290 - guc_malloc()` returns null 
and it is a true positive.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Cool, thanks!

Debug facility NFC: 
https://reviews.llvm.org/rG89d210fe1a7a1c6cbf926df0595b6f107bc491d5
`size` -> `extent` conversion: 
https://reviews.llvm.org/rG9b3df78b4c2ab7a7063e532165492e1ffa38d401




Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:294
+
+  SmallString<128> Msg;
+  llvm::raw_svector_ostream Out(Msg);

NoQ wrote:
> 128 is too much imho :)
I am totally fine with 128, but let us use 64 then:
https://developer.apple.com/documentation/authenticationservices/asauthorizationcontrollerpresentationcontextproviding


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Charusso marked an inline comment as done.
Closed by commit rGdf64f471d1e2: [analyzer] DynamicSize: Store the dynamic size 
(authored by Charusso).

Changed prior to commit:
  https://reviews.llvm.org/D69726?vs=319940=335284#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp

Index: clang/test/Analysis/explain-svals.cpp
===
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -54,7 +54,7 @@
   int *x = new int[ext];
   clang_analyzer_explain(x); // expected-warning-re^pointer to element of type 'int' with index 0 of heap segment that starts at symbol of type 'int \*' conjured at statement 'new int \[ext\]'$
   // Sic! What gets computed is the extent of the element-region.
-  clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re^signed 32-bit integer '4'$
+  clang_analyzer_explain(clang_analyzer_getExtent(x)); // expected-warning-re^\(argument 'ext'\) \* 4$
   delete[] x;
 }
 
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -28,6 +28,7 @@
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
@@ -729,13 +730,6 @@
 // MemRegionManager methods.
 //===--===//
 
-static DefinedOrUnknownSVal getTypeSize(QualType Ty, ASTContext ,
-  SValBuilder ) {
-  CharUnits Size = Ctx.getTypeSizeInChars(Ty);
-  QualType SizeTy = SVB.getArrayIndexType();
-  return SVB.makeIntVal(Size.getQuantity(), SizeTy);
-}
-
 DefinedOrUnknownSVal MemRegionManager::getStaticSize(const MemRegion *MR,
  SValBuilder ) const {
   const auto *SR = cast(MR);
@@ -766,7 +760,7 @@
 if (Ty->isIncompleteType())
   return UnknownVal();
 
-return getTypeSize(Ty, Ctx, SVB);
+return getElementSize(Ty, SVB);
   }
   case MemRegion::FieldRegionKind: {
 // Force callers to deal with bitfields explicitly.
@@ -774,7 +768,7 @@
   return UnknownVal();
 
 QualType Ty = cast(SR)->getDesugaredValueType(Ctx);
-DefinedOrUnknownSVal Size = getTypeSize(Ty, Ctx, SVB);
+DefinedOrUnknownSVal Size = getElementSize(Ty, SVB);
 
 // A zero-length array at the end of a struct often stands for dynamically
 // allocated extra memory.
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -18,6 +18,7 @@
 #include "clang/Analysis/ConstructionContext.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
@@ -689,16 +690,30 @@
 
 // See if we need to conjure a heap pointer instead of
 // a regular unknown pointer.
-bool IsHeapPointer = false;
-if (const auto *CNE = dyn_cast(E))
-  if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
-// FIXME: Delegate this to evalCall in MallocChecker?
-IsHeapPointer = true;
+const auto *CNE = dyn_cast(E);
+if (CNE && CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  R = svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count);
+  const MemRegion *MR = R.getAsRegion()->StripCasts();
+
+  // Store the extent of the allocated object(s).
+  SVal ElementCount;
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr)) {
+ElementCount = State->getSVal(SizeExpr, LCtx);
+  } else {
+ElementCount = svalBuilder.makeIntVal(1, /*IsUnsigned=*/true);
   

[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D97699#2601804 , @NoQ wrote:

> Why are you even tracking `reg_$0`? It's obvious from the structure of 
> the symbol that it's an environment pointer, there's no need to keep it in 
> maps.

`envp` is confusable with `argv`: `int main(int argc, char *argv[], char 
*envp[])` so we obtain `envp`'s region from `main()` to match it later.




Comment at: clang/docs/analyzer/checkers.rst:2103-2104
+puts(envp[i]);
+// envp may no longer point to the current environment
+// this program has unanticipated behavior.
+  }

NoQ wrote:
> It's very important to explain whether the "unanticipated behavior" is an 
> entirely psychological concept ("most people don't understand how this works 
> but this can occasionally also be a valid solution if you know what you're 
> doing") or a 100% clear indication of a bug ("such code can never be 
> correct", eg. it instantly causes undefined behavior, or the written value 
> can never be read later so there's absolutely no point in writing it, or 
> something like that).
The standard terminology is very vague, like that:
```
The getenv function returns a pointer to a string associated with the matched 
list member. The string pointed to shall not be modified by the program but may 
be overwritten by a subsequent call to the getenv function.
```
What the hell.   I think it is about table-doubling and reallocating the 
entire environment pointer table at some point which makes sense in case of the 
non-getter function calls. For the getters I think another processes could 
overwrite the `environ` pointer between two getter calls and problem could 
occur because of such table-doubling. To resolve the issue we need to call 
`strdup()` and create a copy of the current environment entry instead of having 
a pointer to it as I see.

@zukatsinadze it would be great to see a real reference with real issues in 
real world software. Is the true evil the multiple chained getter calls?



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97699/new/

https://reviews.llvm.org/D97699

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98741: [analyzer] Introduce common bug category "Unused code".

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

Great idea!




Comment at: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist:11413
-   categoryDead store
+   categoryUnused code
typeDead initialization
check_namedeadcode.DeadStores

"Initialization was never used"? (Swift style: 
https://swift.godbolt.org/z/c17EMb)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98741/new/

https://reviews.llvm.org/D98741

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

@NoQ, could you upstream it and move this idea forward please?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a reviewer: vsavchenko.
Charusso added a comment.

Hey, I am back.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+   const CXXNewExpr *NE,
+   const LocationContext *LCtx, SValBuilder );

Charusso wrote:
> NoQ wrote:
> > This function is probably going to be used exactly once in the whole code. 
> > There's no need to turn it into a public API.
> It is being used 3 times already, so I believe it is a cool API.
Let us remove the `setDynamicSize(CXXNewExpr)` API from now on (was not that 
cool). This function will reside inside `ExprEngine::bindReturnValue()` which 
is the second phase of evaluating operator new: 
https://reviews.llvm.org/D40560#961694



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+  QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+: CE->getArg(0)->IgnoreParenImpCasts()->getType();
+

Charusso wrote:
> NoQ wrote:
> > How is this better than `getValueType()`? Are you sure you're not getting a 
> > pointer type instead in the `!TVR` case? I.e., can you test this on a 
> > non-heap `SymRegion`?
> > How is this better than getValueType()?
> Consistency. We get the static ~~size~~ extent by getting the desugared type 
> which most likely just an extra overhead.
> 
> > Are you sure you're not getting a pointer type instead in the !TVR case? 
> > I.e., can you test this on a non-heap SymRegion?
> The issue was the `var_region_simple_ptr` test: we need to use regex for 
> checking after the `}}` token where `/ 4` is the correct result. I did not 
> really get why the test pass.
Added a conjured `SymbolicRegion` test called `user_defined_new` and an 
assertion.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1388
- ->castAs()
- ->StripCasts()
- ->castAs();

NoQ wrote:
> What happened to this `StripCasts()`? I don't see it in the after-code and I 
> vaguely remember having to be very careful with it.
I have reimplemented it by a mistake with the `getSuperRegion(MR)` helper 
routine. From now I will use `StripCasts()` instead. Nice catch!



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:29
+/// Helper to bypass the top-level ElementRegion of \p MR.
+static const MemRegion *getSuperRegion(const MemRegion *MR) {
+  assert(MR);

Szelethus wrote:
> `getSuperRegionIfElementRegion`?
`getSuperRegion(MR)` was just a plain helper routine not restricted to return 
iff `MR` is an `ElementRegion`. It obtains the immediate region which we can 
measure in size, but it turns out the same idea as `MemRegion::StripCasts()`, 
so I have removed it.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:36-44
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,
 SValBuilder ) {
+  MR = getSuperRegion(MR);
+
+  if (const DefinedOrUnknownSVal *Size = State->get(MR))
+return *Size;
+

Szelethus wrote:
> Well hold on for a second. Does this mean that we legit resolved 
> `getDynamicSize` to `getStaticSize`??? That's crazy! I've been looking around 
> in the code for how does your patch interact with our current dynamic size 
> modeling, but I guess I never really realized that there is no such a thing. 
> Odd to not even see a FIXME around.
I wanted to upstream both of the patches, but this one became dead:

[analyzer] DynamicSize: Remove 'getExtent()' from regions
Oct 29 2019, 01:43

[analyzer] DynamicSize: Store the dynamic size
Nov 1 2019, 19:57

Sorry for the inconvenience.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:80-82
+  /* FIXME: Make this work.
+  if (const auto CI = Size.getAs())
+assert(CI->getValue().isUnsigned());*/

Szelethus wrote:
> Make this work as is "make sure that checkers actually obey this 
> precondition" or does the assert just not work as-is? Could you specify this 
> comment?
This assertion was about the signedness, because I have seen something like 
`unsigned(42) == signed(42)` did not evaluate to true, and I really wanted to 
make the size equality to be true in such value-equality cases. I have added a 
test (signedness_equality) instead, which is working fine now (I hope, because 
I am not sure what was the original behavior).



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:779-780
 
   bool IsStandardGlobalOpNewFunction =
   FD->isReplaceableGlobalAllocationFunction();
 

Szelethus wrote:
> I wonder what the rationale was here. Why do we explicitly 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 319940.
Charusso marked 6 inline comments as done.
Charusso added a comment.

- Fix everything.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

Files:
  clang/docs/analyzer/developer-docs/DebugChecks.rst
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/memory-model.cpp

Index: clang/test/Analysis/memory-model.cpp
===
--- /dev/null
+++ clang/test/Analysis/memory-model.cpp
@@ -0,0 +1,157 @@
+// RUN: %clang_analyze_cc1 -std=c++20 \
+// RUN:  -analyzer-checker=core,unix,cplusplus,debug.ExprInspection \
+// RUN:  -triple x86_64-unknown-linux-gnu \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t number, size_t size);
+void free(void *);
+
+struct S {
+  int f;
+};
+
+void clang_analyzer_dump(int);
+void clang_analyzer_dump(const void *);
+void clang_analyzer_dumpExtent(int);
+void clang_analyzer_dumpExtent(const void *);
+void clang_analyzer_dumpElementCount(int);
+void clang_analyzer_dumpElementCount(const void *);
+
+int clang_analyzer_getExtent(void *);
+void clang_analyzer_eval(bool);
+
+void var_simple_ref() {
+  int a = 13;
+  clang_analyzer_dump(); // expected-warning {{a}}
+  clang_analyzer_dumpExtent();   // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void var_simple_ptr(int *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning {{(extent_$1{SymRegion{reg_$0}}) / 4}}
+}
+
+void var_array() {
+  int a[] = {1, 2, 3};
+  clang_analyzer_dump(a); // expected-warning {{Element{a,0 S64b,int}}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void string() {
+  clang_analyzer_dump("foo"); // expected-warning {{Element{"foo",0 S64b,char}}}
+  clang_analyzer_dumpExtent("foo");   // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount("foo"); // expected-warning {{4 S64b}}
+}
+
+void struct_simple_ptr(S *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning {{(extent_$1{SymRegion{reg_$0}}) / 4}}
+}
+
+void field_ref(S a) {
+  clang_analyzer_dump(); // expected-warning {{a.f}}
+  clang_analyzer_dumpExtent();   // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void field_ptr(S *a) {
+  clang_analyzer_dump(>f); // expected-warning {{SymRegion{reg_$0}.f}}
+  clang_analyzer_dumpExtent(>f);   // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(>f); // expected-warning {{1 S64b}}
+}
+
+void symbolic_array() {
+  int *a = new int[3];
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] a;
+}
+
+void symbolic_placement_new() {
+  char *buf = new char[sizeof(int) * 3];
+  int *a = new (buf) int(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] buf;
+}
+
+void symbolic_malloc() {
+  int *a = (int *)malloc(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  free(a);
+}
+
+void symbolic_alloca() {
+  int *a = (int *)alloca(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a);   // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void 

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a reviewer: vsavchenko.
Charusso added a comment.

Hey! We are somewhat slow in reviews, please understand that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92634/new/

https://reviews.llvm.org/D92634

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92652: [clang-tidy][docs] Update check options with boolean values instead of non-zero/0/1

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:60
 - If copy to the destination array can overflow [1] and
-  ``AreSafeFunctionsAvailable`` is set to ``Yes``, ``y`` or non-zero and it is
+  ``AreSafeFunctionsAvailable`` is set to `true` and it is
   possible to obtain the capacity of the destination array then the new 
function

aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > This edit loses information about also accepting `Yes` and `y` -- is that 
> > > intentional (or were those unsupported before)?
> > > 
> > > FWIW, I'd be fine dropping support for alternate spellings of `true`.
> > Having looked throughout the NotNullTerminatedResultCheck header/impl 
> > files, I can't find any reference to `AreSafeFunctionsAvailable`.
> > I can only guess this is meant to say WantToUseSafeFunctions. If that is 
> > the case, `Yes` and `y` were never supported spellings.
> > 
> > Should this be changed to use that option name instead? cc @Charusso
> > 
> > FWIW I intend (in the near future) to extend boolean parsing for check 
> > options to:
> > `y|Y|yes|Yes|YES|true|True|TRUE|on|On|ON`
> > `n|N|no|No|NO|false|False|FALSE|off|Off|OFF`.
> > 
> > Reason for this is we claim to use YAML for config format and according to 
> > its specification, this is what is accepted as a boolean value. Ref 
> > https://yaml.org/type/bool.html.
> > Still need to keep the old integer method of specifying bools for backwards 
> > compatibility reasons.
> > 
> > Should this be changed to use that option name instead? cc @Charusso
> 
> I think so, but that can be done in an NFC followup if you'd like.
> 
> > Reason for this is we claim to use YAML for config format and according to 
> > its specification, this is what is accepted as a boolean value.
> 
> Oh, that's a good reason to support those spellings, thank you for clarifying.
Hey, nice catch and cool patch! Sorry for the extra work. 
`WantToUseSafeFunctions` is wanted to be here as a zero or non-zero value [1]. 
It would be great if you could rewrite this variable name because I do not 
write LLVM code any more. Thanks!

[1] 
https://clang.llvm.org/extra/clang-tidy/checks/bugprone-not-null-terminated-result.html#options


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92652/new/

https://reviews.llvm.org/D92652

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70411: [analyzer] CERT STR rule checkers: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks for the reviews!

In D70411#2153562 , @Szelethus wrote:

> Please do not bypass the previous comments that hadn't reached a conclusion 
> -- littering inlines about miscellaneous stuff at this stage does more harm 
> then good, and derails the discussion.


Ugh, it is a long story. I really wanted to create the best and trendiest 
checker of all time with Artem so all the comments are made for him at first. 
We have had the exact opposite sense what we are trying to achieve here, that 
is why we left the project dead.

Hopefully I get your suggestion right and you want to make sure I have 
addressed all the comments. Let me begin with a suggestion as well:

> If you are interested in the solution, please do not care about the problem, 
> but the solution.

- non-literal advice from //It's Not About the Shark: How to Solve Unsolvable 
Problems//

-

Let me rewind / summarize what problems happened in chunks if you would Ctrl+F5 
for certain quotes and what we did in chronological order:

> You're bringing in a completely brand-new machinery here, could you explain 
> how it works and why do you need it?

I have attached multiple bug reports to the program state (by registering a 
map) and invalidated all the non-fatal reports in chain on the bug-path to a 
given fatal-report to support two functionality of the checker.

That two functionality was a complete stupidity it turned out, I have left only 
the necessary one.

> I think it would really help if you draw a state machine for the checker

Because I have removed the complexity of the checker now it is easy to 
understand and it does not require an appendix.

> But it doesn't necessarily mean that static analysis tools can be built by 
> generalizing over the examples from the CERT wiki.

Artem refuse the usefulness of the CERT rules from the static analysis 
perspective, which I agree with.

> "if string length metadata is tainted (in particular, if the string itself is 
> tainted) and is potentially larger than the destination buffer size (eg., 
> unconstrained), warn"

Artem suggests to develop taint analysis, which I do not want to develop, but 
his concerns are real.

> Traditionally checkers either warn immediately when they can detect an error, 
> or assume that the error has not happened.

That was the near the point when I have realized we need to warn on the 
function calls immediately. The buffer overflow is immediately a security 
issue, but I was dumb to realize.

> I took a look at first 5 and in all of them the code does in fact ensure that 
> the buffer space is sufficient, but in 5 different ways.

Artem believes the arrays are cool because they cannot overflow. For example we 
obtain an IP address, which has enough space as `char[64]` our mental model 
suggests. I believe any overflow is not cool in a security point of view: 
https://twitter.com/TwitterSupport/status/1283526400146837511

Now I have accepted Artem's suggestion and I do not care about arrays because 
the Analyzer cannot model the constraints of the array size yet / taintness. If 
someone working with a plain char array that could be easy to detect and 
measure. For the security problems we want to catch allocations.

> The known size does not mean that the string is going to be null-terminated.
>  The problem is not the size, but the missing '\0' (which you can have 
> multiple of at any point).

I still wanted to care about null-termination, however the entire checker 
should be about overflow.

> - "The checker warns every time it finds an execution path on which 
> unintended behavior occurs"
> - "The checker enforces a certain coding guideline on the user": (say, "don't 
> pass the string by pointer anywhere before you null-terminate it")
> - If you rely on the existing taint analysis infrastructure and make a good 
> check, that'll be wonderful and would further encourage us to move taint 
> analysis out of alpha.

Artem tried to create a direction for the checker, it was pretty complex. I 
have refused again, because all the people null terminate by hand.

> This text may be hard to understand.

Balazs get confused by the second behavior, like Artem did for a while. I have 
tried to rephrase the documentation. With Balazs' help we made the 
documentation cool (Thanks again!).

> Get rid of the secondary behavior for now.

That was the time when I have learnt complexity is a huge issue and people will 
get confused. I have removed the entire secondary behavior of null termination 
checking and moved some logic to STR32-C.

-

The conclusion: Now this checker is very strict and hopefully enough simple. I 
hope we could design the future of checker-writing.




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:859
+
+def Str31cChecker : Checker<"31c">,
+  HelpText<"SEI CERT checker of rules defined in STR31-C">,

[PATCH] D70411: [analyzer] CERT STR rule checkers: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 278368.
Charusso marked 18 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.

- Resolve most of the review comments.
- We really need to specify the design of future checkers.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/AST/FormatStringParsing.h
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/AST/FormatString.cpp
  clang/lib/AST/FormatStringParsing.h
  clang/lib/AST/PrintfFormatString.cpp
  clang/lib/AST/ScanfFormatString.cpp
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/cert/str31-c-extras.cpp
  clang/test/Analysis/cert/str31-c-notes.cpp
  clang/test/Analysis/cert/str31-c.cpp

Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,184 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/x/sNUxBQ
+
+#include "../Inputs/system-header-simulator.h"
+
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {
+  }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+  char buf[128];
+  sprintf(buf, "%s.txt", name);
+  // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+  char buff[128];
+  snprintf(buff, sizeof(buff), "%s.txt", name);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buf[BUF_LENGTH];
+  fscanf(stdin, "%s", buf);
+  // expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buff[BUF_LENGTH];
+  fscanf(stdin, "%1023s", buff);
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char prog_name[128];
+  strcpy(prog_name, name);
+  // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+  return 0;
+}
+
+void func(void) {
+  char buff[256];
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+  }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char *prog_name2 = (char *)malloc(strlen(name) + 1);
+  if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+  }
+
+  free(prog_name2);
+  return 0;
+}
+
+void func(void) {
+  char *buff2;
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+  strcpy(buff2, editor);
+}
+
+free(buff2);
+  }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n - 1); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_good
+
+namespace test_getchar_bad {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buf[BUFFERSIZE];
+  char *p;
+  int ch;
+  p = buf;
+  while ((ch = getchar()) != '\n' && ch != EOF) {
+*p++ = (char)ch;
+  }
+  *p++ = 0;
+  if (ch == EOF) {
+/* Handle EOF or error */
+  }
+}
+} // namespace test_getchar_bad
+
+namespace test_getchar_good {
+enum { BUFFERSIZE = 

[PATCH] D71155: [analyzer] CERT STR rule checkers: STR30-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D71155#1854908 , @NoQ wrote:

> Let's separate `CStringChecker` improvements into a separate patch and have a 
> separate set of tests for it.


I was thinking about creating tests, but I cannot figure out any better testing 
than a live checker that uses such features of `CStringChecker`. Also I have 
not found any real world issues with the checker, so I could not really test 
its real behavior.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71155/new/

https://reviews.llvm.org/D71155



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70805: [analyzer] SValHasDescendant: Determine whether the SVal has an SVal descendant

[Csaba Dabis via Phabricator via cfe-commits]

Charusso abandoned this revision.
Charusso added a comment.
Herald added subscribers: ASDenysPetrov, martong, steakhal.

Way more sophisticated matching: https://reviews.llvm.org/D77745


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70805/new/

https://reviews.llvm.org/D70805



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71155: [analyzer] CERT STR rule checkers: STR30-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 265963.
Charusso retitled this revision from "[analyzer] CERT: STR30-C" to "[analyzer] 
CERT STR rule checkers: STR30-C".
Charusso added a comment.
Herald added subscribers: ASDenysPetrov, martong, steakhal.

- Refactor.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71155/new/

https://reviews.llvm.org/D71155

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/cert/str30-c-notes.cpp
  clang/test/Analysis/cert/str30-c.cpp

Index: clang/test/Analysis/cert/str30-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/x/VtYxBQ
+
+#include "../Inputs/system-header-simulator.h"
+
+typedef __SIZE_TYPE__ size_t;
+typedef __typeof__((char *)0 - (char *)0) ptrdiff_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+char *strrchr(const char *str, int c);
+int puts(const char *str);
+
+namespace test_strrchr_bad {
+const char *get_dirname(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+  if (slash) {
+*slash = '\0'; /* Undefined behavior */
+// expected-warning@-1 {{'*slash' is pointing to a constant string}}
+  }
+  return pathname;
+}
+
+int main(void) {
+  puts(get_dirname(__FILE__));
+  return 0;
+}
+} // namespace test_strrchr_bad
+
+namespace test_strrchr_good {
+char *get_dirname(const char *pathname, char *dirname, size_t size) {
+  const char *slash;
+  slash = strrchr(pathname, '/');
+  if (slash) {
+ptrdiff_t slash_idx = slash - pathname;
+if ((size_t)slash_idx < size) {
+  memcpy(dirname, pathname, slash_idx);
+  dirname[slash_idx] = '\0';
+  return dirname;
+}
+  }
+  return 0;
+}
+
+int main(void) {
+  char dirname[260];
+  if (get_dirname(__FILE__, dirname, sizeof(dirname))) {
+puts(dirname);
+  }
+  return 0;
+}
+} // namespace test_strrchr_good
Index: clang/test/Analysis/cert/str30-c-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c-notes.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30c \
+// RUN:  -analyzer-output=text -verify %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/x/VtYxBQ
+
+#include "../Inputs/system-header-simulator.h"
+
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+char *strrchr(const char *str, int c);
+int puts(const char *str);
+
+namespace test_strrchr_bad {
+const char *get_dirname(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+  // expected-note@-1 {{'strrchr' returns a pointer to the constant 'pathname'}}
+
+  slash = strrchr(slash, '/');
+  // expected-note@-1 {{'strrchr' returns a pointer to the constant 'slash'}}
+
+  if (slash) {
+// expected-note@-1 {{'slash' is non-null}}
+// expected-note@-2 {{Taking true branch}}
+
+*slash = '\0'; /* Undefined behavior */
+// expected-note@-1 {{'*slash' is pointing to a constant string}}
+// expected-warning@-2 {{'*slash' is pointing to a constant string}}
+  }
+  return pathname;
+}
+
+int main(void) {
+  puts(get_dirname(__FILE__));
+  // expected-note@-1 {{Calling 'get_dirname'}}
+  return 0;
+}
+} // namespace test_strrchr_bad
Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
@@ -12,6 +12,9 @@
 // - https://wiki.sei.cmu.edu/confluence/x/ptUxBQ
 //
 //  This checker is a base checker which consist of the following checkers:
+//  - STR30-C: Do not attempt to modify string literals:
+// - https://wiki.sei.cmu.edu/confluence/x/VtYxBQ
+//
 //  - STR31-C: Guarantee that storage for strings has sufficient space for
 //character data and the null terminator:
 // - https://wiki.sei.cmu.edu/confluence/x/sNUxBQ
@@ -83,6 +86,17 @@
 
   /// \{
   /// Check the function calls defined in 'CDM'.
+  /// STR30-C.
+  void checkMemchr(const CallEvent , const CallContext ,
+   CheckerContext ) const;
+  void checkStrchr(const CallEvent , const CallContext ,
+   CheckerContext ) const;
+  void checkStrrchr(const CallEvent , const CallContext ,
+CheckerContext ) const;
+  void checkStrstr(const CallEvent , const CallContext ,
+   CheckerContext ) const;
+  void checkStrpbrk(const CallEvent , const CallContext ,
+ 

[PATCH] D71033: [analyzer] CERT STR rule checkers: STR32-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 265962.
Charusso retitled this revision from "[analyzer] CERT: STR32-C" to "[analyzer] 
CERT STR rule checkers: STR32-C".
Charusso added a comment.
Herald added subscribers: ASDenysPetrov, martong, steakhal.

- Refactor.
- State out explicitly whether the Analyzer models the dynamic size.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71033/new/

https://reviews.llvm.org/D71033

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/cert/str32-c-notes.cpp
  clang/test/Analysis/cert/str32-c.cpp
  clang/test/Analysis/malloc.c

Index: clang/test/Analysis/malloc.c
===
--- clang/test/Analysis/malloc.c
+++ clang/test/Analysis/malloc.c
@@ -9,21 +9,6 @@
 
 void clang_analyzer_eval(int);
 
-// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
-// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
-// the builtin type: "Using the typedef version can cause portability
-// problems", but we're ok here because we're not actually running anything.
-// Also of note is this cryptic warning: "The wchar_t type is not supported
-// when you compile C code".
-//
-// See the docs for more:
-// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
-#if !defined(_WCHAR_T_DEFINED)
-// "Microsoft implements wchar_t as a two-byte unsigned value"
-typedef unsigned short wchar_t;
-#define _WCHAR_T_DEFINED
-#endif // !defined(_WCHAR_T_DEFINED)
-
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void *alloca(size_t);
Index: clang/test/Analysis/cert/str32-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str32-c.cpp
@@ -0,0 +1,88 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.32c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR32-C:
+// https://wiki.sei.cmu.edu/confluence/x/r9UxBQ
+
+#include "../Inputs/system-header-simulator.h"
+
+void *realloc(void *memblock, size_t size);
+
+namespace test_strncpy_bad {
+enum { STR_SIZE = 32 };
+
+size_t func(const char *source) {
+  char c_str[STR_SIZE];
+  size_t ret = 0;
+
+  if (source) {
+c_str[sizeof(c_str) - 1] = '\0';
+strncpy(c_str, source, sizeof(c_str));
+ret = strlen(c_str);
+// expected-warning@-1 {{'c_str' is not null-terminated}}
+  }
+  return ret;
+}
+} // namespace test_strncpy_bad
+
+namespace test_strncpy_good {
+enum { STR_SIZE = 32 };
+
+size_t func(const char *src) {
+  char c_str[STR_SIZE];
+  size_t ret = 0;
+
+  if (src) {
+strncpy(c_str, src, sizeof(c_str) - 1);
+c_str[sizeof(c_str) - 1] = '\0';
+ret = strlen(c_str);
+  }
+  return ret;
+}
+} // namespace test_strncpy_good
+
+namespace test_realloc_bad {
+wchar_t *cur_msg = NULL;
+size_t cur_msg_size = 1024;
+size_t cur_msg_len = 0;
+
+void lessen_memory_usage(void) {
+  if (cur_msg == NULL)
+return;
+
+  size_t temp_size = cur_msg_size / 2 + 1;
+  wchar_t *temp = (wchar_t *)realloc(cur_msg, temp_size * sizeof(wchar_t));
+  /* temp and cur_msg may no longer be null-terminated */
+  if (temp == NULL)
+return;
+
+  cur_msg = temp;
+  cur_msg_size = temp_size;
+  cur_msg_len = wcslen(cur_msg);
+  // expected-warning@-1 {{'cur_msg' is not null-terminated}}
+}
+} // namespace test_realloc_bad
+
+namespace test_realloc_good {
+wchar_t *cur_msg = NULL;
+size_t cur_msg_size = 1024;
+size_t cur_msg_len = 0;
+
+void lessen_memory_usage(void) {
+  if (cur_msg == NULL)
+return;
+
+  size_t temp_size = cur_msg_size / 2 + 1;
+  wchar_t *temp = (wchar_t *)realloc(cur_msg, temp_size * sizeof(wchar_t));
+  /* temp and cur_msg may no longer be null-terminated */
+  if (temp == NULL)
+return;
+
+  cur_msg = temp;
+  /* Properly null-terminate cur_msg */
+  cur_msg[temp_size - 1] = L'\0';
+  cur_msg_size = temp_size;
+  cur_msg_len = wcslen(cur_msg);
+}
+} // namespace test_realloc_good
Index: clang/test/Analysis/cert/str32-c-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str32-c-notes.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.32c \
+// RUN:  -analyzer-output=text -verify %s
+
+// See the examples on the page of STR32-C:
+// https://wiki.sei.cmu.edu/confluence/x/r9UxBQ
+
+#include "../Inputs/system-header-simulator.h"
+
+void *realloc(void *memblock, size_t size);
+
+namespace test_realloc_bad {
+size_t cur_msg_size = 1024;
+size_t cur_msg_len = 0;
+
+void lessen_memory_usage(wchar_t *cur_msg) {
+  if (cur_msg == NULL) {
+// expected-note@-1 {{Assuming 'cur_msg' is 

[PATCH] D70411: [analyzer] CERT STR rule checkers: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 265959.
Charusso retitled this revision from "[analyzer] CERT: STR31-C" to "[analyzer] 
CERT STR rule checkers: STR31-C".
Charusso added a comment.

- Refactor.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp
  clang/test/Analysis/cert/str31-c-notes.cpp
  clang/test/Analysis/cert/str31-c.cpp

Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,184 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/x/sNUxBQ
+
+#include "../Inputs/system-header-simulator.h"
+
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {
+  }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+  char buf[128];
+  sprintf(buf, "%s.txt", name);
+  // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+  char buff[128];
+  snprintf(buff, sizeof(buff), "%s.txt", name);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buf[BUF_LENGTH];
+  fscanf(stdin, "%s", buf);
+  // expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buff[BUF_LENGTH];
+  fscanf(stdin, "%1023s", buff);
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char prog_name[128];
+  strcpy(prog_name, name);
+  // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+  return 0;
+}
+
+void func(void) {
+  char buff[256];
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+  }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char *prog_name2 = (char *)malloc(strlen(name) + 1);
+  if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+  }
+
+  free(prog_name2);
+  return 0;
+}
+
+void func(void) {
+  char *buff2;
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+  strcpy(buff2, editor);
+}
+
+free(buff2);
+  }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n - 1); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_good
+
+namespace test_getchar_bad {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buf[BUFFERSIZE];
+  char *p;
+  int ch;
+  p = buf;
+  while ((ch = getchar()) != '\n' && ch != EOF) {
+*p++ = (char)ch;
+  }
+  *p++ = 0;
+  if (ch == EOF) {
+/* Handle EOF or error */
+  }
+}
+} // namespace test_getchar_bad
+
+namespace test_getchar_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buf[BUFFERSIZE];
+  int ch;
+  size_t index = 0;
+  size_t chars_read = 0;
+
+  while ((ch = getchar()) != '\n' && ch != EOF) {
+if (index < sizeof(buf) - 1) {
+  buf[index++] = (char)ch;
+}
+ 

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

I believe it is very strange on a Windows system to have multiple dots in a 
file. The other issue could be the wildcard `/*/` in a path full of `\`s. The 
LLVM `lit` (https://llvm.org/docs/CommandGuide/lit.html) has tons of 
Windows-related shortcuts, which I have never seen being used, but could be 
useful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76768/new/

https://reviews.llvm.org/D76768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76768: [analyzer] Added support of scan-build and exploded-graph-rewriter regression tests for Windows

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76768/new/

https://reviews.llvm.org/D76768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:33-50
+void SignalInMultithreadedProgramCheck::registerMatchers(MatchFinder *Finder) {
+  auto signalCall =
+  callExpr(
+  ignoringImpCasts(hasDescendant(declRefExpr(hasDeclaration(
+  functionDecl(allOf(hasName("signal"), parameterCountIs(2),
+ hasParameter(0, hasType(isInteger())
+  .bind("signal");

abelkocsis wrote:
> steakhal wrote:
> > I apologize for interrupting the review.
> > Should we use `hasDescendant` for matching like everything in translation 
> > unit?
> > 
> > Wouldn't it be more performant to collect all the `signal` calls in a set 
> > and set a bool variable if a `ThreadList` function seen? 
> > At the end of the analysis of the translation unit, emit warnings for each 
> > recorded `signal` call if the variable was set //(aka. we are in 
> > multithreaded environment)//.
> > 
> > Treat this comment rather a question since I'm not really familiar with 
> > `clang-tidy`.
> I updated the checker not exactly in that way you mentioned, but I think you 
> are right that we should not match for all `translationUnitDecl`.
Every single check runs on the whole `TranslationUnitDecl`, and the matcher 
will try to match on every of the TU's descendant, that is why it emit multiple 
reports in the same single run. There is no need to use `translationUnitDecl()` 
and nor to use `forEachDescendant()`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:38
+   functionDecl(hasAnyListedName(ThreadList)))
+  .bind("thread")),
+  hasDescendant(varDecl(hasType(recordDecl(hasName("std::thread")

You can emit the binding of `thread`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:56
+  diag(MatchedSignal->getExprLoc(),
+   "singal function should not be called in a multithreaded program");
+}

`singal` -> `signal`


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75229/new/

https://reviews.llvm.org/D75229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 254420.
Charusso marked 4 inline comments as done.
Charusso added a comment.

- Simplify tests.
- Remove dead code, they are far away to being used.
- Add an extra test case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp
  clang/test/Analysis/cert/str31-c-notes.cpp
  clang/test/Analysis/cert/str31-c.cpp

Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,185 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {
+  }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+  char buf[128];
+  sprintf(buf, "%s.txt", name);
+  // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+  char buff[128];
+  snprintf(buff, sizeof(buff), "%s.txt", name);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buf[BUF_LENGTH];
+  fscanf(stdin, "%s", buf);
+  // expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buff[BUF_LENGTH];
+  fscanf(stdin, "%1023s", buff);
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char prog_name[128];
+  strcpy(prog_name, name);
+  // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+  return 0;
+}
+
+void func(void) {
+  char buff[256];
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+  }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char *prog_name2 = (char *)malloc(strlen(name) + 1);
+  if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+  }
+
+  free(prog_name2);
+  return 0;
+}
+
+void func(void) {
+  char *buff2;
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+  strcpy(buff2, editor);
+}
+
+free(buff2);
+  }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n - 1); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_good
+
+namespace test_getchar_bad {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buf[BUFFERSIZE];
+  char *p;
+  int ch;
+  p = buf;
+  while ((ch = getchar()) != '\n' && ch != EOF) {
+*p++ = (char)ch;
+  }
+  *p++ = 0;
+  if (ch == EOF) {
+/* Handle EOF or error */
+  }
+}
+} // namespace test_getchar_bad
+
+namespace test_getchar_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buf[BUFFERSIZE];
+  int ch;
+  size_t index = 0;
+  size_t 

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 254423.
Charusso added a comment.

- Remove the last dead comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp
  clang/test/Analysis/cert/str31-c-notes.cpp
  clang/test/Analysis/cert/str31-c.cpp

Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,185 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {
+  }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+  char buf[128];
+  sprintf(buf, "%s.txt", name);
+  // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+  char buff[128];
+  snprintf(buff, sizeof(buff), "%s.txt", name);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buf[BUF_LENGTH];
+  fscanf(stdin, "%s", buf);
+  // expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buff[BUF_LENGTH];
+  fscanf(stdin, "%1023s", buff);
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char prog_name[128];
+  strcpy(prog_name, name);
+  // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+  return 0;
+}
+
+void func(void) {
+  char buff[256];
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+  }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char *prog_name2 = (char *)malloc(strlen(name) + 1);
+  if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+  }
+
+  free(prog_name2);
+  return 0;
+}
+
+void func(void) {
+  char *buff2;
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+  strcpy(buff2, editor);
+}
+
+free(buff2);
+  }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n - 1); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_good
+
+namespace test_getchar_bad {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buf[BUFFERSIZE];
+  char *p;
+  int ch;
+  p = buf;
+  while ((ch = getchar()) != '\n' && ch != EOF) {
+*p++ = (char)ch;
+  }
+  *p++ = 0;
+  if (ch == EOF) {
+/* Handle EOF or error */
+  }
+}
+} // namespace test_getchar_bad
+
+namespace test_getchar_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buf[BUFFERSIZE];
+  int ch;
+  size_t index = 0;
+  size_t chars_read = 0;
+
+  while ((ch = getchar()) != '\n' && ch != EOF) {
+if (index < sizeof(buf) - 1) {
+  

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks for the review, hopefully if I ping @NoQ in every round, it will be 
green-marked soon.




Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:55
+  // they can cause a not null-terminated string. In this case we store the
+  // string is being not null-terminated in the 'NullTerminationMap'.
+  //

balazske wrote:
> The functions `gets`, `strcpy` return a null-terminated string according to 
> standard, probably `sprintf` too, and the `fscanf` `%s` output is 
> null-terminated too even if no length is specified (maybe it writes outside 
> of the specified buffer but null terminated).
From where do you observe such information? I have not seen anything 
`strcpy()`-specific in the standard. My observation clearly says that, if there 
is not enough space for the data you *could* even write to overlapping memory, 
or as people says, you could meet nasal demons. It is called *undefined* 
behavior.



Comment at: clang/test/Analysis/cert/str31-c.cpp:117
+  if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);

balazske wrote:
> Do we get a warning if the `+1` above is missing?
Test added.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77066: [analyzer] ApiModeling: Add buffer size arg constraint

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D77066#1953280 , @Charusso wrote:

>   getDynamicSizeWithOffset(State, MR, SVB) {
> Offset = State->getStoreManager().getStaticOffset(MR, SVB);
> ...
>   }
>


Hm, the MemRegion's offset should be great. I was thinking about if we would 
store SVal offsets in the Store.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77066/new/

https://reviews.llvm.org/D77066



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Given that the secondary behavior confuse people I have removed it for now. May 
if someone introduce a NullTerminationChecker then we introduce such option to 
warn on insecure calls instant. Thanks @balazske for influencing that change. 
@NoQ this project had a deadline like half year ago, could you smash that green 
button please?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 254061.
Charusso marked 6 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.

- Get rid of the secondary behavior for now.
- Fix review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/cert/str31-c-false-positive-suppression.cpp
  clang/test/Analysis/cert/str31-c-notes.cpp
  clang/test/Analysis/cert/str31-c.cpp

Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+void do_something(char *buffer);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {
+  }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+  char buf[128];
+  sprintf(buf, "%s.txt", name);
+  // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+  char buff[128];
+  snprintf(buff, sizeof(buff), "%s.txt", name);
+
+  do_something(buff);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buf[BUF_LENGTH];
+  if (fscanf(stdin, "%s", buf)) {
+// expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+  }
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buff[BUF_LENGTH];
+  if (fscanf(stdin, "%1023s", buff)) {
+do_something(buff);
+  }
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char prog_name[128];
+  strcpy(prog_name, name);
+  // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+
+  return 0;
+}
+
+void func(void) {
+  char buff[256];
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+  }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char *prog_name2 = (char *)malloc(strlen(name) + 1);
+  if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+  }
+
+  do_something(prog_name2);
+
+  free(prog_name2);
+  return 0;
+}
+
+void func(void) {
+  char *buff2;
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+  strcpy(buff2, editor);
+}
+
+do_something(buff2);
+free(buff2);
+  }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n - 1); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_good
+
+namespace test_getchar_bad {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buf[BUFFERSIZE];
+  char *p;
+  int ch;
+  p = buf;
+  while ((ch = getchar()) != '\n' && ch != EOF) {
+*p++ = (char)ch;
+  }
+  *p++ = 0;
+  if (ch == EOF) {
+/* Handle EOF or error */
+  }
+}

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 254051.
Charusso added a comment.

- Remove the last gymnastic.
- Rebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

Files:
  clang/docs/analyzer/developer-docs/DebugChecks.rst
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/memory-model.cpp

Index: clang/test/Analysis/memory-model.cpp
===
--- /dev/null
+++ clang/test/Analysis/memory-model.cpp
@@ -0,0 +1,114 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,cplusplus,debug.ExprInspection \
+// RUN:  -triple x86_64-unknown-linux-gnu \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t number, size_t size);
+void free(void *);
+
+struct S { int f; };
+
+void clang_analyzer_dump(int);
+void clang_analyzer_dump(const void *);
+void clang_analyzer_dumpExtent(int);
+void clang_analyzer_dumpExtent(const void *);
+void clang_analyzer_dumpElementCount(int);
+void clang_analyzer_dumpElementCount(const void *);
+
+void var_simple_ref() {
+  int a = 13;
+  clang_analyzer_dump(); // expected-warning {{a}}
+  clang_analyzer_dumpExtent(); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void var_simple_ptr(int *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning-re ^\(extent_\$1\{SymRegion\{reg_\$0\}\}\) / 4$
+}
+
+void var_array() {
+  int a[] = {1, 2, 3};
+  clang_analyzer_dump(a); // expected-warning {{Element{a,0 S64b,int}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void string() {
+  clang_analyzer_dump("foo"); // expected-warning {{Element{"foo",0 S64b,char}}}
+  clang_analyzer_dumpExtent("foo"); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount("foo"); // expected-warning {{4 S64b}}
+}
+
+void struct_simple_ptr(S *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning-re ^\(extent_\$1\{SymRegion\{reg_\$0\}\}\) / 4$
+}
+
+void field_ref(S a) {
+  clang_analyzer_dump(); // expected-warning {{a.f}}
+  clang_analyzer_dumpExtent(); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void field_ptr(S *a) {
+  clang_analyzer_dump(>f); // expected-warning {{SymRegion{reg_$0}.f}}
+  clang_analyzer_dumpExtent(>f); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(>f); // expected-warning {{1 S64b}}
+}
+
+void symbolic_array() {
+  int *a = new int[3];
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] a;
+}
+
+void symbolic_placement_new() {
+  char *buf = new char[sizeof(int) * 3];
+  int *a = new (buf) int(13);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] buf;
+}
+
+void symbolic_malloc() {
+  int *a = (int *)malloc(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  free(a);
+}
+
+void symbolic_alloca() {
+  int *a = (int *)alloca(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void symbolic_complex() {
+  int *a = (int *)malloc(4);
+  clang_analyzer_dumpExtent(a); // expected-warning {{4 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{1 S64b}}
+
+  int *b = (int *)realloc(a, sizeof(int) * 2);
+  

[PATCH] D77022: [analyzer] Use IgnoreImpCasts() instead of reimplementing it.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso marked an inline comment as done.
Charusso added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:512
+  return E->IgnoreImpCasts();
 }
 

thakis wrote:
> NoQ wrote:
> > Charusso wrote:
> > > I think it would make sense to remove the helper-function completely. 
> > > (Being used 2 times.)
> > Yup.
> I didn't do that because I liked the comment that this is for the _Nonnull 
> implicit ARC casts – if I inline the function I have to duplicate the comment.
Well, if you really like that, sure. I believe it is so common to avoid 
implicit casts we do not need to express it why.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77022/new/

https://reviews.llvm.org/D77022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77066: [analyzer] ApiModeling: Add buffer size arg constraint

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Please avoid to stuff in `CheckerContext` because this facility should be used 
by ExprEngine/Store as well.
Let us reword your API: `getDynamicSizeWithOffset(ProgramStateRef, SVal, 
SValBuilder &)`. Of course we are trying to obtain some buffer-ish size, that 
is the purpose of the entire API.
I also could imagine something like `getDynamicSizeMul(ProgramStateRef, const 
MemRegion &, const MemRegion &, SValBuilder &)`, as it is very common.

May it would make sense to use the API like:

  getDynamicSizeWithOffset(State, MR, SVB) {
Offset = State->getStoreManager().getStaticOffset(MR, SVB);
...
  }

This idea is similar to `MemRegionManager::getStaticSize(MR, SVB)`. Hopefully 
no-one needs dynamic offsets.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77066/new/

https://reviews.llvm.org/D77066



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73521: [analyzer] add-new-checker.py: Introduction

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D73521#1951776 , @ASDenysPetrov 
wrote:

> Will this utility affect Visual Studio builds?


The community owns like 50 build-bots, so it should affect all of them by 
default. I am hoping it will just work.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73521/new/

https://reviews.llvm.org/D73521



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73521: [analyzer] add-new-checker.py: Introduction

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 253777.
Charusso marked 3 inline comments as done.
Charusso added a comment.
Herald added a subscriber: ASDenysPetrov.

- Remove the test of creating a live checker, instead copy over the live 
checker when the script runs.
- Simplify the script by adding the new package to the end of the file.
- In case of the `checkers.rst` a non-alpha package is going to be added before 
the alpha packages.
- According to this change simplify the tests.
- `DummyChecker` -> `ExampleChecker`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73521/new/

https://reviews.llvm.org/D73521

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/ExampleChecker.cpp
  clang/test/Analysis/add-new-checker/add-main-package.rst
  clang/test/Analysis/add-new-checker/add-main-package.td
  clang/test/Analysis/add-new-checker/add-multiple-packages.rst
  clang/test/Analysis/add-new-checker/add-multiple-packages.td
  clang/test/Analysis/add-new-checker/check-add-new-checker.py
  clang/test/Analysis/add-new-checker/checker-name.rst
  clang/test/Analysis/add-new-checker/checker-name.td
  clang/test/Analysis/add-new-checker/flow-package-exist-checker.rst
  clang/test/Analysis/add-new-checker/flow-package-exist-checker.td
  clang/test/Analysis/add-new-checker/flow-package-not-exist.rst
  clang/test/Analysis/add-new-checker/flow-package-not-exist.td
  clang/test/Analysis/add-new-checker/lit.local.cfg.py
  clang/test/Analysis/example-checker.cpp
  clang/utils/analyzer/add-new-checker.py

Index: clang/utils/analyzer/add-new-checker.py
===
--- /dev/null
+++ clang/utils/analyzer/add-new-checker.py
@@ -0,0 +1,637 @@
+#!/usr/bin/env python
+#
+#===- add-new-checker.py - Creates a Static Analyzer checker --*- python -*-===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+#
+# Example usage: python add-new-checker.py alpha.package.Foo
+#
+#======#
+
+import argparse
+import json
+import os
+import re
+import subprocess
+
+
+class Checker:
+def __init__(self, packages, name):
+self.packages = packages
+self.name = name
+
+def name(self):
+return self.name
+
+def packages(self):
+return self.packages
+
+def __str__(self):
+return '.'.join(self.packages) + '.' + self.name
+
+
+# Obtain the longest known package-chain of the 'packages' chain. For example
+# the checker creation of 'alpha.core.a.Checker' would return ['alpha', 'core'].
+def get_best_package_match(packages, known_packages):
+best_package_match = []
+for i in range(len(packages)):
+temp_packages = packages[:i + 1]
+package_name = '.'.join(temp_packages)
+if package_name in known_packages:
+best_package_match = temp_packages
+return best_package_match
+
+
+# Put every additional package and the checker into an increasing size jagged
+# array. For example the checker creation of 'core.a.Checker' would return
+# '[['core', 'a'], ['core', 'a', 'Checker']]'.
+def get_addition(packages, name, best_package_match):
+addition = []
+match_count = len(best_package_match)
+for i, package in enumerate(packages):
+if i < match_count:
+continue
+addition.append(best_package_match + packages[match_count : i + 1])
+addition.append(packages + [name])
+return addition
+
+
+def add_checkers_entry(clang_path, checker, args):
+checkers_include_path = os.path.join(clang_path, 
+'include', 'clang', 'StaticAnalyzer', 'Checkers')
+checkers_path = os.path.join(checkers_include_path, 'Checkers.td')
+
+# This only could test '.td'.
+if args.is_test:
+checkers_path = args.test_path
+if not checkers_path.endswith('.td.tmp'):
+return
+
+packages = checker.packages
+name = checker.name
+
+# See if TableGen is found.
+try:
+devnull = open(os.devnull)
+subprocess.Popen([args.tblgen_path, '-h'], stdout=devnull,
+ stderr=devnull).communicate()
+except OSError as e:
+print("[error] llvm-tblgen is not found. Please specify it explicitly. "
+  "For example: '--tblgen-path llvm-tblgen'")
+raise
+
+# Load the checkers' TableGen.
+data = None
+try:
+data = subprocess.check_output([args.tblgen_path, '-dump-json',
+checkers_path,
+'-I=' + checkers_include_path])
+except subprocess.CalledProcessError as e:
+print('TableGen JSON 

[PATCH] D73521: [analyzer] add-new-checker.py: Introduction

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D73521#1923693 , @NoQ wrote:

> Or is each test run updating the repo? Can we simply make the script do `cat 
> DummyChecker.cpp | sed s/DummyChecker/$NAME/g > $NAME.cpp` and store the 
> checker only once?


It was updating, yes. The first idea was to copy over the live checker, but I 
wanted to make it as portable as possible.




Comment at: clang/utils/analyzer/add-new-checker.py:714-715
+help='the path to llvm-tblgen')
+parser.add_argument('--force-creation', dest='is_force_creation',
+action='store_true', help=argparse.SUPPRESS)
+parser.add_argument('--test', dest='is_test',

NoQ wrote:
> So, `--overwrite` and add help?
It was only made for the test. Removed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73521/new/

https://reviews.llvm.org/D73521



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77022: [analyzer] Use IgnoreImpCasts() instead of reimplementing it.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:512
+  return E->IgnoreImpCasts();
 }
 

I think it would make sense to remove the helper-function completely. (Being 
used 2 times.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77022/new/

https://reviews.llvm.org/D77022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 253394.
Charusso marked 19 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.
Herald added a subscriber: ASDenysPetrov.

- Fix `VLASizeChecker`'s multi-dimensional array early return.
- So that fix the regression in test `misc-ps-region-store.m`.
- Fix tests that need regex.
- Add documentation about `dumpExtent`, `dumpElementCount`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

Files:
  clang/docs/analyzer/developer-docs/DebugChecks.rst
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/memory-model.cpp

Index: clang/test/Analysis/memory-model.cpp
===
--- /dev/null
+++ clang/test/Analysis/memory-model.cpp
@@ -0,0 +1,114 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,cplusplus,debug.ExprInspection \
+// RUN:  -triple x86_64-unknown-linux-gnu \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t number, size_t size);
+void free(void *);
+
+struct S { int f; };
+
+void clang_analyzer_dump(int);
+void clang_analyzer_dump(const void *);
+void clang_analyzer_dumpExtent(int);
+void clang_analyzer_dumpExtent(const void *);
+void clang_analyzer_dumpElementCount(int);
+void clang_analyzer_dumpElementCount(const void *);
+
+void var_simple_ref() {
+  int a = 13;
+  clang_analyzer_dump(); // expected-warning {{a}}
+  clang_analyzer_dumpExtent(); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void var_simple_ptr(int *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning-re ^\(extent_\$1\{SymRegion\{reg_\$0\}\}\) / 4$
+}
+
+void var_array() {
+  int a[] = {1, 2, 3};
+  clang_analyzer_dump(a); // expected-warning {{Element{a,0 S64b,int}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+}
+
+void string() {
+  clang_analyzer_dump("foo"); // expected-warning {{Element{"foo",0 S64b,char}}}
+  clang_analyzer_dumpExtent("foo"); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount("foo"); // expected-warning {{4 S64b}}
+}
+
+void struct_simple_ptr(S *a) {
+  clang_analyzer_dump(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_dumpElementCount(a); // expected-warning-re ^\(extent_\$1\{SymRegion\{reg_\$0\}\}\) / 4$
+}
+
+void field_ref(S a) {
+  clang_analyzer_dump(); // expected-warning {{a.f}}
+  clang_analyzer_dumpExtent(); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(); // expected-warning {{1 S64b}}
+}
+
+void field_ptr(S *a) {
+  clang_analyzer_dump(>f); // expected-warning {{SymRegion{reg_$0}.f}}
+  clang_analyzer_dumpExtent(>f); // expected-warning {{4 S64b}}
+  clang_analyzer_dumpElementCount(>f); // expected-warning {{1 S64b}}
+}
+
+void symbolic_array() {
+  int *a = new int[3];
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] a;
+}
+
+void symbolic_placement_new() {
+  char *buf = new char[sizeof(int) * 3];
+  int *a = new (buf) int(13);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  delete[] buf;
+}
+
+void symbolic_malloc() {
+  int *a = (int *)malloc(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 U64b}}
+  clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}}
+  free(a);
+}
+
+void symbolic_alloca() {
+  int *a = (int *)alloca(12);
+  clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_dumpExtent(a); // expected-warning {{12 U64b}}
+  

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:25
 
-/// Get the stored dynamic size for the region \p MR.
+/// \returns The stored dynamic size for the region \p MR.
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,

NoQ wrote:
> Naming: I believe we should keep using the word "Extent". There's no need to 
> introduce a new term for the existing concept; it makes it harder to explain 
> on the mailing list :) Let's make a follow-up patch to change the naming back 
> (so that not to screw the review).
Since then I have changed my mind when I have read about Environment and Store 
in a book. Sure, next up.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+   const CXXNewExpr *NE,
+   const LocationContext *LCtx, SValBuilder );

NoQ wrote:
> This function is probably going to be used exactly once in the whole code. 
> There's no need to turn it into a public API.
It is being used 3 times already, so I believe it is a cool API.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:99
+  .Case("clang_analyzer_region", 
::analyzerRegion)
+  .Case("clang_analyzer_size", 
::analyzerDumpSize)
+  .Case("clang_analyzer_elementCount",

NoQ wrote:
> `clang_analyzer_dump_extent()`? Or just 
> `clang_analyzer_dump(clang_analyzer_getExtent())`.
I like the shorter version, but of course I have seen the longer version.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:283
+  llvm::raw_svector_ostream Out(Msg);
+  Out << MR;
+  reportBug(Out.str(), C);

NoQ wrote:
> So, how is it different from the existing `clang_analyzer_dump()`?
I wanted to make it for the derived regions, but then I have realized I am lazy 
to dig into the buggier parts of the Analyzer. Removed.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+  QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+: CE->getArg(0)->IgnoreParenImpCasts()->getType();
+

NoQ wrote:
> How is this better than `getValueType()`? Are you sure you're not getting a 
> pointer type instead in the `!TVR` case? I.e., can you test this on a 
> non-heap `SymRegion`?
> How is this better than getValueType()?
Consistency. We get the static ~~size~~ extent by getting the desugared type 
which most likely just an extra overhead.

> Are you sure you're not getting a pointer type instead in the !TVR case? 
> I.e., can you test this on a non-heap SymRegion?
The issue was the `var_region_simple_ptr` test: we need to use regex for 
checking after the `}}` token where `/ 4` is the correct result. I did not 
really get why the test pass.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+if (CI->getValue().isUnsigned())
+  Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+

NoQ wrote:
> Charusso wrote:
> > That one is interesting. Some of the checkers / SValBuilder(?) generate 
> > unsigned integers which should not happen, I believe. May we need a FIXME 
> > and an assertion about signedness. What do you think?
> `SymbolExtent` has type `size_t` and both `malloc` and `operator new` accept 
> `size_t` as a parameter. Therefore everything needs to be //unsigned// and we 
> need to assert this.
> 
> That said, array indexes are //signed// (as per implementation of 
> `ElementRegion`).
It was a premature optimization for consistency. I will leave the signedness as 
is, FIXME added.



Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+tmp2[x] = am; // expected-warning \
+{{Access out-of-bound array element (buffer overflow)}}
   }

NoQ wrote:
> Charusso wrote:
> > That is the single regression which I do not get.
> Well, please debug. Like, look at the full report, dump egraph, see what 
> changed. Try to `creduce` the example further under the condition "behavior 
> changed with the patch", maybe that'll clear something up (though if it's a 
> true positive after creduce it doesn't guarantee that it's a true positive 
> before creduce).
Given that I have no access for `rdar` I did my hand-reduce and the solution is 
the following:
```lang=c
// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin9 \
// RUN:  -analyzer-checker=core,alpha.security.ArrayBound \
// RUN:  -verify %s

static void RDar8424269_B(int unknown) {
  unsigned char tmp2t[1][unknown];
  tmp2t[1][13] = 0; // expected-warning \
{{Access out-of-bound array element (buffer 

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 6 inline comments as done.
Charusso added a comment.

"To prevent such errors, either limit copies through truncation or, preferably, 
ensure that the destination is of sufficient size to hold the character data" - 
from the rule's page.
Most of the projects are fine truncating by hand because the write happens in 
somewhat well-bounded strings: IP-addresses, names, numbers... I wanted to make 
this as practical as possible. Until you are having a null-terminated string 
without being read, you are most likely fine. Feel free to try this out, 
probably you would already understand the `WarnOnCall` option very well.




Comment at: clang/docs/analyzer/checkers.rst:1935
+
+alpha.security.cert.str.31c
+"""

balazske wrote:
> Charusso wrote:
> > Szelethus wrote:
> > > balazske wrote:
> > > > There are already more checkers that can check for CERT related 
> > > > problems but not specially made for these. These checkers do not reside 
> > > > in this new `cert` group. And generally a checker does not check for 
> > > > specifically a CERT rule, instead for more of them or other things too, 
> > > > or more checkers can detect a single rule. (And the user can think that 
> > > > only these CERT rules are checkable that exist in this package, that is 
> > > > not true.) So I do not like the introduction of this new `cert` 
> > > > package. (The documentation of existing checkers lists if the checker 
> > > > is designed for a CERT rule.)
> > > I disagree to some extent. I think it would be great to have a `cert` 
> > > package that houses all checkers for each of the rules with the addition 
> > > of checker aliases. Clang-tidy has something similar as well!
> > I designed the checker only for the rule STR31-C that is why I have picked 
> > package `cert`. Clang-Tidy could aliasing checks. For example the check 
> > could be in the `bugprone` category aliased to `cert` and both could 
> > trigger the analysis.
> > 
> > > the user can think that only these CERT rules are checkable
> > We only need to move `security.FloatLoopCounter` under the `cert` package. 
> > What else SEI CERT rules are finished off other than package `cert`? It is 
> > not my fault if someone could not package well or could not catch most of 
> > the issues of a SEI CERT rule or could not reach the SEI CERT group to note 
> > the fact the Analyzer catch a very tiny part of a rule. However, this patch 
> > package well and could catch most of the STR31-C rule.
> So I can move this checker: D71510 into `alpha.security.cert.err.33c`?
Well, not exactly. No one should care about alpha checkers except advanced 
Static Analyzer developers like you do right now. Since alpha is a dead-zone 
you can put your work anywhere. The core issue was Ted's placing of CERT rules.

I have written the first CERT-checker after Ted's, which introduced such 
packaging, then it became dead, so I have lost interest to make the 
package-aliasing a thing. If you wish to move to `cert`, feel free, but please 
note that, your first idea of bad-packaging is right. We really need the 
functionality of package-aliasing but I have ran out of time and the project 
died as well.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803
+
+} // end "cert.str"
+

balazske wrote:
> Charusso wrote:
> > balazske wrote:
> > > NoQ wrote:
> > > > `alpha.cert.str`.
> > > This text may be hard to understand. The option means "if it is off, the 
> > > problem report happens only if there is no hand-written null termination 
> > > of the string"? I looked at the CERT rule but did not find out why is 
> > > null termination by hand important here. (I did not look at the code to 
> > > find out what it does.) And "WarnOnCall" suggests that there is warning 
> > > made on calls if on, or not at all or at other location if off, is this 
> > > correct?
> > You let the buffer overflow by a bugprone function call, then you adjust 
> > the null-terminator by simply `buf[size] = '\0'`, then you made sure the 
> > buffer cannot overflow, since you have terminated it. It is a very common 
> > idiom therefore I do not think a hand-written null-termination is a 
> > security issue. The SEI CERT rules are all theoretical so you will not find 
> > anything useful in practice. My solution is practical.
> > 
> > > This text may be hard to understand.
> > Please note that this text only made for Static Analyzer developers. Let us 
> > rephrase it then:
> > 
> > > Whether the checker needs to warn on the bugprone function calls 
> > > immediately or look for bugprone hand-written null-termination of 
> > > bugprone function call made strings. It is a common idiom to 
> > > null-terminate the string by hand after the insecure function call 
> > > produce the string which could be misused so that it is on by default. It 
> > > is useful to turn it off to reduce the noise of the checker, 

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 4 inline comments as done.
Charusso added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803
+
+} // end "cert.str"
+

balazske wrote:
> NoQ wrote:
> > `alpha.cert.str`.
> This text may be hard to understand. The option means "if it is off, the 
> problem report happens only if there is no hand-written null termination of 
> the string"? I looked at the CERT rule but did not find out why is null 
> termination by hand important here. (I did not look at the code to find out 
> what it does.) And "WarnOnCall" suggests that there is warning made on calls 
> if on, or not at all or at other location if off, is this correct?
You let the buffer overflow by a bugprone function call, then you adjust the 
null-terminator by simply `buf[size] = '\0'`, then you made sure the buffer 
cannot overflow, since you have terminated it. It is a very common idiom 
therefore I do not think a hand-written null-termination is a security issue. 
The SEI CERT rules are all theoretical so you will not find anything useful in 
practice. My solution is practical.

> This text may be hard to understand.
Please note that this text only made for Static Analyzer developers. Let us 
rephrase it then:

> Whether the checker needs to warn on the bugprone function calls immediately 
> or look for bugprone hand-written null-termination of bugprone function call 
> made strings. It is a common idiom to null-terminate the string by hand after 
> the insecure function call produce the string which could be misused so that 
> it is on by default. It is useful to turn it off to reduce the noise of the 
> checker, because people usually null-terminate the string by hand immediately 
> after the bugprone function call produce the string.

Do you buy that?



Comment at: clang/lib/StaticAnalyzer/Checkers/AllocationState.h:28
 
+/// \returns The MallocBugVisitor.
+std::unique_ptr getMallocBRVisitor(SymbolRef Sym);

balazske wrote:
> This documentation could be improved or left out.
Totally, I have already forgotten I wanted to remove it. Thanks!



Comment at: clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp:68
+
+  do_something(dest);
+  // expected-warning@-1 {{'dest' is not null-terminated}}

balazske wrote:
> Maybe I have wrong expectations about what this checker does but did 
> understand it correctly yet. The main problem here is that `dest` may be not 
> large enough (if size of `src` is not known). This would be a better text for 
> the error message? And if this happens (the write outside) it is already a 
> fatal error, can cause crash. If no buffer overflow happens the terminating 
> zero is copied from `src`, so why would `dst` be not null terminated?
I like that error message because that is the truth, that is the issue. If it 
would crash so the operating system would detect a fatal error we should not 
develop Static Analyzer and vulnerabilities would not exist. That would be 
cool, btw.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 5 inline comments as done.
Charusso added a comment.

Thanks for the feedback! Given that it will remain an `alpha` checker for a 
long time (~1 year), no one really should use it.




Comment at: clang/docs/analyzer/checkers.rst:1935
+
+alpha.security.cert.str.31c
+"""

Szelethus wrote:
> balazske wrote:
> > There are already more checkers that can check for CERT related problems 
> > but not specially made for these. These checkers do not reside in this new 
> > `cert` group. And generally a checker does not check for specifically a 
> > CERT rule, instead for more of them or other things too, or more checkers 
> > can detect a single rule. (And the user can think that only these CERT 
> > rules are checkable that exist in this package, that is not true.) So I do 
> > not like the introduction of this new `cert` package. (The documentation of 
> > existing checkers lists if the checker is designed for a CERT rule.)
> I disagree to some extent. I think it would be great to have a `cert` package 
> that houses all checkers for each of the rules with the addition of checker 
> aliases. Clang-tidy has something similar as well!
I designed the checker only for the rule STR31-C that is why I have picked 
package `cert`. Clang-Tidy could aliasing checks. For example the check could 
be in the `bugprone` category aliased to `cert` and both could trigger the 
analysis.

> the user can think that only these CERT rules are checkable
We only need to move `security.FloatLoopCounter` under the `cert` package. What 
else SEI CERT rules are finished off other than package `cert`? It is not my 
fault if someone could not package well or could not catch most of the issues 
of a SEI CERT rule or could not reach the SEI CERT group to note the fact the 
Analyzer catch a very tiny part of a rule. However, this patch package well and 
could catch most of the STR31-C rule.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h:22
+extern const char *const CXXObjectLifecycle;
+extern const char *const SecurityError;
+} // namespace categories

balazske wrote:
> Are there already not other checkers that find security related bugs (the 
> taint checker?)? Why do these not use a `SecurityError`? It is not bad to 
> have a `SecurityError` but maybe there is a reason why was it not there 
> already. If these categories are exclusive it is hard to find out what 
> problem (probably already existing bug type in other checkers) belongs to 
> what category (it can be for this checker `UnixAPI` or `MemoryError` too?). 
We have only three pure security checkers: `security.insecureAPI`, 
`security.FloatLoopCounter` (FLP30-C, FLP30-CPP) and 
`alpha.security.cert.pos.34c`. The non-alpha checkers are very old, but Ted 
definitely made that category:
```
  const char *bugType = "Floating point variable used as loop counter";
  BR.EmitBasicReport(bugType, "Security", os.str().c_str(),
 FS->getLocStart(), ranges.data(), ranges.size());
```
- 
https://github.com/llvm/llvm-project/commit/9c49762776b4d2cb16911dec0c873c90a6508968

Every other security-ish checker does not catch the CERT rule's examples. May 
if the checker evolves I would pick MemoryError, but it will not evolve soon.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76622: [analyzer] ConstraintManager - use EXPENSIVE_CHECKS instead of (gcc specific) __OPTIMIZE__ guard

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

Nice catch, thanks! We have some FIXMEs about MSVC sadly and I was thinking 
about the same change back in the days.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76622/new/

https://reviews.llvm.org/D76622



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76229: [clang-tidy] Added PlacementNewStorageCheck

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D76229#1925363 , @f00kat wrote:

> In D76229#1925268 , @aaron.ballman 
> wrote:
>
> > Have you considered making this a static analyzer check as opposed to a 
> > clang-tidy check? I think using dataflow analysis will really reduce the 
> > false positives because it will be able to track the allocation and 
> > alignment data across control flow.
>
>
> I have never try to write static analyzer before. Okay, I will look at 
> examples of how to work with dataflow.


To getting started with the Analyzer please visit @NoQ's (tiny bit outdated) 
booklet: https://github.com/haoNoQ/clang-analyzer-guide

Advertisement:
If D69726  lands we will have an arsenal of 
APIs for helping dynamic size based checkers.
If D73521  lands it should be easier to 
getting started with the Analyzer, but now you could visit what is our current 
API.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76229/new/

https://reviews.llvm.org/D76229



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75529: [analyzer] Limit UCharMax to min of max uchar or max int

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D75529#1920796 , @vabridgers wrote:

> I believe all comments have been addressed. Please let me know if there's 
> anything else required. Thanks


I think you have solved everything. Do you have commit access? May you would 
request it: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75529/new/

https://reviews.llvm.org/D75529



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73521: [analyzer] add-new-checker.py: Introduction

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked an inline comment as done.
Charusso added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:24
 def CoreBuiltin : Package<"builtin">, ParentPackage, Hidden;
-def CoreUninitialized  : Package<"uninitialized">, ParentPackage;
+def CoreUninitialized : Package<"uninitialized">, ParentPackage;
 def CoreAlpha : Package<"core">, ParentPackage;

The script is allergic to human issues. The followup will be to normalize the 
necessary files.



Comment at: clang/utils/analyzer/add-new-checker.py:79-81
+data = subprocess.check_output(['llvm-tblgen', '-dump-json',
+checkers_path,
+'-I=' + checkers_include_path])

NoQ wrote:
> `llvm-tblgen` needs to be in the `PATH`, right? What if LLVM isn't installed 
> on the host system or has the wrong version?
> 
> We might have to either tell the user to specify it explicitly in the 
> invocation (and then update the LIT substitution to explicitly use the right 
> binary), or, ideally, try to get the script installed into the build 
> directory so that it could find the freshly built `llvm-tblgen` binary 
> relative to itself.
Hm, I am not a magician, so my assumption one has the llvm/clang therefore the 
llvm-tblgen available. If not, it warns you and you need to specify. May if you 
want to make sure it "just works" please adjust it later.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73521/new/

https://reviews.llvm.org/D73521



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73521: [analyzer] add-new-checker.py: Introduction

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 250020.
Charusso marked 3 inline comments as done.
Charusso retitled this revision from "[analyzer][WIP] add-new-checker.py: 
Introduction" to "[analyzer] add-new-checker.py: Introduction".
Charusso added a comment.
Herald added a subscriber: mgorny.

- Try to invoke TableGen, if that fails the user need to specify the path to it.
- The script actually creates a real world (hidden) checker.
  - This checker always made with the build invocation.
  - Its test file always made with the build invocation.
  - Everything else remain as is.
- (calculated: DummyChecker.cpp (100 lines))


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73521/new/

https://reviews.llvm.org/D73521

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/DummyChecker.cpp
  clang/test/Analysis/add-new-checker/add-main-package.rst
  clang/test/Analysis/add-new-checker/add-main-package.td
  clang/test/Analysis/add-new-checker/add-multiple-packages.rst
  clang/test/Analysis/add-new-checker/add-multiple-packages.td
  clang/test/Analysis/add-new-checker/add-new-checker.test
  clang/test/Analysis/add-new-checker/check-add-new-checker.py
  clang/test/Analysis/add-new-checker/checker-name.rst
  clang/test/Analysis/add-new-checker/checker-name.td
  clang/test/Analysis/add-new-checker/flow-package-exist-checker.rst
  clang/test/Analysis/add-new-checker/flow-package-exist-checker.td
  clang/test/Analysis/add-new-checker/flow-package-not-exist-layering.rst
  clang/test/Analysis/add-new-checker/flow-package-not-exist-layering.td
  clang/test/Analysis/add-new-checker/flow-package-not-exist.rst
  clang/test/Analysis/add-new-checker/flow-package-not-exist.td
  clang/test/Analysis/add-new-checker/lit.local.cfg.py
  clang/test/Analysis/dummy-checker.cpp
  clang/utils/analyzer/add-new-checker.py

Index: clang/utils/analyzer/add-new-checker.py
===
--- /dev/null
+++ clang/utils/analyzer/add-new-checker.py
@@ -0,0 +1,775 @@
+#!/usr/bin/env python
+#
+#===- add-new-checker.py - Creates a Static Analyzer checker --*- python -*-===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+#
+# Example usage: python add-new-checker.py alpha.package.Foo
+#
+#======#
+
+import argparse
+import json
+import os
+import re
+import subprocess
+
+
+class Checker:
+def __init__(self, packages, name):
+self.packages = packages
+self.name = name
+
+def name(self):
+return self.name
+
+def packages(self):
+return self.packages
+
+def __str__(self):
+return '.'.join(self.packages) + '.' + self.name
+
+
+# Obtain the longest known package-chain of the 'packages' chain. For example
+# the checker creation of 'alpha.core.a.Checker' would return ['alpha', 'core'].
+def get_best_package_match(packages, known_packages):
+best_package_match = []
+for i in range(len(packages)):
+temp_packages = packages[:i + 1]
+package_name = '.'.join(temp_packages)
+if package_name in known_packages:
+best_package_match = temp_packages
+return best_package_match
+
+
+# Put every additional package and the checker into an increasing size jagged
+# array. For example the checker creation of 'core.a.Checker' would return
+# '[['core', 'a'], ['core', 'a', 'Checker']]'.
+def get_addition(packages, name, best_package_match):
+addition = []
+match_count = len(best_package_match)
+for i, package in enumerate(packages):
+if i < match_count:
+continue
+addition.append(best_package_match + packages[match_count : i + 1])
+addition.append(packages + [name])
+return addition
+
+
+def add_checkers_entry(clang_path, checker, args):
+checkers_include_path = os.path.join(clang_path, 
+'include', 'clang', 'StaticAnalyzer', 'Checkers')
+checkers_path = os.path.join(checkers_include_path, 'Checkers.td')
+
+# This only could test '.td'.
+if args.is_test:
+checkers_path = args.test_path
+if not checkers_path.endswith('.td.tmp'):
+return
+
+packages = checker.packages
+name = checker.name
+
+# See if TableGen is found.
+try:
+devnull = open(os.devnull)
+subprocess.Popen([args.tblgen_path, '-h'], stdout=devnull,
+ stderr=devnull).communicate()
+except OSError as e:
+print('[error] llvm-tblgen is not found, specify it explicitly.')
+exit()
+
+# Load the checkers' TableGen.
+data = None
+try:
+data = 

[PATCH] D75529: [analyzer] Limit UCharMax to min of max uchar or max int

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.

In D75529#1912849 , @vabridgers wrote:

> @Charusso, I cannot find a way to create a test for this change, since this 
> was found using an architecture that supports 16-bit chars.


I really felt that we could write a test that using a target which behaves like 
that. I am not that awesome with triples, sorry. If I am right we are good to 
go without @NoQ's response (he is the maintainer of the Analyzer). Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75529/new/

https://reviews.llvm.org/D75529



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74467: [analyzer] Teach scan-build how to rebuild index.html without analyzing.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D74467#1905416 , @NoQ wrote:

> In D74467#1905011 , @Charusso wrote:
>
> > Could you mention how to use this feature in the Summary please?
> >  And something is not right, I have tried it:
> >
> >   Use of uninitialized value $Clang in concatenation (.) or string at 
> > /llvm-project/clang/tools/scan-build/bin/scan-build line 1895.
> >
>
>
> Uh-oh. Does this still show up if you specify `--use-analyzer` explicitly? 
> Because `%scan_build` in tests includes this flag because otherwise it has no 
> knowledge of the clang build directory.
>
> >   sh: 1: : Permission denied
>
> This looks like a separate problem, dunno.


Well, then please mention the proper usage is:

  cd reports
  scan-build --use-analyzer clang --generate-index-only .

With that none of the concatenation and `sh` permission issue shows up. However 
it would be a nice QoL if that extra flag is not needed to generate HTML.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74467/new/

https://reviews.llvm.org/D74467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74467: [analyzer] Teach scan-build how to rebuild index.html without analyzing.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Could you mention how to use this feature in the Summary please?

  cd reports
  scan-build --generate-index-only .

---

And something is not right, I have tried it:

  Use of uninitialized value $Clang in concatenation (.) or string at 
/llvm-project/clang/tools/scan-build/bin/scan-build line 1895.
  sh: 1: : Permission denied

But it made the `index.html` and works like a charm.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74467/new/

https://reviews.llvm.org/D74467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73520: [analyzer] BugReporterVisitors: Refactor and documentation

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 248110.
Charusso marked 4 inline comments as done.
Charusso added a comment.
Herald added subscribers: martong, steakhal.

- Make the tags robust and more unique.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73520/new/

https://reviews.llvm.org/D73520

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

Index: clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/Version.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
@@ -238,8 +239,9 @@
 // FIXME: What should be reported here?
 break;
   case PathDiagnosticPiece::Event:
-return Piece.getTagStr() == "ConditionBRVisitor" ? Importance::Important
- : Importance::Essential;
+return Piece.getTag() == ConditionBRVisitor::getTag()
+   ? Importance::Important
+   : Importance::Essential;
   case PathDiagnosticPiece::ControlFlow:
 return Importance::Unimportant;
   }
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1,15 +1,10 @@
-//===- BugReporterVisitors.cpp - Helpers for reporting bugs ---===//
+//===- BugReporterVisitors.cpp - Bug explaining and suppression -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
-//
-//  This file defines a set of BugReporter "visitors" which can be used to
-//  enhance the diagnostics reported for a bug.
-//
-//===--===//
 
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
 #include "clang/AST/ASTContext.h"
@@ -82,18 +77,6 @@
   return nullptr;
 }
 
-/// Given that expression S represents a pointer that would be dereferenced,
-/// try to find a sub-expression from which the pointer came from.
-/// This is used for tracking down origins of a null or undefined value:
-/// "this is null because that is null because that is null" etc.
-/// We wipe away field and element offsets because they merely add offsets.
-/// We also wipe away all casts except lvalue-to-rvalue casts, because the
-/// latter represent an actual pointer dereference; however, we remove
-/// the final lvalue-to-rvalue cast before returning from this function
-/// because it demonstrates more clearly from where the pointer rvalue was
-/// loaded. Examples:
-///   x->y.z  ==>  x (lvalue)
-///   foo()->y.z  ==>  foo() (rvalue)
 const Expr *bugreporter::getDerefExpr(const Stmt *S) {
   const auto *E = dyn_cast(S);
   if (!E)
@@ -362,21 +345,17 @@
 SM(MmrMgr.getContext().getSourceManager()),
 PP(MmrMgr.getContext().getPrintingPolicy()), TKind(TKind) {}
 
-  void Profile(llvm::FoldingSetNodeID ) const override {
-static int Tag = 0;
-ID.AddPointer();
-ID.AddPointer(RegionOfInterest);
-  }
-
-  void *getTag() const {
-static int Tag = 0;
-return static_cast();
-  }
-
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
BugReporterContext ,
PathSensitiveBugReport ) override;
 
+  static const char *getTag() { return "NoStoreFuncVisitor"; }
+
+  void Profile(llvm::FoldingSetNodeID ) const override {
+ID.AddPointer(getTag());
+ID.AddPointer(RegionOfInterest);
+  }
+
 private:
   /// Attempts to find the region of interest in a given record decl,
   /// by either following the base classes or fields.
@@ -837,10 +816,7 @@
   R->getAs(), V));
   }
 
-  void* getTag() const {
-static int Tag = 0;
-return static_cast();
-  }
+  static const char *getTag() { return "MacroNullReturnSuppressionVisitor"; }
 
   void Profile(llvm::FoldingSetNodeID ) const override {
 ID.AddPointer(getTag());
@@ -903,13 +879,10 @@
   : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed),
 Options(Options), TKind(TKind) {}
 
-  static void *getTag() {
-static int Tag = 0;
-return static_cast();
-  }
+  static const char *getTag() { return 

[PATCH] D73520: [analyzer] BugReporterVisitors: Refactor and documentation

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:44
 
-/// BugReporterVisitors are used to add custom diagnostics along a path.
+/// BugReporterVisitors are used to add custom diagnostics along a \emoji bug
+/// path. They also could serve false positive suppression.

Szelethus wrote:
> I suspect `\emoji` wasn't intentional here, given that it needs another 
> argument :^)
Well, it is "So Pro" (- Apple) and I really like that. Someone wrote "along a 
path", but we are definitely mentioning the  path.
{F11475626}


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73520/new/

https://reviews.llvm.org/D73520



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73729: [analyzer] AnalyzerOptions: Remove 'fixits-as-remarks'

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

[Achievement unlocked] 3 green marks.

Thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73729/new/

https://reviews.llvm.org/D73729



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73519: [analyzer] AnalysisDeclContext: Refactor and documentation

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7e1a6ca9e89c: [analyzer] AnalysisDeclContext: Refactor and 
documentation (authored by Charusso).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73519/new/

https://reviews.llvm.org/D73519

Files:
  clang/include/clang/Analysis/AnalysisDeclContext.h
  clang/lib/Analysis/AnalysisDeclContext.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp

Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -842,8 +842,7 @@
 return SFC;
 }
 if (const auto *BC = dyn_cast(LC)) {
-  const auto *BR =
-  static_cast(BC->getContextData());
+  const auto *BR = static_cast(BC->getData());
   // FIXME: This can be made more efficient.
   for (BlockDataRegion::referenced_vars_iterator
I = BR->referenced_vars_begin(),
Index: clang/lib/Analysis/AnalysisDeclContext.cpp
===
--- clang/lib/Analysis/AnalysisDeclContext.cpp
+++ clang/lib/Analysis/AnalysisDeclContext.cpp
@@ -52,16 +52,16 @@
 
 using ManagedAnalysisMap = llvm::DenseMap;
 
-AnalysisDeclContext::AnalysisDeclContext(AnalysisDeclContextManager *Mgr,
- const Decl *d,
- const CFG::BuildOptions )
-: Manager(Mgr), D(d), cfgBuildOptions(buildOptions) {
+AnalysisDeclContext::AnalysisDeclContext(AnalysisDeclContextManager *ADCMgr,
+ const Decl *D,
+ const CFG::BuildOptions )
+: ADCMgr(ADCMgr), D(D), cfgBuildOptions(Options) {
   cfgBuildOptions.forcedBlkExprs = 
 }
 
-AnalysisDeclContext::AnalysisDeclContext(AnalysisDeclContextManager *Mgr,
- const Decl *d)
-: Manager(Mgr), D(d) {
+AnalysisDeclContext::AnalysisDeclContext(AnalysisDeclContextManager *ADCMgr,
+ const Decl *D)
+: ADCMgr(ADCMgr), D(D) {
   cfgBuildOptions.forcedBlkExprs = 
 }
 
@@ -96,8 +96,8 @@
 Stmt *Body = FD->getBody();
 if (auto *CoroBody = dyn_cast_or_null(Body))
   Body = CoroBody->getBody();
-if (Manager && Manager->synthesizeBodies()) {
-  Stmt *SynthesizedBody = Manager->getBodyFarm().getBody(FD);
+if (ADCMgr && ADCMgr->synthesizeBodies()) {
+  Stmt *SynthesizedBody = ADCMgr->getBodyFarm().getBody(FD);
   if (SynthesizedBody) {
 Body = SynthesizedBody;
 IsAutosynthesized = true;
@@ -107,8 +107,8 @@
   }
   else if (const auto *MD = dyn_cast(D)) {
 Stmt *Body = MD->getBody();
-if (Manager && Manager->synthesizeBodies()) {
-  Stmt *SynthesizedBody = Manager->getBodyFarm().getBody(MD);
+if (ADCMgr && ADCMgr->synthesizeBodies()) {
+  Stmt *SynthesizedBody = ADCMgr->getBodyFarm().getBody(MD);
   if (SynthesizedBody) {
 Body = SynthesizedBody;
 IsAutosynthesized = true;
@@ -309,19 +309,17 @@
 BodyFarm ::getBodyFarm() { return FunctionBodyFarm; }
 
 const StackFrameContext *
-AnalysisDeclContext::getStackFrame(LocationContext const *Parent, const Stmt *S,
-   const CFGBlock *Blk, unsigned BlockCount,
-   unsigned Idx) {
-  return getLocationContextManager().getStackFrame(this, Parent, S, Blk,
-   BlockCount, Idx);
+AnalysisDeclContext::getStackFrame(const LocationContext *ParentLC,
+   const Stmt *S, const CFGBlock *Blk,
+   unsigned BlockCount, unsigned Index) {
+  return getLocationContextManager().getStackFrame(this, ParentLC, S, Blk,
+   BlockCount, Index);
 }
 
-const BlockInvocationContext *
-AnalysisDeclContext::getBlockInvocationContext(const LocationContext *parent,
-   const BlockDecl *BD,
-   const void *ContextData) {
-  return getLocationContextManager().getBlockInvocationContext(this, parent,
-   BD, ContextData);
+const BlockInvocationContext *AnalysisDeclContext::getBlockInvocationContext(
+const LocationContext *ParentLC, const BlockDecl *BD, const void *Data) {
+  return getLocationContextManager().getBlockInvocationContext(this, ParentLC,
+   BD, Data);
 }
 
 bool AnalysisDeclContext::isInStdNamespace(const Decl *D) {
@@ -340,9 +338,10 @@
 }
 
 LocationContextManager ::getLocationContextManager() {
-  assert(Manager &&
- "Cannot create LocationContexts without an 

[PATCH] D73729: [analyzer] AnalyzerOptions: Remove 'fixits-as-remarks'

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGabdd33c86a34: [analyzer] AnalyzerOptions: Remove 
fixits-as-remarks (authored by Charusso).

Changed prior to commit:
  https://reviews.llvm.org/D73729?vs=241507=248106#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73729/new/

https://reviews.llvm.org/D73729

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/virtualcall-fixit.cpp
  clang/test/Analysis/virtualcall-fixits.cpp

Index: clang/test/Analysis/virtualcall-fixits.cpp
===
--- clang/test/Analysis/virtualcall-fixits.cpp
+++ clang/test/Analysis/virtualcall-fixits.cpp
@@ -1,10 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
 // RUN: -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \
 // RUN: %s 2>&1 | FileCheck -check-prefix=TEXT %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+
+// RUN: %check_analyzer_fixit %s %t \
+// RUN: -analyzer-checker=core,optin.cplusplus.VirtualCall \
 // RUN: -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \
-// RUN: -analyzer-config fixits-as-remarks=true \
-// RUN: -analyzer-output=plist -o %t.plist -verify %s
+// RUN: -analyzer-output=plist -o %t.plist
 // RUN: cat %t.plist | FileCheck -check-prefix=PLIST %s
 
 struct S {
@@ -12,7 +13,9 @@
   S() {
 foo();
 // expected-warning@-1{{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
-// expected-remark@-2{{5-5: 'S::'}}
+// CHECK-FIXES:  S() {
+// CHECK-FIXES-NEXT:   S::foo();
+// CHECK-FIXES-NEXT: }
   }
   ~S();
 };
@@ -30,12 +33,12 @@
 // PLIST-NEXT:remove_range
 // PLIST-NEXT:
 // PLIST-NEXT: 
-// PLIST-NEXT:  line13
+// PLIST-NEXT:  line14
 // PLIST-NEXT:  col5
 // PLIST-NEXT:  file0
 // PLIST-NEXT: 
 // PLIST-NEXT: 
-// PLIST-NEXT:  line13
+// PLIST-NEXT:  line14
 // PLIST-NEXT:  col4
 // PLIST-NEXT:  file0
 // PLIST-NEXT: 
Index: clang/test/Analysis/virtualcall-fixit.cpp
===
--- clang/test/Analysis/virtualcall-fixit.cpp
+++ /dev/null
@@ -1,13 +0,0 @@
-// RUN: %check_analyzer_fixit %s %t \
-// RUN:   -analyzer-checker=core,optin.cplusplus.VirtualCall \
-// RUN:   -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true
-
-struct S {
-  virtual void foo();
-  S() {
-foo();
-// expected-warning@-1 {{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
-// CHECK-FIXES: S::foo();
-  }
-  ~S();
-};
Index: clang/test/Analysis/dead-stores.c
===
--- clang/test/Analysis/dead-stores.c
+++ clang/test/Analysis/dead-stores.c
@@ -1,16 +1,16 @@
-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -Wunused-variable -fblocks -Wno-unreachable-code \
 // RUN:   -analyzer-checker=core,deadcode.DeadStores \
 // RUN:   -analyzer-config deadcode.DeadStores:ShowFixIts=true \
-// RUN:   -analyzer-config fixits-as-remarks=true \
 // RUN:   -analyzer-config \
 // RUN:   deadcode.DeadStores:WarnForDeadNestedAssignments=false \
-// RUN:   -verify=non-nested %s
+// RUN:   -verify=non-nested
 
-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -Wunused-variable -fblocks -Wno-unreachable-code \
 // RUN:   -analyzer-checker=core,deadcode.DeadStores \
 // RUN:   -analyzer-config deadcode.DeadStores:ShowFixIts=true \
-// RUN:   -analyzer-config fixits-as-remarks=true \
-// RUN:   -verify=non-nested,nested %s
+// RUN:   -verify=non-nested,nested
 
 void f1() {
   int k, y; // non-nested-warning {{unused variable 'k'}}
@@ -18,14 +18,17 @@
   int abc = 1;
   long idx = abc + 3 * 5; // non-nested-warning {{never read}}
   // non-nested-warning@-1 {{unused variable 'idx'}}
-  // non-nested-remark@-2 {{11-24: ''}}
+  // CHECK-FIXES:  int abc = 1;
+  // CHECK-FIXES-NEXT: long idx;
 }
 
 void f2(void *b) {
   char *c = (char *)b; // no-warning
   char *d = b + 1; // non-nested-warning {{never read}}
// non-nested-warning@-1 {{unused variable 'd'}}
-   // non-nested-remark@-2 {{10-17: ''}}
+  // CHECK-FIXES:  char *c = (char *)b;
+  // CHECK-FIXES-NEXT: char *d;
+
   printf("%s", c);
   // non-nested-warning@-1 {{implicitly declaring library function 'printf' with type 'int (const char *, ...)'}}
   // non-nested-note@-2 {{include the header  or explicitly provide a 

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang-Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked an inline comment as done.
Charusso added a comment.

Thanks everyone! I hope the Analyzer developers start to use the wonderful 
features from Clang-Tidy.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang-Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf69c74db34f4: [analyzer] FixItHint: Apply and test hints 
with the Clang-Tidys script (authored by Charusso).

Changed prior to commit:
  https://reviews.llvm.org/D69746?vs=246209=248103#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/check-analyzer-fixit.py
  clang/test/Analysis/virtualcall-fixit.cpp
  clang/test/lit.cfg.py

Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -77,6 +77,11 @@
 if config.clang_staticanalyzer_z3 == '1':
 config.available_features.add('z3')
 
+check_analyzer_fixit_path = os.path.join(
+config.test_source_root, "Analysis", "check-analyzer-fixit.py")
+config.substitutions.append(
+('%check_analyzer_fixit',
+ '%s %s' % (config.python_executable, check_analyzer_fixit_path)))
 
 llvm_config.add_tool_substitutions(tools, tool_dirs)
 
Index: clang/test/Analysis/virtualcall-fixit.cpp
===
--- /dev/null
+++ clang/test/Analysis/virtualcall-fixit.cpp
@@ -0,0 +1,13 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:   -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true
+
+struct S {
+  virtual void foo();
+  S() {
+foo();
+// expected-warning@-1 {{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
+// CHECK-FIXES: S::foo();
+  }
+  ~S();
+};
Index: clang/test/Analysis/check-analyzer-fixit.py
===
--- /dev/null
+++ clang/test/Analysis/check-analyzer-fixit.py
@@ -0,0 +1,121 @@
+#!/usr/bin/env python
+#
+#===- check-analyzer-fixit.py - Static Analyzer test helper ---*- python -*-===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+#
+# This file copy-pasted mostly from the Clang-Tidy's 'check_clang_tidy.py'.
+#
+#======#
+
+r"""
+Clang Static Analyzer test helper
+=
+
+This script runs the Analyzer in fix-it mode and verify fixes, warnings, notes.
+
+Usage:
+  check-analyzer-fixit.py   [analyzer arguments]
+
+Example:
+  // RUN: %check-analyzer-fixit %s %t -analyzer-checker=core
+"""
+
+import argparse
+import os
+import re
+import subprocess
+import sys
+
+
+def write_file(file_name, text):
+with open(file_name, 'w') as f:
+f.write(text)
+
+
+def run_test_once(args, extra_args):
+input_file_name = args.input_file_name
+temp_file_name = args.temp_file_name
+clang_analyzer_extra_args = extra_args
+
+file_name_with_extension = input_file_name
+_, extension = os.path.splitext(file_name_with_extension)
+if extension not in ['.c', '.hpp', '.m', '.mm']:
+extension = '.cpp'
+temp_file_name = temp_file_name + extension
+
+with open(input_file_name, 'r') as input_file:
+input_text = input_file.read()
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
+write_file(temp_file_name, cleaned_test)
+
+original_file_name = temp_file_name + ".orig"
+write_file(original_file_name, cleaned_test)
+
+try:
+builtin_include_dir = subprocess.check_output(
+['clang', '-print-file-name=include'], stderr=subprocess.STDOUT)
+except subprocess.CalledProcessError as e:
+print('Cannot print Clang include directory: ' + e.output.decode())
+
+builtin_include_dir = os.path.normpath(builtin_include_dir)
+
+args = (['clang', '-cc1', '-internal-isystem', builtin_include_dir,
+ '-nostdsysteminc', '-analyze', '-analyzer-constraints=range',
+ '-analyzer-config', 'apply-fixits=true']
++ clang_analyzer_extra_args + ['-verify', temp_file_name])
+
+print('Running ' + str(args) + '...')
+
+try:
+clang_analyzer_output = \
+subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+except subprocess.CalledProcessError as e:
+

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 248097.
Charusso marked 4 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.

- Set the size properly.
- Add new debug.ExprInspection patterns: region, size, element count.
- `clang-format -i ExprInspectionChecker.cpp`.
- Having no idea what is the single regression in tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/expr-inspection.cpp
  clang/test/Analysis/memory-model.cpp
  clang/test/Analysis/misc-ps-region-store.m

Index: clang/test/Analysis/misc-ps-region-store.m
===
--- clang/test/Analysis/misc-ps-region-store.m
+++ clang/test/Analysis/misc-ps-region-store.m
@@ -1186,7 +1186,8 @@
 for (y = 0; y < b_h; y++) {
   for (x = 0; x < b_w + 1; x++) {
 int am = 0;
-tmp2[x] = am;
+tmp2[x] = am; // expected-warning \
+{{Access out-of-bound array element (buffer overflow)}}
   }
 }
   }
Index: clang/test/Analysis/memory-model.cpp
===
--- /dev/null
+++ clang/test/Analysis/memory-model.cpp
@@ -0,0 +1,108 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,cplusplus,debug.ExprInspection \
+// RUN:  -triple x86_64-unknown-linux-gnu \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void *alloca(size_t);
+void *realloc(void *ptr, size_t size);
+void *calloc(size_t number, size_t size);
+void free(void *);
+
+struct S { int f; };
+
+void clang_analyzer_region(int);
+void clang_analyzer_region(const void *);
+void clang_analyzer_size(int);
+void clang_analyzer_size(const void *);
+void clang_analyzer_elementCount(int);
+void clang_analyzer_elementCount(const void *);
+
+void var_region_simple_ref() {
+  int a = 13;
+  clang_analyzer_region(); // expected-warning {{a}}
+  clang_analyzer_size(); // expected-warning {{4 S64b}}
+  clang_analyzer_elementCount(); // expected-warning {{1 S64b}}
+}
+
+void var_region_simple_ptr(int *a) {
+  clang_analyzer_region(a); // expected-warning {{SymRegion{reg_$0}}}
+  clang_analyzer_size(a); // expected-warning {{extent_$1{SymRegion{reg_$0
+  clang_analyzer_elementCount(a); // expected-warning {{(extent_$1{SymRegion{reg_$0}}) / 8}}
+}
+
+void var_region_array() {
+  int a[] = {1, 2, 3};
+  clang_analyzer_region(a); // expected-warning {{Element{a,0 S64b,int}}}
+  clang_analyzer_size(a); // expected-warning {{12 S64b}}
+  clang_analyzer_elementCount(a); // expected-warning {{3 S64b}}
+}
+
+void string_region() {
+  clang_analyzer_region("foo"); // expected-warning {{Element{"foo",0 S64b,char}}}
+  clang_analyzer_size("foo"); // expected-warning {{4 S64b}}
+  clang_analyzer_elementCount("foo"); // expected-warning {{4 S64b}}
+}
+
+void field_region_ref(S a) {
+  clang_analyzer_region(); // expected-warning {{a.f}}
+  clang_analyzer_size(); // expected-warning {{4 S64b}}
+  clang_analyzer_elementCount(); // expected-warning {{1 S64b}}
+}
+
+void field_region_ptr(S *a) {
+  clang_analyzer_region(>f); // expected-warning {{SymRegion{reg_$0}.f}}
+  clang_analyzer_size(>f); // expected-warning {{4 S64b}}
+  clang_analyzer_elementCount(>f); // expected-warning {{1 S64b}}
+}
+
+void symbolic_array() {
+  int *a = new int[3];
+  clang_analyzer_region(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_size(a); // expected-warning {{12 S64b}}
+  clang_analyzer_elementCount(a); // expected-warning {{3 S64b}}
+  delete[] a;
+}
+
+void symbolic_placement_new() {
+  char *buf = new char[sizeof(int) * 3];
+  int *a = new (buf) int(13);
+  clang_analyzer_region(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_size(a); // expected-warning {{12 S64b}}
+  clang_analyzer_elementCount(a); // expected-warning {{3 S64b}}
+  delete[] buf;
+}
+
+void symbolic_malloc() {
+  int *a = (int *)malloc(12);
+  clang_analyzer_region(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_size(a); // expected-warning {{12 S64b}}
+  clang_analyzer_elementCount(a); // expected-warning {{3 S64b}}
+  free(a);
+}
+
+void symbolic_alloca() {
+  int *a = (int *)alloca(12);
+  clang_analyzer_region(a); // expected-warning {{Element{HeapSymRegion{conj}}
+  clang_analyzer_size(a); // 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+if (CI->getValue().isUnsigned())
+  Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+

That one is interesting. Some of the checkers / SValBuilder(?) generate 
unsigned integers which should not happen, I believe. May we need a FIXME and 
an assertion about signedness. What do you think?



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:698
+  DefinedOrUnknownSVal Size = UnknownVal();
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+Size = State->getSVal(SizeExpr, LCtx).castAs();

NoQ wrote:
> Same. I guess we should add a test for both cases, given that nothing failed 
> when we screwed up the extent.
Well, it was equally wrong everywhere, so that it works... I have noticed it 
like 5 months ago, but I was lazy to fix.



Comment at: clang/test/Analysis/explain-svals.cpp:52
   // Sic! What gets computed is the extent of the element-region.
-  clang_analyzer_explain(clang_analyzer_getExtent(x)); // 
expected-warning-re^signed 32-bit integer '4'$
+  clang_analyzer_explain(clang_analyzer_getExtent(x)); // 
expected-warning-re^\(argument 'ext'\) \* 4$
   delete[] x;

Yea, that is the fact: The size is the size of the parameter, which is unknown.



Comment at: clang/test/Analysis/memory-model.cpp:108
+  free(c);
+}

Here I wanted to put more, but I am not that cool with other MemRegions. Some 
wise words about the followups of this test file:
> Some day
- The 11 years old comment in ExprEngine.cpp: 
https://github.com/llvm/llvm-project/blame/master/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp#L237



Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+tmp2[x] = am; // expected-warning \
+{{Access out-of-bound array element (buffer overflow)}}
   }

That is the single regression which I do not get.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75529: [analyzer] Limit UCharMax to min of max uchar or max int

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Could you add a test please? We really need tests for every patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75529/new/

https://reviews.llvm.org/D75529



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75514: [Analyzer] Only add container note tags to the operations of te affected container

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

Cool, that one a lucky one! I think the SymbolRef based world also working, 
just at some point it could not scale because other systems are region based... 
For now, it is a much better solution, and this pattern to overload the 
callback with all the interestingness seems like the standard way of using 
NoteTags. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75514/new/

https://reviews.llvm.org/D75514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75514: [Analyzer] Only add container note tags to the operations of te affected container

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

I believe our path and context sensitive engine is more extensible and precise 
than checking the source file. Are you sure it scales? I would prefer to tie 
this information for MemRegions, rather than arbitrary places in the source 
code. My knowledge is very weak in this checker but I have changed from the 
Tidy world to the Analyzer to enjoy its benefits. Please enjoy these benefits 
in your work as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75514/new/

https://reviews.llvm.org/D75514



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:731
+  }
+  return C.getNoteTag([Text, Name](BugReport ) -> std::string {
+  SmallString<256> Msg;

baloghadamsoftware wrote:
> Szelethus wrote:
> > baloghadamsoftware wrote:
> > > NoQ wrote:
> > > > Szelethus wrote:
> > > > > NoQ wrote:
> > > > > > baloghadamsoftware wrote:
> > > > > > > NoQ wrote:
> > > > > > > > You'll need to check whether the container is actually of 
> > > > > > > > interest to the bug report. We don't want notes to be added 
> > > > > > > > about changes to irrelevant containers.
> > > > > > > > 
> > > > > > > > You can use a combination of "Report `BR` was emitted by one of 
> > > > > > > > the iterator checkers" and "The memory region of the container 
> > > > > > > > is marked as interesting" (while also actually marking it as 
> > > > > > > > interesting in the checker).
> > > > > > > > 
> > > > > > > > Ideally we should instead make a new generic storage inside the 
> > > > > > > > `BugReport` object, in order to pass down the interesting 
> > > > > > > > information from the call site of `emitReport` ("Hi, i'm an 
> > > > > > > > iterator checker who emitted this report and i'm interested in 
> > > > > > > > changes made to the size of this container").
> > > > > > > Are you sure in this? I already wondered how it works so I added 
> > > > > > > a test that checks one container and changes another one and 
> > > > > > > there were no note tags displayed for the one we did not check 
> > > > > > > but change. See the last test.
> > > > > > That's because you didn't do
> > > > > > ```lang=c++
> > > > > >   V2.cbegin();
> > > > > >   V2.cend();
> > > > > > ```
> > > > > > in the beginning.
> > > > > A similar conversation sparked up recently in between @boga95, 
> > > > > @steakhal and me regarding reporting taintedness. Bug reports are 
> > > > > fine up to the point where (in reverse) the first propagation 
> > > > > happens, but finding out which value tainted the one that caused the 
> > > > > report isn't handled at the moment. One idea was to mark the initial 
> > > > > (again, in reverse) value as interesting, create a `NoteTag` at the 
> > > > > point of propagation, where we should know which value was the cause 
> > > > > of the spread, mark that interesting as well, etc.
> > > > > 
> > > > > If `NoteTag`s only emit a message when the concerning value is 
> > > > > interesting, this should theoretically solve that problem. I guess 
> > > > > you could say that we're propagating interestingness in reverse.
> > > > > 
> > > > > I'm not immediately sure if this idea was ever mentioned or 
> > > > > implemented here.
> > > > Yes, that's the intended solution to such problems. 
> > > > `trackExpressionValue` works similarly, just with assignments instead 
> > > > of taint propagations. And in both cases note tags are a much more 
> > > > straightforward solution to the problem.
> > > Yes, you are right. My problem now is that how to mark interesting when 
> > > debugging? I I filter for interesting containers only, I lose my ability 
> > > to debug. Should I create a debug function just for marking a container 
> > > as interesting. Or is there such function already?
> > I'm not immediately sure how interetingness ties into debugging, what 
> > specific scenario are you thinking about?
> In the test of the modeling checker we use debug checkers. They should be 
> able to mark the container interesting to be able to test the not tags. I 
> managed to solve problem, even in a somewhat unorthodox way.
The core issue with NoteTag it does not know about interestingness and nor 
about MemRegion. I believe everything based on MemRegions already and when you 
emit the report you know exactly which MemRegion raised an error. So I think 
first we need to solve that the NoteTags only report on given MemRegions and 
those regions of course mega-interesting: we do not need to keep around the 
interestingness then.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73720/new/

https://reviews.llvm.org/D73720



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75432: [analyzer][NFC][MallocChecker] Convert many parameters into CallEvent

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

I have added green markers to all of your patches as well. I really appreciate 
the simplification of the `MallocChecker`. May you would commit it as soon as 
possible, given that you have nailed what Artem has suggested. Cool^2.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75432/new/

https://reviews.llvm.org/D75432



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75432: [analyzer][NFC][MallocChecker] Convert many parameters into CallEvent

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

> Also... as to why I added so much `LLVM_UNREACHABLE` annotations

I believe it is not necessary to add `LLVM_NODISCARD`, but as it perfectly 
works here, I like it.

I would mention the mailing list here as well: 
http://lists.llvm.org/pipermail/cfe-dev/2020-February/064754.html


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75432/new/

https://reviews.llvm.org/D75432



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75431: [analyzer][NFC] Merge checkNewAllocator's paramaters into CXXAllocatorCall

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

> look for a solution better then demonstrated in this patch.

I believe this solution exactly what Artem suggested, so there is nothing to 
feel bad about. Cool.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75431/new/

https://reviews.llvm.org/D75431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75430: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Cool! May it worth to mention the corresponding mail from the mailing list in 
the Summary: http://lists.llvm.org/pipermail/cfe-dev/2020-February/064754.html




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:941
+  CXXDeallocatorCall(const CXXDeleteExpr *E, ProgramStateRef St,
+ const LocationContext *LCtx)
+  : AnyFunctionCall(E, St, LCtx) {}

`St` -> `State`



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:958
+
+  unsigned getNumArgs() const override { return getDecl()->getNumParams(); }
+

`return getOriginExpr()->getNumArgs()`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75430/new/

https://reviews.llvm.org/D75430



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

I wish for a third map, something like `ReallocationMap`. Other than that it is 
a great direction, I love it. Thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:355
+  template 
+  void checkRealloc(CheckerContext , const CallExpr *CE,
+ProgramStateRef State) const;

balazske wrote:
> The `CHECK_FN` could be used even here?
> ```
> template 
> CHECK_FN(checkRealloc)
> ```
I think about the opposite, passing around arguments by templates is not cool. 
They meant to be arguments.

This type of callback simply could be a third `CallDescriptionMap` for future 
profeness.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68165/new/

https://reviews.llvm.org/D68165



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.

> ! In D75271#1896223 , @Szelethus 
> wrote:
>  Thinking back, I did have a number of failed attempts to make something a 
> bit less ugly, but the sharp divide between the 2 libraries makes is 
> really-really difficult, and I don't recall alternative solutions being that 
> much better. Either the checker interface gets worse, or the checker 
> registration interface gets so messy that it would severely hurt further 
> improvements in terms of checker dependency development.

Well, if you cannot solve this problem, most likely we neither, but keep in 
mind:

> If you can't solve a problem, then there is an easier problem you can't 
> solve: find it.

- George Polya

---

As far as you guys keep updating your API I will buy it, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75271/new/

https://reviews.llvm.org/D75271



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D75271#1895949 , 
@baloghadamsoftware wrote:

> It is impossible to use `CheckerManager` as parameter for 
> `shouldRegisterXXX()` because `CheckerRegistry` does not have it. If I add 
> `CheckerManager` to `CheckerRegistry` then the `printXXX()` functions will 
> not work because they do not have `CheckerManager` at all. This patch does 
> not help in printing error message, see D75171 
> . I need a way to solve 44998 
>  by rejecting the checker if the 
> option is disabled **and** printing an error message.


Aha, I see as of D75171 . Well, @Szelethus 
felt the same to pass around the `CheckerManager`. Let us see:

  std::unique_ptr ento::createCheckerManager(
  ASTContext , 
  AnalyzerOptions ,
  ArrayRef plugins,
  ArrayRef> checkerRegistrationFns,
  DiagnosticsEngine ) {   
auto checkerMgr = std::make_unique(context, opts);

CheckerRegistry allCheckers(plugins, diags, opts, context.getLangOpts(),
checkerRegistrationFns);  
  
allCheckers.initializeManager(*checkerMgr);
allCheckers.validateCheckerOptions();
checkerMgr->finishedCheckerRegistration();

return checkerMgr;
  }

- from `StaticAnalyzer/Frontend/CheckerRegistration.cpp`.

Are you sure we cannot pass around the manager like 
`allCheckers.initializeManager(*checkerMgr);`? I am also thinking of a 
glue-layer between the two managers, like `CheckerRegistryManager` or 
something, which holds all your needs, like `AnalyzerOptions` and `LangOptions` 
for now.

I think if we ask @Szelethus kindly to design his callback using 
`CheckerManager` instead, we could simplify this patch a lot. As he is the code 
owner, most likely he continues his journey to make sure people enable the 
checker dependencies, most likely using `shouldRegisterXXX()`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75271/new/

https://reviews.llvm.org/D75271



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

PS: The `CheckerManager` also could serve this behavior as `registerXXX()` 
already passing around that manager, but I believe the `AnalysisManager` 
supposed to manage the analysis.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75271/new/

https://reviews.llvm.org/D75271



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

I am so sorry to mention, but we need the `AnalysisManager` to pass around 
which manages the analysis, therefore it knows both the `LangOptions` and 
`AnalyzerOptions`. Also this entire callback should be removed ideally: it has 
to be a virtual function defaulting to `return true;` and if someone needs this 
feature could rewrite the behavior. I guess there was some debate whether it 
should be on by default or not, but for a checker writer and future changes 
this patch shows that how weak this API is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75271/new/

https://reviews.llvm.org/D75271



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang-Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:92
+
+  bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false,
+   ApplyFixIts = false;

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > I'll be perfectly happy with removing `FixitsAsRemarks` entirely. Your 
> > > new mechanism is superior.
> > Okai, challenge accepted. Thanks!
> So, can we remove `FixitsAsRemarks` now or is anything still blocking it?
https://reviews.llvm.org/D73729


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang-Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 246209.
Charusso marked 4 inline comments as done.
Charusso retitled this revision from "[analyzer] FixItHint: Apply and test 
hints with the Clang Tidy's script" to "[analyzer] FixItHint: Apply and test 
hints with the Clang-Tidy's script".
Charusso added a comment.

- Fix.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/check-analyzer-fixit.py
  clang/test/Analysis/virtualcall-fixit.cpp
  clang/test/lit.cfg.py

Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -77,6 +77,11 @@
 if config.clang_staticanalyzer_z3 == '1':
 config.available_features.add('z3')
 
+check_analyzer_fixit_path = os.path.join(
+config.test_source_root, "Analysis", "check-analyzer-fixit.py")
+config.substitutions.append(
+('%check_analyzer_fixit',
+ '%s %s' % (config.python_executable, check_analyzer_fixit_path)))
 
 llvm_config.add_tool_substitutions(tools, tool_dirs)
 
Index: clang/test/Analysis/virtualcall-fixit.cpp
===
--- /dev/null
+++ clang/test/Analysis/virtualcall-fixit.cpp
@@ -0,0 +1,13 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:   -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true
+
+struct S {
+  virtual void foo();
+  S() {
+foo();
+// expected-warning@-1 {{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
+// CHECK-FIXES: S::foo();
+  }
+  ~S();
+};
Index: clang/test/Analysis/check-analyzer-fixit.py
===
--- /dev/null
+++ clang/test/Analysis/check-analyzer-fixit.py
@@ -0,0 +1,121 @@
+#!/usr/bin/env python
+#
+#===- check-analyzer-fixit.py - Static Analyzer test helper ---*- python -*-===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+#
+# This file copy-pasted mostly from the Clang-Tidy's 'check_clang_tidy.py'.
+#
+#======#
+
+r"""
+Clang Static Analyzer test helper
+=
+
+This script runs the Analyzer in fix-it mode and verify fixes, warnings, notes.
+
+Usage:
+  check-analyzer-fixit.py   [analyzer arguments]
+
+Example:
+  // RUN: %check-analyzer-fixit %s %t -analyzer-checker=core
+"""
+
+import argparse
+import os
+import re
+import subprocess
+import sys
+
+
+def write_file(file_name, text):
+with open(file_name, 'w') as f:
+f.write(text)
+
+
+def run_test_once(args, extra_args):
+input_file_name = args.input_file_name
+temp_file_name = args.temp_file_name
+clang_analyzer_extra_args = extra_args
+
+file_name_with_extension = input_file_name
+_, extension = os.path.splitext(file_name_with_extension)
+if extension not in ['.c', '.hpp', '.m', '.mm']:
+extension = '.cpp'
+temp_file_name = temp_file_name + extension
+
+with open(input_file_name, 'r') as input_file:
+input_text = input_file.read()
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
+write_file(temp_file_name, cleaned_test)
+
+original_file_name = temp_file_name + ".orig"
+write_file(original_file_name, cleaned_test)
+
+try:
+builtin_include_dir = subprocess.check_output(
+['clang', '-print-file-name=include'], stderr=subprocess.STDOUT)
+except subprocess.CalledProcessError as e:
+print('Cannot print Clang include directory: ' + e.output.decode())
+
+builtin_include_dir = os.path.normpath(builtin_include_dir)
+
+args = (['clang', '-cc1', '-internal-isystem', builtin_include_dir,
+ '-nostdsysteminc', '-analyze', '-analyzer-constraints=range',
+ '-analyzer-config', 'apply-fixits=true']
++ clang_analyzer_extra_args + ['-verify', temp_file_name])
+
+print('Running ' + str(args) + '...')
+
+try:
+clang_analyzer_output = \
+subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+except subprocess.CalledProcessError as e:
+print('Clang 

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang-Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks for the reviews! Are we good to go?




Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:112
   void enableFixitsAsRemarks() { FixitsAsRemarks = true; }
+  void enableFixItApplication() { ApplyFixIts = true; }
 

alexfh wrote:
> nit: I'd suggest naming the method closer to the name of the corresponding 
> field, e.g. `enableApplyFixIts`. Why isn't this `setApplyFixIts(bool)` btw?
We do not support the disable way of options, so let us make it 
`enableApplyFixIts()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74467: [analyzer] Teach scan-build how to rebuild index.html without analyzing.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

That is very unfortunate, but may if you could introduce a bullet-proof `ls` we 
could see if the `scan-build` sub-directory removal is non-alphabetical. I 
think the latter smells more badly.




Comment at: clang/test/Analysis/scan-build/rebuild_index/rebuild_index.test:16
+
+RUN: ls %t.output_dir | FileCheck -check-prefix CHECK-FILES %s
+

If you think that the `ls` is the problem may we need `ls -R` to print out 
every folder. (https://explainshell.com/explain?cmd=ls+-R)

There are more exotic ways to sort the order, like `LANG=C ls`: 
https://stackoverflow.com/questions/878249/unixs-ls-sort-by-name/878269


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74467/new/

https://reviews.llvm.org/D74467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

I think on a long term our knowledge increases so we could generalize upon what 
we have learnt. For now as long as it is working and have tests, it is cool.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:872
+///
+/// Example: \c class T : public S { using S::S; }; T(1);
+class CXXInheritedConstructorCall : public AnyFunctionCall {

NoQ wrote:
> martong wrote:
> > Perhaps the example could provide the definition of  the class `S` too.
> Dunno, it's kinda obvious that it has some constructor from an integer, and 
> that's really the only thing that we need to know about it. Also what's the 
> proper way to make a line break in `\c`? Because i always forget how to build 
> with doxygen :)
The simplest way is the Markdown support with triple ``` above and below the 
code-snippet: http://www.doxygen.nl/manual/markdown.html
The other more commonly used way is `\code`: 
http://www.doxygen.nl/manual/commands.html#cmdcode



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:494
   if (State != Pred->getState()) {
+assert(CE && "Inherited constructors do not have construction contexts!");
 static SimpleProgramPointTag T("ExprEngine",

baloghadamsoftware wrote:
> martong wrote:
> > `CIE` ?
> No. `CE`. Since inherited constructors do not have construction contexts, 
> `State` is the same for `CIE` as the previous `State`. Thus if they are 
> different, we are facing a `CE`.
This assertion has been removed intentionally?



Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:545
 
+// Anonymous parameters of an inheriting constructor are live for the 
entire
+// duration of the constructor.

NoQ wrote:
> martong wrote:
> > `live` -> `alive` ?
> Ah yes, I'd love me some good ol' `RelaxedAliveVariablesAnalysis` (:
> 
> Dunno why the terminology is made that way (i.e., live as in "live music"), 
> but it seems to be that way :/
I like the wording `live`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74735/new/

https://reviews.llvm.org/D74735



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74467: [analyzer] Teach scan-build how to rebuild index.html without analyzing.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

Thanks! I would mention in the Summary the necessary flags to perform 
index-only output. (May write some release notes?)

That option sounds very strange, but I like it. For example to run only 
necessary custom tests one could write:

  -analyzer-checker=core,unix,custom.checkers \
  -analyzer-config silence-checkers="core;unix"




Comment at: clang/tools/scan-build/bin/scan-build:953
+  my $ExitStatus = shift;
+
+  if (defined $Options{OutputFormat}) {

May inject `Diag "Analysis run complete.\n";` here?



Comment at: clang/tools/scan-build/bin/scan-build:976
+
+  if ($Options{ExitStatusFoundBugs}) {
+exit 1 if ($NumBugs > 0);

If we stick to printing information to the user: `Diag "No bugs found.\n"`?.



Comment at: clang/tools/scan-build/bin/scan-build:1303
+   from existing report.html files. Useful for making a custom Static Analyzer
+   integration into a build system that isn't otherwise supported by 
scan-build.
+

What about `--index-only`?



Comment at: clang/tools/scan-build/bin/scan-build:1618
+DieDiag("Only one of '-o' or '--generate-index' can be specified.\n");
+  }
+

I may smash this duplicated error-handling above in one if-stmt, given that the 
options are correlate with each other.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74467/new/

https://reviews.llvm.org/D74467



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71433: [analyzer] CERT: POS34-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D71433#1808316 , @Charusso wrote:

> In D71433#1784238 , @NoQ wrote:
>
> > Currently the check may warn on non-bugs of the following kind:
> >
> >   void foo() {
> > char env[] = "NAME=value";
> > putenv(env);
> > doStuff();
> > putenv("NAME=anothervalue");
> >   }
> >
>
>
> That could be the next round as a follow-up patch as the next semester starts 
> in February [...]


Well, the next semester is about to start. Could you implement that request 
please?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71433/new/

https://reviews.llvm.org/D71433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73966: [analyzer][WIP] Add 10.0.0 release notes.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:408
+
+- New checker: ``alpha.plusplus.PlacementNew`` to detect whether the storage
+  provided for default placement new is sufficiently large.

balazske wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > I think this one's out of alpha already (i.e., `cplusplus.PlacementNew`). 
> > > I'd also bump it to the top because it applies to more users.
> > Yup, but AFAIK it moved out of alpha after rc1 was tagged.
> `alpha.cplusplus.PlacementNew` not `alpha.plusplus.PlacementNew`?
`cplusplus.PlacementNew`
(cf. https://github.com/llvm/llvm-project/commit/bc29069dc40)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73966/new/

https://reviews.llvm.org/D73966



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73993: [analyzer] Fix a couple of bugs in HTML report generation.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

Cool, thanks you!




Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:612
+  return !(Range.getBegin().isMacroID() || Range.getEnd().isMacroID());
+}
+

Side note: I like the other form of De Morgan's laws because here I have to 
apply it in my head every time I see such a construct. Also we are using this 
function in negation, so I would write:
```lang=c
static bool isMacro(const SourceRange ) {
  return Range.getBegin().isMacroID() || Range.getEnd().isMacroID();
}
```

The idiom is to write code for readability so that understandability over 
everything else.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73993/new/

https://reviews.llvm.org/D73993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73993: [analyzer] Fix a couple of bugs in HTML report generation.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp:925
 HighlightRange(R, LPosInfo.first, Range);
-  }
 }
 

Here the gray highlighting goes, so the `PopUpRanges` store whether we have 
already highlighted the range and we early-continue. The cool thing is, we do 
not check for intersections because the pop-up range has a smaller scope than 
the entire expression's gray range so the HTML handles the colors/mouse-hover 
for us. In case of macro pop-ups the coloring and-or pop-upping was already 
buggy, they are handled differently, where I have not found such a cool 
workaround.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73993/new/

https://reviews.llvm.org/D73993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73993: [analyzer] Fix a couple of bugs in HTML report generation.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks for the fix! The `PopUpRanges` is very important, please revert it back 
in its original shape. Sorry for the inconvenience.

I have ran a quick scan-build with this patch on LLVM because I wanted to give 
you a real world example (which you cannot visibly see at the test file). Here 
it is:
F11294494: Screenshot_20200204_225522.png 

The idea of ranges was that to add a new entry into a pop-up note as a table so 
you could inject any kind of pop-up information in order on the same range. 
Also if we highlighted the range as a pop-up do not highlight it in gray as 
well (which could break the HTML). Back in the days I did not realize the 
importance of the Doxygen and test files, I did not really know how to do so. 
Here I have adjusted the comments a little-bit:

  + /// Create the pop-up notes' table's start tag.
  static void
  HandlePopUpPieceStartTag(Rewriter ,
   const std::vector ) {
for (const auto  : PopUpRanges) {
  html::HighlightRange(R, Range.getBegin(), Range.getEnd(), "",
   "",
   /*IsTokenRange=*/true);
}
  }
  
  + /// Inject a new entry into the pop-up notes' table or add the table's end 
tag.
  static void HandlePopUpPieceEndTag(Rewriter ,
 const PathDiagnosticPopUpPiece ,
 std::vector ,
 unsigned int LastReportedPieceIndex,
 unsigned int PopUpPieceIndex) {
SmallString<256> Buf;
llvm::raw_svector_ostream Out(Buf);
  
SourceRange Range(Piece.getLocation().asRange());
  
// Write out the path indices with a right arrow and the message as a row.
Out << ""
<< LastReportedPieceIndex;
  
// Also annotate the state transition with extra indices.
Out << '.' << PopUpPieceIndex;
  
Out << "" << Piece.getString() << "";
  
  - // If no report made at this range mark the variable and add the end tags.
  + // If no report made at the current range \c Range we could inject the 
table's end tag as this is the last report on that range. Also this is the 
first encounter of the range, after that it is safe to insert new entries to 
the table.
if (std::find(PopUpRanges.begin(), PopUpRanges.end(), Range) ==
PopUpRanges.end()) {
  // Store that we create a report at this range.
  PopUpRanges.push_back(Range);
  
  Out << "";
  html::HighlightRange(R, Range.getBegin(), Range.getEnd(),
   "", Buf.c_str(),
   /*IsTokenRange=*/true);
} else {
  -// Otherwise inject just the new row at the end of the range.
  +// Otherwise we are working with multiple notes at the same range, so 
inject a new row to the table at the end of the range which is the beginning of 
the table. With that we fill the table "upwards" which is the same order as we 
emit reports.
  html::HighlightRange(R, Range.getBegin(), Range.getEnd(), "", Buf.c_str(),
   /*IsTokenRange=*/true);
}
  }



---

Please rename the `variable-popups.c` to `variable-popups-simple.c`, 
`variable-popups-2.c` to `variable-popups-macro.c`, and here 
`variable-popups-multiple.c` comes:

  // RUN: rm -fR %t
  // RUN: mkdir %t
  // RUN: %clang_analyze_cc1 -analyzer-checker=core \
  // RUN:-analyzer-output=html -o %t -verify %s
  // RUN: cat %t/report-*.html | FileCheck %s
  
  void bar(int);
  
  void foo() {
int a;
for (unsigned i = 0; i < 3; ++i)
  if (i)
bar(a); // expected-warning{{1st function call argument is an 
uninitialized value}}
  }
  
  // CHECK:  i
  // CHECK-SAME:   
  // CHECK-SAME: 
  // CHECK-SAME:   2.1
  // CHECK-SAME: 
  // CHECK-SAME: 'i' is 0
  // CHECK-SAME:   
  // CHECK-SAME:   
  // CHECK-SAME: 
  // CHECK-SAME:   4.1
  // CHECK-SAME: 
  // CHECK-SAME: 'i' is 1
  // CHECK-SAME:   
  // CHECK-SAME: 


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73993/new/

https://reviews.llvm.org/D73993



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71155: [analyzer] CERT: STR30-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks! I have felt that may I create a too complex symbolic value. Sorry for 
stealing your time.




Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2097-2098
 
+void CStringChecker::evalCharPtrCommon(CheckerContext ,
+   const CallExpr *CE) const {
+  CurrentFunctionDescription = "char pointer returning function";

NoQ wrote:
> Ok, so this whole thing is trying to evaluate `strchr`-like functions, right? 
> I've no idea what it does but the problem is much more trivial that what 
> you're trying to do here.
> 
> Branch 1:
> 1. Conjure the offset.
> 2. Add it to the original pointer (using `evalBinOp`).
> 3. Bind the result.
> Branch 2:
> 1. Bind null.
> 
> And you probably should drop branch 2 completely because usually there's no 
> indication that the character may in fact be missing in the string, and i 
> don't want more null dereference false alarms. So it's just branch 1.
> 
> So the whole function should be 3 lines of code (maybe with a couple line 
> breaks).
> 
> Well, maybe you should also handle the case where the original pointer is 
> null (not sure if it's an immediate UB or not).
> 
> This could be improved by actually taking into account the contents of the 
> string, but you don't seem to be trying to do this here.
> I've no idea what it does
I wanted to represent the root string's VarDecl in the symbolic value so I 
could refer to it. I think it became too complex and printing out the root 
string's variable name does not worth that complexity. But here it is from 
`str30-c-explain.cpp`:
```
  char *slash;
  slash = strrchr(pathname, '/');

  clang_analyzer_dump(slash);
  // CHECK: {pathname,conj_$1{long long, LC1, S{{[0-9]+}}, #1},char}

  clang_analyzer_explain(slash);
  // CHECK: pointer to element of type 'char' with index 'symbol of type 'long 
long' conjured at statement 'pathname'' of parameter 'pathname'
```

> This could be improved by actually taking into account the contents of the 
> string, but you don't seem to be trying to do this here.
At first the name of the string would be cool to print out and we need dataflow 
support to make it cool. Given that it is too complex I have reverted the 
modeling part. Also I wanted to create a simpler modeling with your modeling 
solution for my name-storing purposes, but I cannot.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2101-2103
+  SValBuilder  = C.getSValBuilder();
+  ProgramStateRef State, StateNull;
+  std::tie(StateNull, State) = C.getState()->assume(SVB.makeNull());

NoQ wrote:
> So, like, `StateNull` is the state in which it is assumed that `0` is 
> non-zero and `State` is the state in which it is assumed that `0` is zero?
> 
> I mean, apart from the naming flip-flop - i can tell you in advance that `0` 
> is zero, it's not a matter of assumptions.
I wanted to force out the state split of an unknown indexing: null and non-null 
but may it was too explicit and useless, yes.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2127-2128
+  if (const auto *SL = dyn_cast(SrcExpr->IgnoreImpCasts())) {
+const StringRegion *ResultMR =
+C.getStoreManager().getRegionManager().getStringRegion(SL);
+SVal ResultV = loc::MemRegionVal(ResultMR);

NoQ wrote:
> This is guaranteed to return the string region that you already have as the 
> value of that expression.
Whoops. The idea was that to handle string regions explicitly and may calculate 
the returning index as well.



Comment at: clang/test/Analysis/cert/str30-c-notes.cpp:29
+  if (slash) {
+// expected-note@-1 {{'slash' is non-null}}
+// expected-note@-2 {{Taking true branch}}

Charusso wrote:
> Needs to be an assumption.
Let us say it is non-null.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71155/new/

https://reviews.llvm.org/D71155



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71155: [analyzer] CERT: STR30-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 242159.
Charusso marked 8 inline comments as done.
Charusso added a comment.

- Remove the modeling from CStringChecker.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71155/new/

https://reviews.llvm.org/D71155

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/cert/str30-c-notes.cpp
  clang/test/Analysis/cert/str30-c.cpp

Index: clang/test/Analysis/cert/str30-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+typedef __typeof__((char *)0 - (char *)0) ptrdiff_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+char *strrchr(const char *str, int c);
+int puts(const char *str);
+
+namespace test_strrchr_bad {
+const char *get_dirname(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+  if (slash) {
+*slash = '\0'; /* Undefined behavior */
+// expected-warning@-1 {{'slash' is pointing to a constant string}}
+  }
+  return pathname;
+}
+
+int main(void) {
+  puts(get_dirname(__FILE__));
+  return 0;
+}
+} // namespace test_strrchr_bad
+
+namespace test_strrchr_good {
+char *get_dirname(const char *pathname, char *dirname, size_t size) {
+  const char *slash;
+  slash = strrchr(pathname, '/');
+  if (slash) {
+ptrdiff_t slash_idx = slash - pathname;
+if ((size_t)slash_idx < size) {
+  memcpy(dirname, pathname, slash_idx);
+  dirname[slash_idx] = '\0';
+  return dirname;
+}
+  }
+  return 0;
+}
+
+int main(void) {
+  char dirname[260];
+  if (get_dirname(__FILE__, dirname, sizeof(dirname))) {
+puts(dirname);
+  }
+  return 0;
+}
+} // namespace test_strrchr_good
Index: clang/test/Analysis/cert/str30-c-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str30-c-notes.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.30c \
+// RUN:  -analyzer-output=text -verify %s
+
+// See the examples on the page of STR30-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+char *strrchr(const char *str, int c);
+int puts(const char *str);
+
+namespace test_strrchr_bad {
+const char *get_dirname(const char *pathname) {
+  char *slash;
+  slash = strrchr(pathname, '/');
+  // expected-note@-1 {{'strrchr' returns a pointer to the constant 'pathname'}}
+
+  slash = strrchr(slash, '/');
+  // expected-note@-1 {{'strrchr' returns a pointer to the constant 'slash'}}
+
+  if (slash) {
+// expected-note@-1 {{'slash' is non-null}}
+// expected-note@-2 {{Taking true branch}}
+
+*slash = '\0'; /* Undefined behavior */
+// expected-note@-1 {{'slash' is pointing to a constant string}}
+// expected-warning@-2 {{'slash' is pointing to a constant string}}
+  }
+  return pathname;
+}
+
+int main(void) {
+  puts(get_dirname(__FILE__));
+  // expected-note@-1 {{Calling 'get_dirname'}}
+  return 0;
+}
+} // namespace test_strrchr_bad
Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
@@ -12,6 +12,9 @@
 //  https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152038
 //
 //  This checker is a base checker which consist of the following checkers:
+//  - '30c'
+//  https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals
+//
 //  - '31c'
 //  https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
 //
@@ -54,9 +57,6 @@
 
 class StrCheckerBase
 : public Checker> {
-  using StrCheck = std::function;
-
 public:
   // We report a note when any of the calls in 'CDM' is being used because
   // they can cause a not null-terminated string. In this case we store the
@@ -73,6 +73,19 @@
 
   void checkPostStmt(const DeclStmt *S, CheckerContext ) const;
 
+  // STR30-C.
+  void checkMemchr(const CallEvent , const CallContext ,
+   CheckerContext ) const;
+  void checkStrchr(const CallEvent , const CallContext ,
+   CheckerContext ) 

[PATCH] D71033: [analyzer] CERT: STR32-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 241761.
Charusso added a comment.

- Rebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71033/new/

https://reviews.llvm.org/D71033

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp
  clang/test/Analysis/cert/str31-c-notes-warn-on-call-on.cpp
  clang/test/Analysis/cert/str32-c-notes.cpp
  clang/test/Analysis/cert/str32-c.cpp
  clang/test/Analysis/malloc.c

Index: clang/test/Analysis/malloc.c
===
--- clang/test/Analysis/malloc.c
+++ clang/test/Analysis/malloc.c
@@ -9,21 +9,6 @@
 
 void clang_analyzer_eval(int);
 
-// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
-// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
-// the builtin type: "Using the typedef version can cause portability
-// problems", but we're ok here because we're not actually running anything.
-// Also of note is this cryptic warning: "The wchar_t type is not supported
-// when you compile C code".
-//
-// See the docs for more:
-// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
-#if !defined(_WCHAR_T_DEFINED)
-// "Microsoft implements wchar_t as a two-byte unsigned value"
-typedef unsigned short wchar_t;
-#define _WCHAR_T_DEFINED
-#endif // !defined(_WCHAR_T_DEFINED)
-
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void *alloca(size_t);
Index: clang/test/Analysis/cert/str32-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str32-c.cpp
@@ -0,0 +1,88 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.32c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR32-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string
+
+#include "../Inputs/system-header-simulator.h"
+
+void *realloc(void *memblock, size_t size);
+
+namespace test_strncpy_bad {
+enum { STR_SIZE = 32 };
+
+size_t func(const char *source) {
+  char c_str[STR_SIZE];
+  size_t ret = 0;
+
+  if (source) {
+c_str[sizeof(c_str) - 1] = '\0';
+strncpy(c_str, source, sizeof(c_str));
+ret = strlen(c_str);
+// expected-warning@-1 {{'c_str' is not null-terminated}}
+  }
+  return ret;
+}
+} // namespace test_strncpy_bad
+
+namespace test_strncpy_good {
+enum { STR_SIZE = 32 };
+
+size_t func(const char *src) {
+  char c_str[STR_SIZE];
+  size_t ret = 0;
+
+  if (src) {
+strncpy(c_str, src, sizeof(c_str) - 1);
+c_str[sizeof(c_str) - 1] = '\0';
+ret = strlen(c_str);
+  }
+  return ret;
+}
+} // namespace test_strncpy_good
+
+namespace test_realloc_bad {
+wchar_t *cur_msg = NULL;
+size_t cur_msg_size = 1024;
+size_t cur_msg_len = 0;
+
+void lessen_memory_usage(void) {
+  if (cur_msg == NULL)
+return;
+
+  size_t temp_size = cur_msg_size / 2 + 1;
+  wchar_t *temp = (wchar_t *)realloc(cur_msg, temp_size * sizeof(wchar_t));
+  /* temp and cur_msg may no longer be null-terminated */
+  if (temp == NULL)
+return;
+
+  cur_msg = temp;
+  cur_msg_size = temp_size;
+  cur_msg_len = wcslen(cur_msg);
+  // expected-warning@-1 {{'cur_msg' is not null-terminated}}
+}
+} // namespace test_realloc_bad
+
+namespace test_realloc_good {
+wchar_t *cur_msg = NULL;
+size_t cur_msg_size = 1024;
+size_t cur_msg_len = 0;
+
+void lessen_memory_usage(void) {
+  if (cur_msg == NULL)
+return;
+
+  size_t temp_size = cur_msg_size / 2 + 1;
+  wchar_t *temp = (wchar_t *)realloc(cur_msg, temp_size * sizeof(wchar_t));
+  /* temp and cur_msg may no longer be null-terminated */
+  if (temp == NULL)
+return;
+
+  cur_msg = temp;
+  /* Properly null-terminate cur_msg */
+  cur_msg[temp_size - 1] = L'\0';
+  cur_msg_size = temp_size;
+  cur_msg_len = wcslen(cur_msg);
+}
+} // namespace test_realloc_good
Index: clang/test/Analysis/cert/str32-c-notes.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str32-c-notes.cpp
@@ -0,0 +1,64 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.32c \
+// RUN:  -analyzer-output=text -verify %s
+
+// See the examples on the page of STR32-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR32-C.+Do+not+pass+a+non-null-terminated+character+sequence+to+a+library+function+that+expects+a+string
+
+#include "../Inputs/system-header-simulator.h"
+
+void *realloc(void *memblock, size_t size);
+
+namespace test_strncpy_bad {
+enum { STR_SIZE = 32 };
+
+size_t func(const char *source) {
+  char c_str[STR_SIZE];
+  // expected-note@-1 {{'c_str' is 

[PATCH] D70805: [analyzer] SValHasDescendant: Determine whether the SVal has an SVal descendant

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 241758.
Charusso marked an inline comment as done.
Charusso added a comment.

- Rebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70805/new/

https://reviews.llvm.org/D70805

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValHasDescendant.h
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/test/Analysis/cert/str31-c-warn-on-call-off.cpp
  clang/test/Analysis/cert/str31-c-warn-on-call-on.cpp
  clang/test/Analysis/sval-has-desc.c

Index: clang/test/Analysis/sval-has-desc.c
===
--- /dev/null
+++ clang/test/Analysis/sval-has-desc.c
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,debug.ExprInspection \
+// RUN:  -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+void clang_analyzer_hasDescendantTrue(size_t, const char *);
+void clang_analyzer_hasDescendantFalse(const char *, size_t);
+void clang_analyzer_hasDescendantEquals(const char *, const char *);
+
+void test_desc(const char *src) {
+  size_t size = strlen(src);
+  clang_analyzer_hasDescendantTrue(size, src); // expected-warning {{TRUE}}
+}
+
+void test_non_desc(const char *src) {
+  size_t size = strlen(src);
+  clang_analyzer_hasDescendantFalse(src, size); // expected-warning {{FALSE}}
+}
+
+void test_equality_is_desc(const char *src) {
+  clang_analyzer_hasDescendantEquals(src, src); // expected-warning {{TRUE}}
+}
Index: clang/test/Analysis/cert/str31-c-warn-on-call-on.cpp
===
--- clang/test/Analysis/cert/str31-c-warn-on-call-on.cpp
+++ clang/test/Analysis/cert/str31-c-warn-on-call-on.cpp
@@ -39,7 +39,6 @@
   dest = (char *)malloc(size * 2 + 2);
 
   strcpy(dest, [13]);
-  // expected-warning@-1 {{'strcpy' could write outside of 'dest'}}
   free(dest);
 }
 
@@ -52,7 +51,6 @@
   dest = (char *)malloc(size * 2);
 
   strcpy(dest, [13]);
-  // expected-warning@-1 {{'strcpy' could write outside of 'dest'}}
   free(dest);
 }
 
Index: clang/test/Analysis/cert/str31-c-warn-on-call-off.cpp
===
--- clang/test/Analysis/cert/str31-c-warn-on-call-off.cpp
+++ clang/test/Analysis/cert/str31-c-warn-on-call-off.cpp
@@ -92,7 +92,6 @@
   free(dest); // no-warning
 }
 
-// FIXME: Suppress that with 'SValVisitor' or something similar.
 void test_complex_size_false_positive(const char *src) {
   char *dest;
   size_t size;
@@ -102,8 +101,6 @@
 
   strcpy(dest, [13]);
   do_something(dest);
-  // expected-warning@-1 {{'dest' is not null-terminated}}
-
   free(dest);
 }
 
@@ -117,8 +114,6 @@
 
   strcpy(dest, [13]);
   do_something(dest);
-  // expected-warning@-1 {{'dest' is not null-terminated}}
-
   free(dest);
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
@@ -27,6 +27,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SValHasDescendant.h"
 #include "llvm/ADT/Optional.h"
 #include 
 
@@ -244,16 +245,11 @@
   DefinedOrUnknownSVal DestSize = getDynamicSize(State, DestMR, SVB);
 
   // 'strlen(src) + integer' is most likely fine.
-  // FIXME: Use the 'SValVisitor' to catch every such constructs of the symbol.
   // FIXME: We cannot catch every '+ integer' part at the moment so we do not
   // check that property for now.
-  if (const SymExpr *SE = DestSize.getAsSymExpr())
-for (const auto  : SE->symbols())
-  if (const auto *SIE = dyn_cast(Sym))
-if (SIE->getOpcode() == BO_Add)
-  if (const auto *SM = dyn_cast(SIE->getLHS()))
-if (SM->getRegion() == SrcMR)
-  return;
+  SValHasDescendant HasDescendantSrc(SrcMR);
+  if (HasDescendantSrc.Visit(DestSize))
+return;
 
   // 'StringRegion' returns the size with the null-terminator.
   if (const llvm::APSInt *SrcSizeInt = SVB.getKnownValue(State, SrcSize))
Index: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
@@ -14,6 +14,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SValHasDescendant.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ScopedPrinter.h"
 
@@ -41,6 +42,7 @@
   void 

[PATCH] D70805: [analyzer] SValHasDescendant: Determine whether the SVal has an SVal descendant

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValHasDescendant.h:55
+  bool VisitSymbolRegionValue(const SymbolRegionValue *S) {
+return Visit(S->getRegion());
+  }

NoQ wrote:
> Arithmetic is indeed easy, but for example this part requires a much deeper 
> justification.
Well, this is an experiment. I have checked out the Clang Tidy's 
matcher-writing language's framework so I believe their own language is way 
more better, than implementing `hasDescendant()` only. Some kind of framework 
would be neat, but this is what I came up with as the `hasDescendant()` is the 
most powerful matcher in their world.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70805/new/

https://reviews.llvm.org/D70805



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70411: [analyzer] CERT: STR31-C

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 241752.
Charusso edited the summary of this revision.
Charusso added a comment.

- 2020-ify the checker writing
- Remove extra bullet-points of CERT checker documentation: we only need the 
checker's documentation, not the packages.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
  clang/lib/StaticAnalyzer/Checkers/AllocationState.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp
  clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp
  clang/test/Analysis/Inputs/system-header-simulator.h
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp
  clang/test/Analysis/cert/str31-c-notes-warn-on-call-on.cpp
  clang/test/Analysis/cert/str31-c-warn-on-call-off.cpp
  clang/test/Analysis/cert/str31-c-warn-on-call-on.cpp
  clang/test/Analysis/cert/str31-c.cpp

Index: clang/test/Analysis/cert/str31-c.cpp
===
--- /dev/null
+++ clang/test/Analysis/cert/str31-c.cpp
@@ -0,0 +1,196 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,unix,alpha.security.cert.str.31c \
+// RUN:  -verify %s
+
+// See the examples on the page of STR31-C:
+// https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator
+
+#include "../Inputs/system-header-simulator.h"
+
+#define EOF -1
+typedef __SIZE_TYPE__ size_t;
+
+void free(void *memblock);
+void *malloc(size_t size);
+
+void do_something(char *buffer);
+
+namespace test_gets_bad {
+#define BUFFER_SIZE 1024
+
+void func(void) {
+  char buf[BUFFER_SIZE];
+  if (gets(buf)) {
+// expected-warning@-1 {{'gets' could write outside of 'buf'}}
+  }
+}
+} // namespace test_gets_bad
+
+namespace test_gets_good {
+enum { BUFFERSIZE = 32 };
+
+void func(void) {
+  char buff[BUFFERSIZE];
+
+  if (fgets(buff, sizeof(buff), stdin)) {
+  }
+}
+} // namespace test_gets_good
+
+namespace test_sprintf_bad {
+void func(const char *name) {
+  char buf[128];
+  sprintf(buf, "%s.txt", name);
+  // expected-warning@-1 {{'sprintf' could write outside of 'buf'}}
+}
+} // namespace test_sprintf_bad
+
+namespace test_sprintf_good {
+void func(const char *name) {
+  char buff[128];
+  snprintf(buff, sizeof(buff), "%s.txt", name);
+
+  do_something(buff);
+}
+} // namespace test_sprintf_good
+
+namespace test_fscanf_bad {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buf[BUF_LENGTH];
+  if (fscanf(stdin, "%s", buf)) {
+// expected-warning@-1 {{'fscanf' could write outside of 'buf'}}
+  }
+}
+} // namespace test_fscanf_bad
+
+namespace test_fscanf_good {
+enum { BUF_LENGTH = 1024 };
+
+void get_data(void) {
+  char buff[BUF_LENGTH];
+  if (fscanf(stdin, "%1023s", buff)) {
+do_something(buff);
+  }
+}
+} // namespace test_fscanf_good
+
+namespace test_strcpy_bad {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char prog_name[128];
+  strcpy(prog_name, name);
+  // expected-warning@-1 {{'strcpy' could write outside of 'prog_name'}}
+
+  return 0;
+}
+
+void func(void) {
+  char buff[256];
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+strcpy(buff, editor);
+// expected-warning@-1 {{'strcpy' could write outside of 'buff'}}
+  }
+}
+} // namespace test_strcpy_bad
+
+namespace test_strcpy_good {
+int main(int argc, char *argv[]) {
+  const char *const name = (argc && argv[0]) ? argv[0] : "";
+  char *prog_name2 = (char *)malloc(strlen(name) + 1);
+  if (prog_name2 != NULL) {
+strcpy(prog_name2, name);
+  }
+
+  do_something(prog_name2);
+
+  free(prog_name2);
+  return 0;
+}
+
+void func(void) {
+  char *buff2;
+  char *editor = getenv("EDITOR");
+  if (editor != NULL) {
+size_t len = strlen(editor) + 1;
+buff2 = (char *)malloc(len);
+if (buff2 != NULL) {
+  strcpy(buff2, editor);
+}
+
+do_something(buff2);
+free(buff2);
+  }
+}
+} // namespace test_strcpy_good
+
+//===--===//
+// The following are from the rule's page which we do not handle yet.
+//===--===//
+
+namespace test_loop_index_bad {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] && (i < n); ++i) {
+dest[i] = src[i];
+  }
+  dest[i] = '\0';
+}
+} // namespace test_loop_index_bad
+
+namespace test_loop_index_good {
+void copy(size_t n, char src[n], char dest[n]) {
+  size_t i;
+
+  for (i = 0; src[i] 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 241749.
Charusso edited the summary of this revision.
Charusso added a comment.

- Let us reuse this patch.
- Remove the expression storing feature.
- Only store known sizes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -10,15 +10,16 @@
 //
 //===--===//
 
-#include "clang/AST/Decl.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "PrettyStackTraceLocationContext.h"
 #include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/Analysis/Analyses/LiveVariables.h"
 #include "clang/Analysis/ConstructionContext.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -690,16 +691,18 @@
 
 // See if we need to conjure a heap pointer instead of
 // a regular unknown pointer.
-bool IsHeapPointer = false;
-if (const auto *CNE = dyn_cast(E))
-  if (CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
-// FIXME: Delegate this to evalCall in MallocChecker?
-IsHeapPointer = true;
-  }
-
-R = IsHeapPointer ? svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count)
-  : svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy,
- Count);
+const auto *CNE = dyn_cast(E);
+if (CNE && CNE->getOperatorNew()->isReplaceableGlobalAllocationFunction()) {
+  // FIXME: Delegate this to evalCall in MallocChecker?
+  DefinedOrUnknownSVal Size = UnknownVal();
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+Size = State->getSVal(SizeExpr, LCtx).castAs();
+
+  R = svalBuilder.getConjuredHeapSymbolVal(E, LCtx, Count);
+  State = setDynamicSize(State, R.getAsRegion(), Size);
+} else {
+  R = svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
+}
   }
   return State->BindExpr(E, LCtx, R);
 }
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -10,15 +10,16 @@
 //
 //===--===//
 
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
-#include "clang/Analysis/ConstructionContext.h"
 #include "clang/AST/DeclCXX.h"
-#include "clang/AST/StmtCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/AST/StmtCXX.h"
+#include "clang/Analysis/ConstructionContext.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 
 using namespace clang;
 using namespace ento;
@@ -761,11 +762,17 @@
   // heap. We realize this is an approximation that might not correctly model
   // a custom global allocator.
   if (symVal.isUnknown()) {
-if (IsStandardGlobalOpNewFunction)
+if (IsStandardGlobalOpNewFunction) {
+  DefinedOrUnknownSVal Size = UnknownVal();
+  if (const Expr *SizeExpr = CNE->getArraySize().getValueOr(nullptr))
+Size = State->getSVal(SizeExpr, LCtx).castAs();
+
   symVal = svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount);
-else
+  State = setDynamicSize(State, symVal.getAsRegion(), Size);
+} else {
   symVal = svalBuilder.conjureSymbolVal(nullptr, CNE, LCtx, CNE->getType(),
 blockCount);
+}
   }
 
   CallEventManager  = getStateManager().getCallEventManager();
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- 

[PATCH] D73729: [analyzer] AnalyzerOptions: Remove 'fixits-as-remarks'

[Csaba Dabis via Phabricator via cfe-commits]

Charusso created this revision.
Charusso added a reviewer: NoQ.
Charusso added a project: clang.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Charusso added a parent revision: D69746: [analyzer] FixItHint: Apply and test 
hints with the Clang Tidy's script.

The new way of checking fix-its is `%check_analyzer_fixit`.


Repository:
  rC Clang

https://reviews.llvm.org/D73729

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/virtualcall-fixit.cpp
  clang/test/Analysis/virtualcall-fixits.cpp

Index: clang/test/Analysis/virtualcall-fixits.cpp
===
--- clang/test/Analysis/virtualcall-fixits.cpp
+++ clang/test/Analysis/virtualcall-fixits.cpp
@@ -1,10 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
 // RUN: -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \
 // RUN: %s 2>&1 | FileCheck -check-prefix=TEXT %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \
+
+// RUN: %check_analyzer_fixit %s %t \
+// RUN: -analyzer-checker=core,optin.cplusplus.VirtualCall \
 // RUN: -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \
-// RUN: -analyzer-config fixits-as-remarks=true \
-// RUN: -analyzer-output=plist -o %t.plist -verify %s
+// RUN: -analyzer-output=plist -o %t.plist
 // RUN: cat %t.plist | FileCheck -check-prefix=PLIST %s
 
 struct S {
@@ -12,7 +13,9 @@
   S() {
 foo();
 // expected-warning@-1{{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
-// expected-remark@-2{{5-5: 'S::'}}
+// CHECK-FIXES:  S() {
+// CHECK-FIXES-NEXT:   S::foo();
+// CHECK-FIXES-NEXT: }
   }
   ~S();
 };
@@ -30,12 +33,12 @@
 // PLIST-NEXT:remove_range
 // PLIST-NEXT:
 // PLIST-NEXT: 
-// PLIST-NEXT:  line13
+// PLIST-NEXT:  line14
 // PLIST-NEXT:  col5
 // PLIST-NEXT:  file0
 // PLIST-NEXT: 
 // PLIST-NEXT: 
-// PLIST-NEXT:  line13
+// PLIST-NEXT:  line14
 // PLIST-NEXT:  col4
 // PLIST-NEXT:  file0
 // PLIST-NEXT: 
Index: clang/test/Analysis/virtualcall-fixit.cpp
===
--- clang/test/Analysis/virtualcall-fixit.cpp
+++ /dev/null
@@ -1,13 +0,0 @@
-// RUN: %check_analyzer_fixit %s %t \
-// RUN:   -analyzer-checker=core,optin.cplusplus.VirtualCall \
-// RUN:   -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true
-
-struct S {
-  virtual void foo();
-  S() {
-foo();
-// expected-warning@-1 {{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
-// CHECK-FIXES: S::foo();
-  }
-  ~S();
-};
Index: clang/test/Analysis/dead-stores.c
===
--- clang/test/Analysis/dead-stores.c
+++ clang/test/Analysis/dead-stores.c
@@ -1,16 +1,16 @@
-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -Wunused-variable -fblocks -Wno-unreachable-code \
 // RUN:   -analyzer-checker=core,deadcode.DeadStores \
 // RUN:   -analyzer-config deadcode.DeadStores:ShowFixIts=true \
-// RUN:   -analyzer-config fixits-as-remarks=true \
 // RUN:   -analyzer-config \
 // RUN:   deadcode.DeadStores:WarnForDeadNestedAssignments=false \
-// RUN:   -verify=non-nested %s
+// RUN:   -verify=non-nested
 
-// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -Wunused-variable -fblocks -Wno-unreachable-code \
 // RUN:   -analyzer-checker=core,deadcode.DeadStores \
 // RUN:   -analyzer-config deadcode.DeadStores:ShowFixIts=true \
-// RUN:   -analyzer-config fixits-as-remarks=true \
-// RUN:   -verify=non-nested,nested %s
+// RUN:   -verify=non-nested,nested
 
 void f1() {
   int k, y; // non-nested-warning {{unused variable 'k'}}
@@ -18,14 +18,17 @@
   int abc = 1;
   long idx = abc + 3 * 5; // non-nested-warning {{never read}}
   // non-nested-warning@-1 {{unused variable 'idx'}}
-  // non-nested-remark@-2 {{11-24: ''}}
+  // CHECK-FIXES:  int abc = 1;
+  // CHECK-FIXES-NEXT: long idx;
 }
 
 void f2(void *b) {
   char *c = (char *)b; // no-warning
   char *d = b + 1; // non-nested-warning {{never read}}
// non-nested-warning@-1 {{unused variable 'd'}}
-   // non-nested-remark@-2 {{10-17: ''}}
+  // CHECK-FIXES:  char *c = (char *)b;
+  // CHECK-FIXES-NEXT: char *d;
+
   printf("%s", c);
   // non-nested-warning@-1 {{implicitly declaring library function 'printf' with type 'int (const char *, ...)'}}
   // 

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 241492.
Charusso marked 8 inline comments as done.
Charusso added a comment.

- Change to 4-column space standard.
- Simplify obtaining the Clang include directory.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/check_analyzer_fixit.py
  clang/test/Analysis/virtualcall-fixit.cpp
  clang/test/lit.cfg.py

Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -77,6 +77,11 @@
 if config.clang_staticanalyzer_z3 == '1':
 config.available_features.add('z3')
 
+check_analyzer_fixit_path = os.path.join(
+config.test_source_root, "Analysis", "check_analyzer_fixit.py")
+config.substitutions.append(
+('%check_analyzer_fixit',
+ '%s %s' % (config.python_executable, check_analyzer_fixit_path)))
 
 llvm_config.add_tool_substitutions(tools, tool_dirs)
 
Index: clang/test/Analysis/virtualcall-fixit.cpp
===
--- /dev/null
+++ clang/test/Analysis/virtualcall-fixit.cpp
@@ -0,0 +1,13 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:   -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true
+
+struct S {
+  virtual void foo();
+  S() {
+foo();
+// expected-warning@-1 {{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
+// CHECK-FIXES: S::foo();
+  }
+  ~S();
+};
Index: clang/test/Analysis/check_analyzer_fixit.py
===
--- /dev/null
+++ clang/test/Analysis/check_analyzer_fixit.py
@@ -0,0 +1,121 @@
+#!/usr/bin/env python
+#
+#===- check_analyzer_fixit.py - Static Analyzer test helper ---*- python -*-===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+#
+# This file copy-pasted mostly from the Clang Tidy's 'check_clang_tidy.py'.
+#
+#======#
+
+r"""
+Clang Static Analyzer test helper
+=
+
+This script runs the Analyzer in fix-it mode and verify fixes, warnings, notes.
+
+Usage:
+  check_analyzer_fixit.py   [analyzer arguments]
+
+Example:
+  // RUN: %check_analyzer_fixit %s %t -analyzer-checker=core
+"""
+
+import argparse
+import os
+import re
+import subprocess
+import sys
+
+
+def write_file(file_name, text):
+with open(file_name, 'w') as f:
+f.write(text)
+
+
+def run_test_once(args, extra_args):
+input_file_name = args.input_file_name
+temp_file_name = args.temp_file_name
+clang_analyzer_extra_args = extra_args
+
+file_name_with_extension = input_file_name
+_, extension = os.path.splitext(file_name_with_extension)
+if extension not in ['.c', '.hpp', '.m', '.mm']:
+extension = '.cpp'
+temp_file_name = temp_file_name + extension
+
+with open(input_file_name, 'r') as input_file:
+input_text = input_file.read()
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
+write_file(temp_file_name, cleaned_test)
+
+original_file_name = temp_file_name + ".orig"
+write_file(original_file_name, cleaned_test)
+
+try:
+builtin_include_dir = subprocess.check_output(
+['clang', '-print-file-name=include'], stderr=subprocess.STDOUT)
+except subprocess.CalledProcessError as e:
+print('Cannot print Clang include directory: ' + e.output.decode())
+
+builtin_include_dir = os.path.normpath(builtin_include_dir)
+
+args = (['clang', '-cc1', '-internal-isystem', builtin_include_dir,
+ '-nostdsysteminc', '-analyze', '-analyzer-constraints=range',
+ '-analyzer-config', 'apply-fixits=true']
++ clang_analyzer_extra_args + ['-verify', temp_file_name])
+
+print('Running ' + str(args) + '...')
+
+try:
+clang_analyzer_output = \
+subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+except subprocess.CalledProcessError as e:
+print('Clang Static Analyzer test failed:\n' + e.output.decode())
+raise
+
+print('- Clang Static 

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 241493.
Charusso edited the summary of this revision.
Charusso added a comment.

- Rename the script from `check_analyzer_fixit.py` to `check-analyzer-fixit.py`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/lib/StaticAnalyzer/Frontend/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/check-analyzer-fixit.py
  clang/test/Analysis/virtualcall-fixit.cpp
  clang/test/lit.cfg.py

Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -77,6 +77,11 @@
 if config.clang_staticanalyzer_z3 == '1':
 config.available_features.add('z3')
 
+check_analyzer_fixit_path = os.path.join(
+config.test_source_root, "Analysis", "check-analyzer-fixit.py")
+config.substitutions.append(
+('%check_analyzer_fixit',
+ '%s %s' % (config.python_executable, check_analyzer_fixit_path)))
 
 llvm_config.add_tool_substitutions(tools, tool_dirs)
 
Index: clang/test/Analysis/virtualcall-fixit.cpp
===
--- /dev/null
+++ clang/test/Analysis/virtualcall-fixit.cpp
@@ -0,0 +1,13 @@
+// RUN: %check_analyzer_fixit %s %t \
+// RUN:   -analyzer-checker=core,optin.cplusplus.VirtualCall \
+// RUN:   -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true
+
+struct S {
+  virtual void foo();
+  S() {
+foo();
+// expected-warning@-1 {{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}}
+// CHECK-FIXES: S::foo();
+  }
+  ~S();
+};
Index: clang/test/Analysis/check-analyzer-fixit.py
===
--- /dev/null
+++ clang/test/Analysis/check-analyzer-fixit.py
@@ -0,0 +1,121 @@
+#!/usr/bin/env python
+#
+#===- check-analyzer-fixit.py - Static Analyzer test helper ---*- python -*-===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+#
+# This file copy-pasted mostly from the Clang Tidy's 'check_clang_tidy.py'.
+#
+#======#
+
+r"""
+Clang Static Analyzer test helper
+=
+
+This script runs the Analyzer in fix-it mode and verify fixes, warnings, notes.
+
+Usage:
+  check-analyzer-fixit.py   [analyzer arguments]
+
+Example:
+  // RUN: %check-analyzer-fixit %s %t -analyzer-checker=core
+"""
+
+import argparse
+import os
+import re
+import subprocess
+import sys
+
+
+def write_file(file_name, text):
+with open(file_name, 'w') as f:
+f.write(text)
+
+
+def run_test_once(args, extra_args):
+input_file_name = args.input_file_name
+temp_file_name = args.temp_file_name
+clang_analyzer_extra_args = extra_args
+
+file_name_with_extension = input_file_name
+_, extension = os.path.splitext(file_name_with_extension)
+if extension not in ['.c', '.hpp', '.m', '.mm']:
+extension = '.cpp'
+temp_file_name = temp_file_name + extension
+
+with open(input_file_name, 'r') as input_file:
+input_text = input_file.read()
+
+# Remove the contents of the CHECK lines to avoid CHECKs matching on
+# themselves.  We need to keep the comments to preserve line numbers while
+# avoiding empty lines which could potentially trigger formatting-related
+# checks.
+cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
+write_file(temp_file_name, cleaned_test)
+
+original_file_name = temp_file_name + ".orig"
+write_file(original_file_name, cleaned_test)
+
+try:
+builtin_include_dir = subprocess.check_output(
+['clang', '-print-file-name=include'], stderr=subprocess.STDOUT)
+except subprocess.CalledProcessError as e:
+print('Cannot print Clang include directory: ' + e.output.decode())
+
+builtin_include_dir = os.path.normpath(builtin_include_dir)
+
+args = (['clang', '-cc1', '-internal-isystem', builtin_include_dir,
+ '-nostdsysteminc', '-analyze', '-analyzer-constraints=range',
+ '-analyzer-config', 'apply-fixits=true']
++ clang_analyzer_extra_args + ['-verify', temp_file_name])
+
+print('Running ' + str(args) + '...')
+
+try:
+clang_analyzer_output = \
+subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+except subprocess.CalledProcessError as e:
+print('Clang Static Analyzer test failed:\n' + e.output.decode())
+raise
+
+print('- Clang Static 

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks for the reviews! Sorry for the delay.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69746: [analyzer] FixItHint: Apply and test hints with the Clang Tidy's script

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:92
+
+  bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false,
+   ApplyFixIts = false;

NoQ wrote:
> I'll be perfectly happy with removing `FixitsAsRemarks` entirely. Your new 
> mechanism is superior.
Okai, challenge accepted. Thanks!



Comment at: clang/test/Analysis/check_analyzer_fixit.py:1
+#!/usr/bin/env python
+#

lebedev.ri wrote:
> This does work with python3?
I think it should. It is only made for running by the `lit` which is left in 
Python 2.



Comment at: clang/test/Analysis/check_analyzer_fixit.py:50-51
+  clang_dir = clang_dir.strip()
+  if sys.platform in ['win32']:
+clang_dir = clang_dir.replace('\\', '/')
+

NoQ wrote:
> I think there must be an `os.path` function for this.
I hope it is `os.path.normpath()`.



Comment at: clang/test/Analysis/check_analyzer_fixit.py:59
+f.write(text)
+f.truncate()
+

NoQ wrote:
> Mmm, what does this do?
I think an empty `truncate()` does not do anything, so removed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69746/new/

https://reviews.llvm.org/D69746



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69731: [analyzer] CheckerContext: Make the Preprocessor available

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG38ab3b876baa: [analyzer] CheckerContext: Make the 
Preprocessor available (authored by Charusso).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69731/new/

https://reviews.llvm.org/D69731

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp
  clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  clang/unittests/StaticAnalyzer/Reusables.h

Index: clang/unittests/StaticAnalyzer/Reusables.h
===
--- clang/unittests/StaticAnalyzer/Reusables.h
+++ clang/unittests/StaticAnalyzer/Reusables.h
@@ -58,7 +58,7 @@
   ExprEngineConsumer(CompilerInstance )
   : C(C), ChkMgr(C.getASTContext(), *C.getAnalyzerOpts()), CTU(C),
 Consumers(),
-AMgr(C.getASTContext(), Consumers,
+AMgr(C.getASTContext(), C.getPreprocessor(), Consumers,
  CreateRegionStoreManager, CreateRangeConstraintManager, ,
  *C.getAnalyzerOpts()),
 VisitedCallees(), FS(),
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -193,7 +193,7 @@
 
 public:
   ASTContext *Ctx;
-  const Preprocessor 
+  Preprocessor 
   const std::string OutDir;
   AnalyzerOptionsRef Opts;
   ArrayRef Plugins;
@@ -336,8 +336,8 @@
 checkerMgr = createCheckerManager(
 *Ctx, *Opts, Plugins, CheckerRegistrationFns, PP.getDiagnostics());
 
-Mgr = std::make_unique(*Ctx, PathConsumers, CreateStoreMgr,
-CreateConstraintMgr,
+Mgr = std::make_unique(*Ctx, PP, PathConsumers,
+CreateStoreMgr, CreateConstraintMgr,
 checkerMgr.get(), *Opts, Injector);
   }
 
Index: clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp
+++ clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp
@@ -13,7 +13,7 @@
 
 void AnalysisManager::anchor() { }
 
-AnalysisManager::AnalysisManager(ASTContext ,
+AnalysisManager::AnalysisManager(ASTContext , Preprocessor ,
  const PathDiagnosticConsumers ,
  StoreManagerCreator storemgr,
  ConstraintManagerCreator constraintmgr,
@@ -38,7 +38,7 @@
   Options.ShouldElideConstructors,
   /*addVirtualBaseBranches=*/true,
   injector),
-  Ctx(ASTCtx), LangOpts(ASTCtx.getLangOpts()),
+  Ctx(ASTCtx), PP(PP), LangOpts(ASTCtx.getLangOpts()),
   PathConsumers(PDC), CreateStoreMgr(storemgr),
   CreateConstraintMgr(constraintmgr), CheckerMgr(checkerMgr),
   options(Options) {
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
@@ -107,6 +107,8 @@
 return getBugReporter().getSourceManager();
   }
 
+  Preprocessor () { return getBugReporter().getPreprocessor(); }
+
   SValBuilder () {
 return Eng.getSValBuilder();
   }
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
@@ -16,6 +16,7 @@
 
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/PathDiagnostic.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
@@ -32,6 +33,7 @@
   AnalysisDeclContextManager AnaCtxMgr;
 
   ASTContext 
+  Preprocessor 
   const LangOptions 
   PathDiagnosticConsumers PathConsumers;
 
@@ -44,7 +46,7 @@
 public:
   AnalyzerOptions 
 
-  AnalysisManager(ASTContext ,
+  AnalysisManager(ASTContext , Preprocessor ,
   const PathDiagnosticConsumers ,
   StoreManagerCreator storemgr,
   ConstraintManagerCreator constraintmgr,
@@ -61,6 +63,8 @@
 return AnaCtxMgr;
   }
 
+  Preprocessor () override { return PP; }
+
   StoreManagerCreator getStoreManagerCreator() {
 return CreateStoreMgr;
   }
Index: 

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Csaba Dabis via Phabricator via cfe-commits]

Charusso abandoned this revision.
Charusso marked an inline comment as done.
Charusso added a comment.

Let us say, we do not need this patch. In case we would need it, I will reopen.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69726/new/

https://reviews.llvm.org/D69726



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69599: [analyzer] DynamicSize: Remove 'getSizeInElements()' from store

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGaf3d0d16286a: [analyzer] DynamicSize: Remove 
getSizeInElements() from store (authored by Charusso).

Changed prior to commit:
  https://reviews.llvm.org/D69599?vs=227546=241460#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69599/new/

https://reviews.llvm.org/D69599

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -623,15 +623,6 @@
   SymbolReaper& SymReaper) override;
 
   //===--===//
-  // Region "extents".
-  //===--===//
-
-  // FIXME: This method will soon be eliminated; see the note in Store.h.
-  DefinedOrUnknownSVal getSizeInElements(ProgramStateRef state,
- const MemRegion* R,
- QualType EleTy) override;
-
-  //===--===//
   // Utility methods.
   //===--===//
 
@@ -1388,37 +1379,6 @@
 }
 
 //===--===//
-// Extents for regions.
-//===--===//
-
-DefinedOrUnknownSVal
-RegionStoreManager::getSizeInElements(ProgramStateRef state,
-  const MemRegion *R,
-  QualType EleTy) {
-  DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
-  const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
-  if (!SizeInt)
-return UnknownVal();
-
-  CharUnits RegionSize = CharUnits::fromQuantity(SizeInt->getSExtValue());
-
-  if (Ctx.getAsVariableArrayType(EleTy)) {
-// FIXME: We need to track extra state to properly record the size
-// of VLAs.  Returning UnknownVal here, however, is a stop-gap so that
-// we don't have a divide-by-zero below.
-return UnknownVal();
-  }
-
-  CharUnits EleSize = Ctx.getTypeSizeInChars(EleTy);
-
-  // If a variable is reinterpreted as a type that doesn't fit into a larger
-  // type evenly, round it down.
-  // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize,
-svalBuilder.getArrayIndexType());
-}
-
-//===--===//
 // Location and region casting.
 //===--===//
 
Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
===
--- clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
+++ clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
+#include "clang/AST/Expr.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
@@ -26,5 +27,22 @@
   return MR->getMemRegionManager().getStaticSize(MR, SVB);
 }
 
+DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State,
+const MemRegion *MR,
+SValBuilder ,
+QualType ElementTy) {
+  MemRegionManager  = MR->getMemRegionManager();
+  ASTContext  = MemMgr.getContext();
+
+  DefinedOrUnknownSVal Size = getDynamicSize(State, MR, SVB);
+  SVal ElementSizeV = SVB.makeIntVal(
+  Ctx.getTypeSizeInChars(ElementTy).getQuantity(), SVB.getArrayIndexType());
+
+  SVal DivisionV =
+  SVB.evalBinOp(State, BO_Div, Size, ElementSizeV, SVB.getArrayIndexType());
+
+  return DivisionV.castAs();
+}
+
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ 

[PATCH] D69540: [analyzer] DynamicSize: Remove 'getExtent()' from regions

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG601687bf731a: [analyzer] DynamicSize: Remove 
getExtent() from regions (authored by Charusso).

Changed prior to commit:
  https://reviews.llvm.org/D69540?vs=227013=241447#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69540/new/

https://reviews.llvm.org/D69540

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -329,7 +329,7 @@
 }
 
 QualType SymbolExtent::getType() const {
-  ASTContext  = R->getMemRegionManager()->getContext();
+  ASTContext  = R->getMemRegionManager().getContext();
   return Ctx.getSizeType();
 }
 
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
@@ -876,7 +877,7 @@
 
   // Find the length (in bits) of the region being invalidated.
   uint64_t Length = UINT64_MAX;
-  SVal Extent = Top->getExtent(SVB);
+  SVal Extent = Top->getMemRegionManager().getStaticSize(Top, SVB);
   if (Optional ExtentCI =
   Extent.getAs()) {
 const llvm::APSInt  = ExtentCI->getValue();
@@ -1394,7 +1395,7 @@
 RegionStoreManager::getSizeInElements(ProgramStateRef state,
   const MemRegion *R,
   QualType EleTy) {
-  SVal Size = cast(R)->getExtent(svalBuilder);
+  DefinedOrUnknownSVal Size = getDynamicSize(state, R, svalBuilder);
   const llvm::APSInt *SizeInt = svalBuilder.getKnownValue(state, Size);
   if (!SizeInt)
 return UnknownVal();
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -142,7 +142,7 @@
   return false;
 }
 
-MemRegionManager* SubRegion::getMemRegionManager() const {
+MemRegionManager ::getMemRegionManager() const {
   const SubRegion* r = this;
   do {
 const MemRegion *superRegion = r->getSuperRegion();
@@ -159,56 +159,6 @@
   return SSR ? SSR->getStackFrame() : nullptr;
 }
 
-//===--===//
-// Region extents.
-//===--===//
-
-DefinedOrUnknownSVal TypedValueRegion::getExtent(SValBuilder ) const {
-  ASTContext  = svalBuilder.getContext();
-  QualType T = getDesugaredValueType(Ctx);
-
-  if (isa(T))
-return nonloc::SymbolVal(svalBuilder.getSymbolManager().getExtentSymbol(this));
-  if (T->isIncompleteType())
-return UnknownVal();
-
-  CharUnits size = Ctx.getTypeSizeInChars(T);
-  QualType sizeTy = svalBuilder.getArrayIndexType();
-  return svalBuilder.makeIntVal(size.getQuantity(), sizeTy);
-}
-
-DefinedOrUnknownSVal FieldRegion::getExtent(SValBuilder ) const {
-  // Force callers to deal with bitfields explicitly.
-  if (getDecl()->isBitField())
-return UnknownVal();
-
-  DefinedOrUnknownSVal Extent = DeclRegion::getExtent(svalBuilder);
-
-  // A zero-length array at the end of a struct often stands for dynamically-
-  // allocated extra memory.
-  if (Extent.isZeroConstant()) {
-QualType T = getDesugaredValueType(svalBuilder.getContext());
-
-if (isa(T))
-  return UnknownVal();
-  }
-
-  return 

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a reviewer: NoQ.
Charusso added a comment.

Hey, thanks! The patch looks great, but please note that we do the reviews with 
context using `git diff -U99` or uploading with `arc` 
(https://secure.phabricator.com/book/phabricator/article/arcanist/).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73629/new/

https://reviews.llvm.org/D73629



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73521: [analyzer][WIP] add-new-checker.py: Introduction

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked an inline comment as done.
Charusso added a comment.

This script dumps out a dummy checker which is the continuation of the Clang 
Tidy's 
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/add_new_check.py

However, we could stick to the main-function-redeclaration-checker in a 2020 
fashion. As I see peoples starting their careers with Tidy, that is why this 
checker born, but please feel free to swap or modify. I think that is the 
easiest way to document changes in requirements of checker-writing. I also 
wanted to document common mistakes, like how to use a `Twine`, what is a 
`dyn_cast_or_null<>` vs. `cast<>`, make sure the `SVal` type is written out and 
it is not `const` or `const*`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73521/new/

https://reviews.llvm.org/D73521



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73521: [analyzer][WIP] add-new-checker.py: Introduction

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 240759.
Charusso added a comment.

- Make it runable. Whoops.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73521/new/

https://reviews.llvm.org/D73521

Files:
  clang/test/Analysis/add-new-checker/add-main-package.rst
  clang/test/Analysis/add-new-checker/add-main-package.td
  clang/test/Analysis/add-new-checker/add-multiple-packages.rst
  clang/test/Analysis/add-new-checker/add-multiple-packages.td
  clang/test/Analysis/add-new-checker/check-add-new-checker.py
  clang/test/Analysis/add-new-checker/checker-name.rst
  clang/test/Analysis/add-new-checker/checker-name.td
  clang/test/Analysis/add-new-checker/flow-package-exist-checker.rst
  clang/test/Analysis/add-new-checker/flow-package-exist-checker.td
  clang/test/Analysis/add-new-checker/flow-package-not-exist-layering.rst
  clang/test/Analysis/add-new-checker/flow-package-not-exist-layering.td
  clang/test/Analysis/add-new-checker/flow-package-not-exist.rst
  clang/test/Analysis/add-new-checker/flow-package-not-exist.td
  clang/test/Analysis/add-new-checker/lit.local.cfg.py
  clang/utils/analyzer/add-new-checker.py

Index: clang/utils/analyzer/add-new-checker.py
===
--- /dev/null
+++ clang/utils/analyzer/add-new-checker.py
@@ -0,0 +1,732 @@
+#!/usr/bin/env python
+#
+#===- add-new-checker.py - Creates a Static Analyzer checker --*- python -*-===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+#
+# Example usage: python add-new-checker.py alpha.package.Foo
+#
+#======#
+
+import argparse
+import json
+import os
+import re
+import subprocess
+
+
+class Checker:
+def __init__(self, packages, name):
+self.packages = packages
+self.name = name
+
+def name(self):
+return self.name
+
+def packages(self):
+return self.packages
+
+def __str__(self):
+return '.'.join(self.packages) + '.' + self.name
+
+
+# Obtain the longest known package-chain of the 'packages' chain. For example
+# the checker creation of 'alpha.core.a.Checker' would return ['alpha', 'core'].
+def get_best_package_match(packages, known_packages):
+best_package_match = []
+for i in range(len(packages)):
+temp_packages = packages[:i + 1]
+package_name = '.'.join(temp_packages)
+if package_name in known_packages:
+best_package_match = temp_packages
+return best_package_match
+
+
+# Put every additional package and the checker into an increasing size jagged
+# array. For example the checker creation of 'core.a.Checker' would return
+# '[['core', 'a'], ['core', 'a', 'Checker']]'.
+def get_addition(packages, name, best_package_match):
+addition = []
+match_count = len(best_package_match)
+for i, package in enumerate(packages):
+if i < match_count:
+continue
+addition.append(best_package_match + packages[match_count : i + 1])
+addition.append(packages + [name])
+return addition
+
+
+def add_checkers_entry(clang_path, checker, args):
+checkers_include_path = os.path.join(clang_path, 
+'include', 'clang', 'StaticAnalyzer', 'Checkers')
+checkers_path = os.path.join(checkers_include_path, 'Checkers.td')
+if args.is_test:
+checkers_path = args.test_path
+if not checkers_path.endswith('.td.tmp'):
+return
+
+packages = checker.packages
+name = checker.name
+
+# Load the checkers' TableGen.
+# FIXME: Add path to 'llvm-tblgen'.
+data = None
+try:
+data = subprocess.check_output(['llvm-tblgen', '-dump-json',
+checkers_path,
+'-I=' + checkers_include_path])
+except subprocess.CalledProcessError as e:
+print('TableGen JSON dump failed:\n' + e.output.decode())
+
+data = json.loads(data)
+
+# Store the package names with their chained definition. For example the
+# package 'CoreAlpha' => 'alpha.core'.
+package_name_def_map = {}
+for package_def in data['!instanceof']['Package']:
+package = data[package_def]
+if package['ParentPackage']:
+chain = [package]
+while True:
+parent_package = chain[-1]['ParentPackage']
+if parent_package is None:
+break
+chain.append(data[parent_package['def']])
+
+dump = []
+for elem in reversed(chain):
+package_name = elem['PackageName']
+dump.append(package_name)
+package_name_def_map['.'.join(dump)] = package['!name']
+else:
+