[PATCH] D156787: [analyzer][attr] Add docs to ownership_* attributes

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1189
+``onwership_returns``: Functions with this annotation return dynamic memory.
+The second annotation parameter is the size of the returned memory in bytes.
+

aaron.ballman wrote:
> I'm a bit confused on this last bit; how is the user supposed to statically 
> know that value? I read this as claiming `my_malloc` returns 1 byte of 
> dynamic storage; can the user tie the allocation to a parameter value instead?
I share your confusion, I guess. What I've written here is my best 
understanding of how this works, unfortunately, these annotations were made in 
~2010 (when I was still in elementary school) and abandoned since.

> I read this as claiming my_malloc returns 1 byte of dynamic storage;

Thats what the corresponding code in the Static Analyzer leads me to believe as 
well.

> can the user tie the allocation to a parameter value instead?

No. 


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

https://reviews.llvm.org/D156787

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

[PATCH] D156787: [analyzer][attr] Add docs to ownership_* attributes

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 548116.
Szelethus marked 4 inline comments as done.
Szelethus added a comment.

Fixes according to reviewer comments.


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

https://reviews.llvm.org/D156787

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/malloc-annotations.c

Index: clang/test/Analysis/malloc-annotations.c
===
--- clang/test/Analysis/malloc-annotations.c
+++ clang/test/Analysis/malloc-annotations.c
@@ -148,6 +148,14 @@
   return p; // no-warning
 }
 
+// Test ownership_holds -- we expect the ownership to be taken over, and a
+// result assume that the memory will not leak, but also not released yet.
+void af6(void) {
+  int *p = my_malloc(12);
+  my_hold(p);
+  *p = 4; // no-warn: memory is not released yet
+} // no-warn: assume my_hold takes care of releasing the memory
+
 
 
 // This case tests that storing malloc'ed memory to a static variable which is
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1776,6 +1776,8 @@
   if (!State)
 return nullptr;
 
+  // TODO: Check the attribute docs in AttrDocs.td, it states that only malloc
+  // is accepted. Please adjust if we start supporting "new".
   if (Att->getModule()->getName() != "malloc")
 return nullptr;
 
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -1164,6 +1164,61 @@
   }];
 }
 
+def OwnershipDocs : Documentation {
+  let Category = DocCatFunction;
+  let Heading = "ownership_holds, ownership_returns, ownership_takes";
+  let Content = [{
+These attributes help the static analyzer understand custom ownership management
+functions. None of these affect the generated binary in any way, they are only
+meant to assist the Clang Static Analyzer. You can use these annotations if your
+code uses wrapper functions around ``malloc`` or ``free``, or uses functions
+that take over ownership altogether.
+
+Each annotation has two parameters: the first is the type of allocator used to
+allocate the memory (the only accepted value currently is ``malloc``, but
+we intend to recognize ``new`` in the future). We use this to check whether the
+correct deallocator is used. The second is an integer value, described below.
+
+.. code-block:: c
+
+  void __attribute((ownership_takes(malloc, 1))) my_free(void *);
+  void *__attribute((ownership_returns(malloc, 1))) my_malloc(size_t);
+  void __attribute((ownership_holds(malloc, 1))) my_hold(void *);
+
+``onwership_returns``: Functions with this annotation return dynamic memory.
+The second annotation parameter is the size of the returned memory in bytes.
+
+``ownership_takes``: Functions with this annotation deallocate one of the
+function parameters. The second annotation parameter is the index of the
+function parameter to deallocate.
+
+``ownership_holds``: Functions with this annotation take over the ownership of
+one of the parameters. The static analyzer doesn't assume it to be deallocated
+immediately, but assumes that it will be before the end of the program. The
+second annotation parameter is the index of the function parameter to take hold
+of. Indexing starts at 1, so the first parameter's index is 1. The implicit this
+parameter is not supported at this time.
+
+.. code-block:: c
+
+  void foobar() {
+int *p = my_malloc(sizeof(int));
+  } // warn: Memory leak
+
+  void foo() {
+int *p = my_malloc(sizeof(int));
+my_free(p);
+free(p); // warn: Attempt to free released memory
+  }
+
+  void bar() {
+int *p = my_malloc(sizeof(int));
+my_hold(p);
+*p = 4; // no warn: Pointer is not released
+  } // no warn: Ownership is transferred and assumed to have not escaped
+  }];
+}
+
 def ObjCMethodFamilyDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2355,7 +2355,7 @@
   let Args = [IdentifierArgument<"Module">,
   VariadicParamIdxArgument<"Args">];
   let Subjects = SubjectList<[HasFunctionProto]>;
-  let Documentation = [Undocumented];
+  let Documentation = [OwnershipDocs];
 }
 
 def Packed : InheritableAttr {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156787: [analyzer][attr] Add docs to ownership_* attributes

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, steakhal, donat.nagy, balazske, gamesh411.
Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun, yaxunl.
Herald added a reviewer: aaron.ballman.
Herald added a project: All.
Szelethus requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Pretty much what it says on the tin! I worked a bunch on this checker so I had 
some vague memory (hehe) what these annotations mean, and also found this 
thread in the mailing list:
https://discourse.llvm.org/t/ownership-attribute-for-malloc-etc-checking/16260


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156787

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/malloc-annotations.c

Index: clang/test/Analysis/malloc-annotations.c
===
--- clang/test/Analysis/malloc-annotations.c
+++ clang/test/Analysis/malloc-annotations.c
@@ -148,6 +148,12 @@
   return p; // no-warning
 }
 
+void af6(void) {
+  int *p = my_malloc(12);
+  my_hold(p);
+  *p = 4;
+}
+
 
 
 // This case tests that storing malloc'ed memory to a static variable which is
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1776,6 +1776,8 @@
   if (!State)
 return nullptr;
 
+  // TODO: Check the attribute docs in AttrDocs.td, it states that only malloc
+  // is accepted. Please adjust if we start supporting "new".
   if (Att->getModule()->getName() != "malloc")
 return nullptr;
 
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -1164,6 +1164,60 @@
   }];
 }
 
+def OwnershipDocs : Documentation {
+  let Category = DocCatFunction;
+  let Heading = "ownership_holds, ownership_returns, ownership_takes";
+  let Content = [{
+These attributes help the static analyzer understand custom ownership management
+functions. Neither of these affect the generated binary in any way, its only
+meant to assist the Clang Static Analyzer. You can use these annotations if your
+code uses wrapper functions around ``malloc`` or ``free``, or uses functions
+that take over ownership altogether.
+
+All 3 annotations have two parameters: the first is type of allocator used to
+allocate the memory (as of writing, the only accepted value is ``malloc``, but
+we intend to recognize ``new`` as well). We use this to check whether the
+correct deallocator is used. The second is an integer value.
+
+.. code-block:: c
+
+  void __attribute((ownership_takes(malloc, 1))) my_free(void *);
+  void *__attribute((ownership_returns(malloc, 1))) my_malloc(size_t);
+  void __attribute((ownership_holds(malloc, 1))) my_hold(void *);
+
+``onwership_returns``: Functions with this annotation return dynamic memory.
+The second annotation parameter is the size of the returned memory in bytes.
+
+``ownership_takes``: Functions with this annotation deallocate one of the
+function parameters. The second annotation parameter is the index of the
+function parameter to deallocate.
+
+``ownership_holds``: Functions with this annotation take over the ownership of
+one of the parameters. The static analyzer doesn't assume it to be deallocated
+immediately, but assumes that it will be before the end of the program. The
+second annotation parameter is the index of the function parameter to take hold
+of.
+
+.. code-block:: c
+
+  void foobar() {
+int *p = my_malloc(sizeof(int));
+  } // warn: Memory leak
+
+  void foo() {
+int *p = my_malloc(sizeof(int));
+my_free(p);
+free(p); // warn: Attempt to free released memory
+  }
+
+  void bar() {
+int *p = my_malloc(sizeof(int));
+my_hold(p);
+*p = 4; // no warn: Pointer is not released
+  } // no warn: Ownership is transferred and assumed to have not escaped
+  }];
+}
+
 def ObjCMethodFamilyDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -2355,7 +2355,7 @@
   let Args = [IdentifierArgument<"Module">,
   VariadicParamIdxArgument<"Args">];
   let Subjects = SubjectList<[HasFunctionProto]>;
-  let Documentation = [Undocumented];
+  let Documentation = [OwnershipDocs];
 }
 
 def Packed : InheritableAttr {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe

[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer

[Kristóf Umann via Phabricator via cfe-commits]

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

But this renders much of my C++98 knowledge obsolete!11


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154325

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

This is the quintessential example of a patch where while the test files look 
promising, we need some evaluation on the real world. I understand that its a 
chore -- but this is simply the nature of the beast.

While the changes in the test file look promising. I'd be more pleased if some 
real world data would back it up (I understand that we shared a link around 
internally, but that kind of defeats the purpose of an open source review).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D152436#4443858 , @balazske wrote:

> In D152436#4438956 , @NoQ wrote:
>
>> I'm somewhat skeptical of the decision made in D151225 
>>  because the entire reason I originally 
>> implemented `StdCLibraryFunctions` was to deal with false positives I was 
>> seeing. It was really valuable even without the bug-finding part. So I 
>> really wish we could find some way to keep bug-finding and modeling separate.
>
> The problem was that modeling and report generation could not be separated 
> correctly. Both are implemented in one class but are differently named 
> checkers that should run in a specific order because dependency issues, this 
> was not good.

In my view, it would certainly be possible through enormous efforts to further 
granularize this checker (or these large ones in general), so that the modeling 
and reporting portions would could be cleanly separated into their own checker 
objects. That certianly was my belief a couple years back -- I sank months and 
months into `MallocChecker`, yet I'm still not even close to that goal.

So, with the modeling and the reporting being the same entity, we can't express 
that some more specific checkers should run before it. `StreamChecker` can 
construct more specific messages thatn `StdLibraryFunctions` for a null stream 
object, but only if it runs ahead of it. That implies a both a weak and a 
strong dependency on what is essentially the same checker. As things stand, not 
sure how we could have avoided this if we want these checkers to finally leave 
the alpha state.

In D152436#4443828 , @balazske wrote:

> For first experiment I have made patch D153612 
>  that adds a `NoteTag` to "all" standard 
> function calls.

Could you post the results for it as you have them please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152436

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

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D152436#4432736 , @balazske wrote:

> From these two solutions, which one is better? (Show many unnecessary notes, 
> or show only necessary ones but lose some of the useful notes too.)

How likely are the problems with the 2nd case? I know its a hassle, but can you 
upload results for the first and the second case? Its hard to tell without 
seeing how these pan out in the real world.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152436

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

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D152436#4411912 , @steakhal wrote:

> In D152436#4408811 , @balazske 
> wrote:
>
>> In D152436#4408301 , @steakhal 
>> wrote:
>>
>>> I looked at the TPs, and if the violation was introduced by an assumption 
>>> (instead of an assignment), then it's really hard to spot which assumption 
>>> is important for the bug.
>>> I wonder if we could add the `TrackConstraintBRVisitor` to the bugreport to 
>>> "highlight" that particular assumption/place.
>>
>> The question is first if this problem must be fixed before the checker comes 
>> out of alpha state. If yes I try to make another patch with this fix. I 
>> tried this previously but do not remember exactly what the problem was.
>
> WIthout an explicit note message there, I don't see how could we advertise 
> this as a "mature" checker.

Is it possible to hide functions hindered by this problem behing an 
off-by-default flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152436

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

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

> I am not sure about the exact requirements, this review can be a place for 
> discussion about what should be fixed (if any).

D52984  added the "Making your checker better" 
section to the dev manual: 
https://clang-analyzer.llvm.org/checker_dev_manual.html (nobody can be faulted 
for not finding this, aside from those that witnessed its creation, it has 
faded from the collective memory of the analyzer developers).

In D152436#4405558 , @balazske wrote:

> I could test the checker on these projects (CTU analysis was not used):
> memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres,tinyxml2,libwebm,xerces,bitcoin,protobuf,qtbase,contour,acid
>
> These are reports that could be improved:
> link 
> 
> In this case function `fileno` returns -1 because of failure, but this is not 
> indicated in a `NoteTag`. This is a correct result, only the note is missing. 
> This problem can be solved if a note is displayed on every branch ("case") of 
> the standard C functions. But this leads to many notes at un-interesting 
> places. If the note is displayed only at "interesting" values another 
> difficulty shows up: The note disappears from places where it should be shown 
> because the "interestingness" is not set, for example at conditions of `if` 
> statement. So the solution may require more work. This case with function 
> `fileno` occurs 13 times in all the tested projects.

Yeah, this is a tough cookie... is it okay to find hide the -1 branch behind an 
off-by-default checker option for the time being?

> link 
> 
> The function `open` is not modeled in `StdCLibraryFunctionsChecker`, it 
> should not return less than -1 but this information is not included now.
> link 
> 
> `socket` can not return less than -1 but this function is not modeled 
> currently.

These should be a rather painless fix, right?

> link 
> 
> This looks wrong, `L` should not be 0 because `len` looks > 0 (see the macros 
> that set `len`). Probably the many bitwise operations cause the problem.
> link 
> 
> When `file_size` is 0 `status.ok()` is probably false that is not correctly 
> recognized (may work in CTU mode?).

Looks like something we can live with.

> link 
> 
> `fwrite` with 0 buffer and 0 size should not be an error, this is not checked 
> now.

Some discussion for that: D140387#inline-1360054 
. There is a FIXME in the code 
for it -- not sure how common this specific issue is, but we did stumble on it 
in an open source project... how painful would it be to fix this?

> These results look good:
> link 
> 
> link 
> 
> link 
> 

[PATCH] D151225: [clang][analyzer] Merge apiModeling.StdCLibraryFunctions and StdCLibraryFunctionArgs checkers into one.

[Kristóf Umann via Phabricator via cfe-commits]

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

Ugh, yeah, so it has come to this. I championed my idea of granulalizing 
checkers into modeling and reporting components since what... 2018? I think the 
goal is still something to shoot for, but its time to admit that we the 
underlaying infrastructure needs to encourage that a lot more (and there is 
already so much dept in the form of these giant checkers).

I like the idea that the modeling portion and the reporting portion are in 
different checkers, but that is hard to achieve, when fatal errors by the sheer 
fact that they create sinkholes makes this practically impossible (we'd have to 
rewrite almost EVERY checker with that in mind, and also in way that it wont 
just require a branch). Even then, I'm not sure we would want that -- our 
checkers are not perfect, and yes, we sometimes do want to explicitly disable a 
non-alpha checker and everything that they do, including modeing because they 
are not working well on a project.

For how much I took a dump on checker silencing back in the day, I'm coming to 
terms with the fact that it is likely to be the most sustainable approach 
moving forward. Or, I don't know, the sheer size of these checkers really 
scream for some splitting up...

In any case, LGTM. I'm sure there are other ways around this, like creating an 
inner checker callback system inside this checker, but that sounds ridiculus, 
and its not like we can't do it after the fact.

Please grep the codebase for `apiModeling.StdCLibraryFunctionsChecker` to find 
other references to the now removed checker. Do the docs need any adjustment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151225

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

[PATCH] D149447: [clang][analyzer] Improve documentation of StdCLibraryFunctionArgs checker (NFC)

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/docs/analyzer/checkers.rst:2476-2477
+suppressed. However, the assumption about the argument is still modeled.
+For instance, if the argument to a function must be in between 0 and 255.
+If the value of the argument is unknown, the analyzer will assume that it is in
+this interval, even if warnings for this checker are disabled. Similarly, if a





Comment at: clang/docs/analyzer/checkers.rst:2490-2523
+**List of checked functions**
+
+``fgetc``, ``fread``, ``fwrite``, ``getc``, ``getchar``, ``getdelim``,
+``getenv``, ``getline``, ``isalnum``, ``isalpha``, ``isascii``, ``isblank``,
+``isdigit``, ``isgraph``, ``islower``, ``isprint``, ``ispunct``, ``isspace``,
+``isupper``, ``isxdigit``, ``read``, ``toascii``, ``tolower``, ``toupper``,
+``write``

balazske wrote:
> Szelethus wrote:
> > We should create an option or something the //actual// list of functions we 
> > model. This is the prime example of unsustainable documenting.
> Such function lists are used at documentation of other checkers, but I am not 
> sure if it is good to add such a long list here. Probably the 
> "DisplayLoadedSummaries" option of `apiModeling.StdCLibraryFunctions` checker 
> can be used, this lists only the actually found functions (that have 
> available declaration and are enabled), and the console output needs to be 
> observed to see the list. And this option is currently not documented.
> Such function lists are used at documentation of other checkers

Is it possible that those lists are not really expected to change? We do expect 
the list for this checker to grow, do we not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149447

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

[PATCH] D149447: [clang][analyzer] Improve documentation of StdCLibraryFunctionArgs checker (NFC)

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added inline comments.
This revision now requires changes to proceed.



Comment at: clang/docs/analyzer/checkers.rst:2457
 If the user disables the checker then the argument violation warning is
 suppressed. However, the assumption about the argument is still modeled. This
 is because exploring an execution path that already contains undefined behavior

This makes sense to me, and likely all of us knowledgable about symbolic 
execution (and clang in particular), but make little sense to end users. Could 
you add an example to ease reading?

"For instance, if the argument to  must be in between 0 and 255. If the 
value of the argument is unknown, the analyzer will assume that it is in this 
interval, even if warnings for this checker are disabled. Similarly, if a 
function mustn't be called with a null pointer but it is, analysis will stop on 
that execution path (similarly to a division by zero), with or without a 
warning."



Comment at: clang/docs/analyzer/checkers.rst:2459
 is because exploring an execution path that already contains undefined behavior
-is not valuable.
+is not valuable. This applies to all the restrictions that are listed below.
 

Restriction is not a bad word, but lets ease into it a bit.

"You can think of this checker as defining restrictions (pre- and 
postconditions) on standard library functions. Preconditions are checked, and 
when they are violated, a warning is emitted. Post conditions are added to the 
analysis, e.g. that the return value must be no greater than 255."



Comment at: clang/docs/analyzer/checkers.rst:2490-2523
+**List of checked functions**
+
+``fgetc``, ``fread``, ``fwrite``, ``getc``, ``getchar``, ``getdelim``,
+``getenv``, ``getline``, ``isalnum``, ``isalpha``, ``isascii``, ``isblank``,
+``isdigit``, ``isgraph``, ``islower``, ``isprint``, ``ispunct``, ``isspace``,
+``isupper``, ``isxdigit``, ``read``, ``toascii``, ``tolower``, ``toupper``,
+``write``

We should create an option or something the //actual// list of functions we 
model. This is the prime example of unsustainable documenting.



Comment at: clang/docs/analyzer/checkers.rst:2542
+modeling (and emit diagnostics) of additional functions that are defined in the
+POSIX standard. This option is disabled by default. Note that this option
+belongs to a separate built-in checker ``apiModeling.StdCLibraryFunctions`` and

Isn't this something that we either do or do not enable by default? My memory 
betrays me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149447

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

[PATCH] D149321: [clang][analyzer] Display buffer sizes in StdCLibraryFunctionArgs checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:889
+  State, getArgSVal(Call, ArgN)))
+Out << " (that is " << *Val << ")";
+}

I think this would be correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149321

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

[PATCH] D149321: [clang][analyzer] Display buffer sizes in StdCLibraryFunctionArgs checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added reviewers: steakhal, gamesh411.
Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

I am Szelethus, and I approve this warning message!

Now, on another issue, if we can output such a thorough message, we should 
start to consider that in the few cases where we cannot, maybe we should just 
choose to suppress the warning. But that is a job for another patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149321

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

[PATCH] D144003: [clang][analyzer] Improve bug reports of StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

LGTM! Most of the diagnostic messages are short but precise. I like this very 
much. Well done!

Balázs actually tested the change on open source projects, but accidentally 
uploaded it to our internal server, so I have seen them, and they look good 
from the diagnostic message standpoint.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144003

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

[PATCH] D144003: [clang][analyzer] Improve bug reports of StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Please run this on open source projects and upload the results.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:164-166
+/// This function is called when the current constraint represents the
+/// opposite of a constraint that was not satisfied in a given state, but
+/// the opposite is satisfied. In this case the available information in 
the

I know what you mean, but this could use an example.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:168-170
+/// description about the original constraint violation. This can be get by
+/// try to narrow the current constraint while it remains satisfied in the
+/// given program state. If useful information is found it is put into

This sentence makes so sense, unfortunately :( Could you rephrase, and, again, 
support it with an example?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:803
+if (ValuesPrinted)
+  MsgOs << " that is out of the accepted range; It should be ";
+else

Looking at the tests, this adds very little how about this:
" that is out of the accepted range; It should be " -> ", but should be "

Do you agree? I won't die on this hill, and am willing to accept this is good 
as-is if you really think that it is.

The other case is fine.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:807
+VC->describe(Call, C.getState(), Summary, MsgOs);
+// NegatedVC->describeBug1(Call, N->getState(), Summary, MsgOs);
 Msg[0] = toupper(Msg[0]);

Did you mean to leave this here?



Comment at: 
clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp:18
   __not_null(nullptr); // \
-  // expected-warning{{The 1st argument to '__not_null' should not be NULL}}
+  // expected-warning{{The 1st argument to '__not_null' is NULL that is out of 
the accepted range; It should be not NULL [}}
 }

This english is broken, but more importantly, this is the one scenario where 
the original message was just good enough -- in fact, better. How about either
1. Restoring the original message (by somehow excluding `NotNullConstraint` in 
the new `describe()`)
2. Or saying something like "The 1st argument to '__not_null' is NULL, but 
should be non-NULL"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144003

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

[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:449
   // No diagnostic if region was modified inside the frame.
   if (!CallExitLoc || isModifiedInFrame(N))
 return nullptr;

We also lost this, unfortunately, and this is kind of the point where we check 
whether the std function has anything to do with the uninitialized value (its 
been passed as parameter or something).

Please investigate whether losing this has any effect (and it very likely does).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145069

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

[PATCH] D143751: [clang][analyzer][NFC] Refactor code of StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143751

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

[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

We worked on this together, so I waited a bit for others to have a say in this, 
but this design seems like a no brainer to me. Please fix those comments, 
otherwise LGTM.

Also, while the patch is LGTM (moving code around is okay), the comment says 
"system headers having a chance to initialize the value but failing to do so", 
but do we ever check anywhere whether the function actually had the chance to 
change the value (passed by reference/pointer)?




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:507-515
   // Optimistically suppress uninitialized value bugs that result
   // from system headers having a chance to initialize the value
   // but failing to do so. It's too unlikely a system header's fault.
   // It's much more likely a situation in which the function has a failure
   // mode that the user decided not to check. If we want to hunt such
   // omitted checks, we should provide an explicit function-specific note
   // describing the precondition under which the function isn't supposed to

Since we don't check within this visitor whether the value is uninitialized, 
these comments are no longer up-to-date (and were not before your patch either).

What we are really doing here is flat out suppressing the report if it contains 
an inlined standard library function. What also needs to be said is that this 
visitor is as dumb as it comes -- its really down to the caller (or, more 
specifically, the one who registers this visitor) to make sure that this 
heuristic should be employed.

For example, if the bug report is about an uninitialized value that was passed 
to a system header function as a pointer/reference, this visitor could suppress 
the warning on the grounds that the system header likely would have initialized 
this value, but in some obscure cases the function could fail and not 
initialize the value, which could theoretically occur but practice it yields 
only (or mostly) false positives.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:517-523
 // We make an exception for system header functions that have no branches.
 // Such functions unconditionally fail to initialize the variable.
 // If they call other functions that have more paths within them,
 // this suppression would still apply when we visit these inner functions.
 // One common example of a standard function that doesn't ever initialize
 // its out parameter is operator placement new; it's up to the follow-up
 // constructor (if any) to initialize the memory.

Here as well, drop the bit about uninitializedness, unless you talk about it in 
the context of an example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145069

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

[PATCH] D143751: [clang][analyzer][NFC] Refactor code of StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

A high level comment before getting into (even more) nitty gritty stuff. But 
shut me down if I misunderstood whats happening.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:124-125
   // requirement would render the initialization of the Summary map infeasible.
+  // A new value constraint is created furthermore by the negate functionality
+  // of the constraint and returned as pointer.
   using ValueConstraintPtr = std::shared_ptr;

balazske wrote:
> Szelethus wrote:
> > Is this what you meant?
> Probably this is better:
> ```
>   // Mind that a pointer to a new value constraint can be created by negating 
> an existing
>   // constraint.
> ```
> The important thing is that one reason for the shared pointer is the negate 
> function that returns a pointer.
Can be, or //will// be? But otherwise, sure.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:271
+
+  private:
+using RangeApplyFunction =

balazske wrote:
> This new private section is the new added code.
While generalizing code is great, whenever its done by introducing function 
arguments / lambdas, the code becomes harder to understand. This is fine, as 
long as this complexity is justified, but I really needed to see what happened 
in the followup patch to see whats the gain behind this.

The gain is that you can capture a stream and construct a helpful message as 
the range is applied. 

Question: couldn't you just expose a lambda for the specific purpose for string 
smithing, and only that? Seems like all lambdas contain kind of the same thing: 
a call to `ConstraintManager::assumeInclusiveRange`. 

Maybe this design (for the above reasons) is worth documenting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143751

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

[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D144269#4143066 , @NoQ wrote:

> The challenging part with note tags is how do you figure out whether your bug 
> report is taint-related. The traditional solution is to check the `BugType` 
> but in this case an indeterminate amount of checkers may emit taint-related 
> reports.

Yeah, this is why we created a new type. Not sure what is the better 
infrastructure design, whether to create a subtype of `BugType` or `BugReport`, 
but it fundamentally achieves the same thing.

In D144269#4146809 , @dkrupp wrote:

> My fear with the interestingness is that it may propagating backwards 
> according to different "rules" than whot the taintedness is popagating in the 
> foward direction even with the "extensions" pointed out by steakhal.
> So the two types of propagation may be or can get out of sync.
>
> So if the above is not a concern and you think implementing this with 
> interestingness is more elegant, idiomatic and causes less maintenance 
> burden, I am happy to create an alternative patch with that solution.

@dkrupp and I discussed in detail whether to use FlowID's (what is currently 
implemented in the patch) or something similar, or reuse interestingness. 
Here's why we decided against reusing interestiness as is.

Interestingness, as it stands now, mostly expresses data-dependency, and is 
propageted with using the analyzers usualy somewhat conservative approach. 
While the length of a string is strictly speaking data dependent on the actual 
string, I don't think analyzer currently understand that. We approach taint 
very differently, and propagete it in some sense more liberally.

As I best recall, however, interestingness may be propagated through other 
means as well. If we reused interestingness, I fear that the interestiness set 
could actually be greater than the actual //interesting tainted// set, causing 
more notes to be emitted than needed.

For these reasons, which I admit are a result of some speculation, we concluded 
that interstingness as it is and taint are two different properties that are 
best separated.

In D144269#4143066 , @NoQ wrote:

> I think now's a good time to include a "generic data map"-like data structure 
> in `PathSensitiveBugReport` objects, so that checkers could put some data 
> there during `emitReport()`, which can be picked up by note tags and 
> potentially mutated in the process.

Maybe a new interestingness kind (D65723 )? 
Not sure how this design aged, but we don't really need to store an ID for 
this, so a simple interestingness flag (just not the default `Thorough` 
interestiness) is good enough.

In D144269#4146809 , @dkrupp wrote:

> The tricky part was is to how to show only that single "Taint originated 
> here" note tag at the taint source only which is relevant to the report. This 
> is done by remembering the unique flowid in the
> NoteTag in the forward analysis direction (see GenericTaintChecker:cpp:859) 
> `InjectionTag = createTaintOriginTag(C, TaintFlowId);` and then filtering out 
> the irrelevant 
> NoteTags when the the report is generated (when the lamdba callback is 
> called). See that flowid which reaches the sink is backpropagated in the 
> PathSensitiveBugreport (see GenericTaintCHekcer.cpp:167).
>
> FlowIds are unique and increased at every taint source 
> (GenericTaintChecker:869) and it is stored as an additional simple `int` in 
> the program state along with the already existing (Taint.cpp:22) TaintTagType.

If you propagate this property during analysis, those IDs may be needed, but a 
simple flag should suffice when BugReporter does it.


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

https://reviews.llvm.org/D144269

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

[PATCH] D143751: [clang][analyzer][NFC] Refactor code of StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Ugh, I admit, its a little hard to follow what happened here. You moved a lot 
of code around (I agree with that!), but also changed code as well. Can you 
just summarize what is NOT just moved code and needs a more thorough look?




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:124-125
   // requirement would render the initialization of the Summary map infeasible.
+  // A new value constraint is created furthermore by the negate functionality
+  // of the constraint and returned as pointer.
   using ValueConstraintPtr = std::shared_ptr;

Is this what you meant?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:218
+  /// Check if a single argument falls into a specific range.
+  /// The "range" can be built from sub-ranges that are closed intervals.
   class RangeConstraint : public ValueConstraint {

If you want a killer doc, use examples. Not saying this is not good enough, but 
it wouldn't hurt.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:220-222
+/// The constraint can be specified by allowing or disallowing the range.
+/// WithinRange indicates allowing the range, OutOfRange indicates
+/// disallowing it.

I'd place parts of this comments above the enum values declaration (now on line 
72), but even that small comment that is already there looks good enough. After 
that, maybe there is not much to note here.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:262
 
+  protected:
 bool checkSpecificValidity(const FunctionDecl *FD) const override {

Nice.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1031
+
+ProgramStateRef 
StdLibraryFunctionsChecker::NotNullConstraint::apply(ProgramStateRef State, 
const CallEvent &Call,
+  const Summary &Summary,

seems like this could use [[ 
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting | 
clang-format-diff.py]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143751

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

[PATCH] D143194: [clang][analyzer] Make messages of StdCLibraryFunctionsChecker user-friendly

[Kristóf Umann via Phabricator via cfe-commits]

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

LGTM!




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:95
+QualType ArgT, BasicValueFactory &BVF,
+DescString &Out);
+  /// Append textual description of a numeric range out of [RMin,RMax] to the

balazske wrote:
> Szelethus wrote:
> > Using a `raw_ostream` as a parameter sounds more elegant than a 
> > `SmallString` with a precise stack buffer length. Not to mention that you 
> > could call this function with `llvm::errs()` for easy debugging.
> I want to improve this in a later patch. The change does involve not new code 
> too (`getArgDesc` should be changed too and other places where the messages 
> are generated).
Fair enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143194

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

[PATCH] D143194: [clang][analyzer] Make messages of StdCLibraryFunctionsChecker user-friendly

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

A small nit, otherwise LGTM.

In D143194#4112538 , @balazske wrote:

> Probably it is not always useful to explain why the argument is wrong. In 
> cases when we can find out that the value is exactly between two consecutive 
> valid ranges we can display a note, or when the exact value is known. 
> Otherwise it may end up in something like "the value should be between 1 and 
> 4 or between 6 and 10, but it is less than 1 or exactly 5 or greater than 10".

You're totally right. I'm not sure how to approach tricky cases like that 
either.

> The "good" cases are (when more information is available): "the value is less 
> than 1", "the value is 5", "the value is greater than 10". If two bad ranges 
> are known it may be OK too: "the value is less than 1 or it is exactly 5". 
> The explanation may be possible to implement by test assumes for the negated 
> ranges.

Maybe we can start out with the easier cases, but we should check whether there 
is a checker that solves a different problem, and should try to aim at a 
somewhat general, reusable solution.




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:95
+QualType ArgT, BasicValueFactory &BVF,
+DescString &Out);
+  /// Append textual description of a numeric range out of [RMin,RMax] to the

Using a `raw_ostream` as a parameter sounds more elegant than a `SmallString` 
with a precise stack buffer length. Not to mention that you could call this 
function with `llvm::errs()` for easy debugging.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:100
+   QualType ArgT, BasicValueFactory &BVF,
+   DescString &Out);
 

Here as well.



Comment at: 
clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp:51-79
+int __single_val_0(int);  // [0, 0]
 int __single_val_1(int);  // [1, 1]
 int __range_1_2(int); // [1, 2]
-int __range_1_2__4_5(int);// [1, 2], [4, 5]
-void test_range(int x) {
-__single_val_1(2); // \
-// expected-note{{The 1st argument should be within the range [1, 1]}} \
-// expected-warning{{}}
-}
-// Do more specific check against the range strings.
+int __range_m1_1(int); // [-1, 1]
+int __range_m2_m1(int); // [-2, -1]
+int __range_m10_10(int); // [-10, 10]
+int __range_m1_inf(int); // [-1, inf]

Please clang-format the changes in this test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143194

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

[PATCH] D143194: [clang][analyzer] Make messages of StdCLibraryFunctionsChecker user-friendly

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a reviewer: gamesh411.
Szelethus added a comment.

Awesome, been a long time coming!!

Other than the minor observation of changing "of function" to "to", I'm 
inclined to accept this patch. We definitely should describe what the value IS, 
not just what is should be (aside from the cases concerning nullness, I think 
they are fine as-is), but seems to be another can of worms deserving of its own 
patch.

The reason why I'd solve the description of the argument separately is that 
other checkers also suffer from this problem (`alpha.security.ArrayBoundV2`, 
hello), and it opens conversations such as "What if we can't sensibly describe 
that value?", and on occasions, we have felt that the go-to solution should be 
just suppressing the warning. But then again, its an intentional false negative.




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:915
   Result += getArgDesc(ArgN);
-  Result += DK == Violation ? " should not be NULL" : " is not NULL";
+  Result += " of function '";
+  Result += getFunctionName(Call);

Looking at the tests, the word 'function' may be redundant and unnecessary.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:932
   Result += getArgDesc(ArgN);
-  Result += DK == Violation ? " should be " : " is ";
-
-  // Range kind as a string.
-  Kind == OutOfRange ? Result += "out of" : Result += "within";
-
-  // Get the range values as a string.
-  Result += " the range ";
-  if (Ranges.size() > 1)
-Result += "[";
-  unsigned I = Ranges.size();
-  for (const std::pair &R : Ranges) {
-Result += "[";
-const llvm::APSInt &Min = BVF.getValue(R.first, T);
-const llvm::APSInt &Max = BVF.getValue(R.second, T);
-Min.toString(Result);
-Result += ", ";
-Max.toString(Result);
-Result += "]";
-if (--I > 0)
-  Result += ", ";
+  Result += " of function '";
+  Result += getFunctionName(Call);

Here too.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:966
   Result += getArgDesc(ArgN);
-  Result += DK == Violation ? " should be " : " is ";
-  Result += "equal to or greater than the value of ";
+  Result += " of function '";
+  Result += getFunctionName(Call);

Here too.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:980
   }
   return Result.c_str();
 }

We are saying what the value should be, but neglect to say what it is instead. 
Something like this would be good:

"The value of the 1st argument to _nonnull should be equal to or less then 10, 
but is 11".

This is what I (and many of our users IIRC) miss in checker messages, like in 
those where we are supposed to describe what the value of the index is (besides 
it causing a buffer overflow).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143194

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

[PATCH] D142354: [analyzer] Create a stub for an std::variant checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

While we were there, we also dug into `std::any`, and learned that the analyzer 
can model it shockingly well. Hopefully we can submit a few patches that 
demonstrates it in a form of some test files.

In D142354#4079758 , @steakhal wrote:

> I would be interested in some of the free-functions dealing with variants, 
> such as `std::visit()`. https://godbolt.org/z/bbocrz4dG
> I hope that's also on the radar.

Thank you so much!! We definitely intend to work on this issue, and haven't 
thought of it before your suggestion.


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

https://reviews.llvm.org/D142354

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

[PATCH] D142354: [analyzer] Create a stub for an std::variant checker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D142354#4078450 , @NoQ wrote:

> Interesting, what specific goals do you have here? Are you planning to find 
> specific bugs (eg. force-unwrap to a wrong type) or just to model the 
> semantics?

Hi!

Meant to write this comment yesterday, but here we go. My idea was aiming for 
both of the goals you mentioned:

1. Emit diagnostics on improper usages of `std::variant`, which mostly boils 
down retrieving a field through a wrong type, yes. This will be, in my view, 
the main showpiece of the thesis.
2. Model the semantics.

In this or in a followup patch I think we should demonstrate with a few tests 
what we expect the checker to be capable of.

> In the latter case, have you explored the possibility of force-inlining the 
> class instead, like I suggested in the thread?

I have done some tests then, and figured that the analyzer can't really follow 
what happens in std::variant. I admit, now that I've taken a more thorough 
look, I was wrong! Here are some examples.

  std::variant v = 0;
  int i = std::get(v);
  clang_analyzer_eval(i == 0); // expected-warning{{TRUE}}
  10 / i; // FIXME: This should warn for division by zero!



  std::variant v = 0;
  std::string *sptr = std::get_if(&v);
  clang_analyzer_eval(sptr == nullptr); // expected-warning{{TRUE}}
  *sptr = "Alma!"; // FIXME: Should warn, sptr is a null pointer!



  void g(std::variant v) {
if (!std::get_if(&v))
  return;
int *iptr = std::get_if(&v);
clang_analyzer_eval(iptr == nullptr); // expected-warning{{TRUE}}
*iptr = 5; // FIXME: Should warn, iptr is a null pointer!
  }

In neither of these cases was a warning emitted, but that was a result of bug 
report suppression, because the exploded graph indicates that the errors were 
found.

We will need to teach these suppression strategies in these cases, the 
heuristic is too conservative, and I wouldn't mind some `NoteTag`s to tell the 
user something along the lines of "Assuming this variant holds and std::string 
value".

> Have you found a reasonable amount of code that uses `std::variant`, to test 
> your checker on?

Acid  has a few, csv-parser 
 in one place, there is a couple in 
config-loader  and cista 
, and a surprising amount in userver 
. Though its a question how 
painful it is to set up their dependencies.


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

https://reviews.llvm.org/D142354

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

[PATCH] D140387: [clang][analyzer] Add stream related functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

LGTM! I think I prefer this solution anyways. Please commit (the entire 
patchstack).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140387

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

[PATCH] D137790: [clang][analyzer] Remove report of null stream from StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:507-511
+  // FIXME: Use a return value from EvalFn instead of isDifferent.
+  // Some functions should not change state and not have any other
+  // (invalidation, including errno) effect.
+  // For example if 'feof' is used with unknown stream we know that errno does
+  // not change (and presumably no other state that is otherwise invalidated).

Okay, but how does `CheckerContext::isDifferent()` misbehave in that case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137790

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

[PATCH] D137790: [clang][analyzer] Remove report of null stream from StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

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

Please rerun the evaluation before commiting to confirm the results haven't 
changed! Otherwise, LGTM.




Comment at: clang/test/Analysis/stream.c:89
 void check_freopen_1(void) {
-  FILE *f1 = freopen("foo.c", "r", (FILE *)0); // expected-warning {{Stream 
pointer might be NULL}}
+  FILE *f1 = freopen("foo.c", "r", (FILE *)0); // Not reported by the stream 
checker.
   f1 = freopen(0, "w", (FILE *)0x123456);  // Do not report this as error.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137790

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

[PATCH] D140395: [clang][analyzer] Extend StreamChecker with some new functions.

[Kristóf Umann via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140395

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

[PATCH] D140387: [clang][analyzer] Add stream related functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

LGTM, granted you add that test in the followup commit. If possible, I'd prefer 
to have features tested in the patch that added them (but this is fine for now).




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:838
+  Result += getArgDesc(ArgN);
+  Result += DK == Violation ? " should not be zero" : " is not zero";
+  return Result.c_str();

balazske wrote:
> Szelethus wrote:
> > I don't mean to make you test every single `Case` or `ArgumentConstraint` 
> > you added, but `NotZeroConstraint` is brand new, and is not tested anywhere.
> This new class is probably not needed at all. It is always possible to use 
> `ReturnValueCondition(WithinRange, SingleValue(0))` or similar for nonzero 
> cases.
Convenience is a good enough reason. Please test this before commiting.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1848-1849
+// int fseek(FILE *stream, long offset, int whence);
+// FIXME: It is possible to get the 'SEEK_' values (like EOFv) for arg 2
+// condition.
+addToFunctionSummaryMap(

balazske wrote:
> Szelethus wrote:
> > Sure, but what should be fixed? We should check whether the the last 
> > argument is a `SEEK_*` value?
> It could be an improvement to detect possible values for the argument 2 that 
> are the `SEEK_*` values. Now the range [0,2] is used (the `SEEK_*` constants 
> are usually 0,1,2).
Could you please more-or-less copy-paste this inline into the code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140387

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

[PATCH] D140395: [clang][analyzer] Extend StreamChecker with some new functions.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Mostly LGTM, but I see that you have tests for the predecessor patch here as 
well, so I'll accept both at once.




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:89
   /// The last file operation called in the stream.
+  /// Can be nullptr.
   const FnDescription *LastOperation;

When can this occur?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140395

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

[PATCH] D140387: [clang][analyzer] Add stream related functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:838
+  Result += getArgDesc(ArgN);
+  Result += DK == Violation ? " should not be zero" : " is not zero";
+  return Result.c_str();

I don't mean to make you test every single `Case` or `ArgumentConstraint` you 
added, but `NotZeroConstraint` is brand new, and is not tested anywhere.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1105-1113
+  // It is possible that the function was evaluated in a checker callback
+  // where the state constraints are already applied, then no change 
happens
+  // here to the state (if the ErrnoConstraint did not change it either).
+  // If the evaluated function requires a NoteTag for errno change, it is
+  // added here.
+  if (const auto *D = dyn_cast_or_null(Call.getDecl()))
+if (const NoteTag *NT =

Can you name an example for that? `fgetpos`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140387

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

[PATCH] D140387: [clang][analyzer] Add stream related functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D140387#4021806 , @Szelethus wrote:

> Would be possible to test the errno specific changes as well?

I suppose the tests are done in the followup patch mostly?




Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:310
+  const MemRegion *ErrnoR = State->get();
+  if (!ErrnoR)
+return State;

balazske wrote:
> Szelethus wrote:
> > When can this occur? Maybe we can turn this early return to an assert -- 
> > when `ModelPOSIX` is true, this mustn't be null (if what I'm saying is 
> > correct).
> I am not totally sure that `errno` is found always (definition of `errno` is 
> encountered) if a summary for a standard function is added. The standard 
> function in itself must be there, otherwise no summary is added and this 
> function should not be called. But definition of `errno` may reside in other 
> header (errno.h) that is probably not included in the translation unit. If 
> the errno definition is not found the `ErrnoRegion` is not set (the checker 
> may still turned be on but does nothing, but these functions are allowed to 
> be called). Existence of `errno` is not even dependent on `ModelPOSIX`, 
> `errno` is defined in the (non POSIX) C standard.
Very well, I'm sold.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1705-1707
+  // FIXME: It should be allowed to have a null buffer if any of
+  // args 1 or 2 are zero. Remove NotNull check of arg 0, add a check
+  // for non-null buffer if non-zero size to BufferSizeConstraint?

balazske wrote:
> Szelethus wrote:
> > Maybe `StreamChecker` could take over the handling of that case? Seems like 
> > it also warns about the nullness of the first `fread` parameter.
> The current approach was that the NULL argument warning is generated by one 
> checker only, `StreamChecker` or this checker. The NULL check is already 
> built into these summaries so this is the better place for the warning. The 
> NULL warning is removed from `StreamChecker` in D137790. Additionally the 
> same problem can happen with `BufferSizeConstraint` at other function 
> summaries, we can say generally (or more often) that if a buffer has size of 
> 0 it is allowed to be NULL pointer.
Aha, okay, so we should extend the existing infrastructure to express this as 
well. Please do that in another patch though! Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140387

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

[PATCH] D137790: [clang][analyzer] Remove report of null stream from StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Are you sure that the refactoring made no changes to the results? Could you 
maybe just run a nightly or something like that to confirm?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137790

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

[PATCH] D140395: [clang][analyzer] Extend StreamChecker with some new functions.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Does this patch fix any false positives from before, or is this just all new 
stuff? I ask, because I wonder whats the shortest path towards popping these 
checkers out of alpha, and fix what we already have. By no means am I saying 
that we should postpone landing this, but take a more directed attempt at tying 
off loose ends after this stack.




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:952
 
+  // According to POSIX no change to 'errno' shall happen.
+

Why the comment? Seems like its solely `StdLibraryFunctionChecker`'s job to 
handle errno.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:979
 
+  // According to POSIX no change to 'errno' shall happen.
+

Same.



Comment at: clang/test/Analysis/stream-errno-note.c:1-2
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.unix.Stream,alpha.unix.Errno,apiModeling.StdCLibraryFunctions
 \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true 
-analyzer-output text -verify %s
+

Can you break this line up, such that we have an `-analyzer-checker=` argument 
for each package/checker?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140395

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

[PATCH] D140387: [clang][analyzer] Add stream related functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Would be possible to test the errno specific changes as well?




Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:310
+  const MemRegion *ErrnoR = State->get();
+  if (!ErrnoR)
+return State;

When can this occur? Maybe we can turn this early return to an assert -- when 
`ModelPOSIX` is true, this mustn't be null (if what I'm saying is correct).



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:839
+  Result += DK == Violation ? " should not be zero" : " is not zero";
+  return Result.c_str();
+}

Just thinking aloud, no changes required here necesserily -- in most places, we 
use a combination of `SmallString` and `raw_svector_ostream` so construct 
strings, but I can't find anything set in stone about the practice (mostly 
looking at [[ 
https://llvm.org/docs/ProgrammersManual.html#string-like-containers | LLVM's 
Programmers Manual ]]). Isn't this just good enough? Is `llvm::Twine` worth the 
hassle?



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1691-1702
+  .Case({ArgumentCondition(1U, WithinRange, Range(1, SizeMax)),
+ ArgumentCondition(2U, WithinRange, Range(1, SizeMax)),
+ ReturnValueCondition(BO_LT, ArgNo(2)),
  ReturnValueCondition(WithinRange, Range(0, SizeMax))},
-ErrnoIrrelevant)
+ErrnoNEZeroIrrelevant)
+  .Case({ArgumentCondition(1U, WithinRange, Range(1, SizeMax)),
+ ReturnValueCondition(BO_EQ, ArgNo(2)),

If I undetstand correctly, these 3 cases will cause the current path of 
execution to split into 3. I vaguely recall some arguments against this, but 
that's been a while, did the stance on this change? I see a number of already 
commited summaries with 3 cases, so this could be fine for all I know.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1705-1707
+  // FIXME: It should be allowed to have a null buffer if any of
+  // args 1 or 2 are zero. Remove NotNull check of arg 0, add a check
+  // for non-null buffer if non-zero size to BufferSizeConstraint?

Maybe `StreamChecker` could take over the handling of that case? Seems like it 
also warns about the nullness of the first `fread` parameter.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1848-1849
+// int fseek(FILE *stream, long offset, int whence);
+// FIXME: It is possible to get the 'SEEK_' values (like EOFv) for arg 2
+// condition.
+addToFunctionSummaryMap(

Sure, but what should be fixed? We should check whether the the last argument 
is a `SEEK_*` value?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140387

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

[PATCH] D140387: [clang][analyzer] Add stream related functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D140387#4007751 , @balazske wrote:

> This patch and D140395  is (almost) the 
> same code as D135360  and D135247 
> . The changes are separated for the 
> different checkers. Tests are added at the second patch.

Well, that is confusing. Which patches should I review? Can you please only 
have a single opened differential per change, and a single patch stack? If you 
decided that this patch is **the ONE** for the `ErrnoModeling` changes, please 
abandon the other patch with the very same change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140387

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

[PATCH] D140387: [clang][analyzer] Add stream related functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a reviewer: gamesh411.
Szelethus added a comment.

Some of the changes are also present in D135247 
. I suppose you're in the middle of splitting 
those patches apart and remaking the patch stack?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140387

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

[PATCH] D137790: [clang][analyzer] Remove report of null stream from StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I have a fear that we may have too few results as well -- maybe we should 
expand our testing infrastructure with more POSIX API heavy codebases. Looking 
at a few new projects, https://github.com/audacity/audacity looks like a good 
candidate, but of course it often turns out that adding a new project to the 
benchmark is more troublesome than it appears...

In D137790#4004803 , @balazske wrote:

> Here is a postgers result link 
> ,
>  results are from **StdCLibraryFunctionArgs** checker. The second report is 
> in file relcache.c (it looks like that `data` pointer is really null but the 
> `len` value is 0, and in this special case the data pointer should be ignored 
> according to the standards). A FIXME for this case was added to the code in 
> D135247 .

Could you please re-evaluate the patch stack with the new fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137790

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

[PATCH] D137790: [clang][analyzer] Remove report of null stream from StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D137790#3992216 , @balazske wrote:

> On the postgres results, the second is one that can be fixed in the checker 
> (add special cases to `StdLibraryFunctionsChecker` for zero `len` or `size` 
> `fread` and `fwrite` arguments). The others are false positives because the 
> error path is impossible because implicit constraints (what is not known to 
> the analyzer) on variables.

I'd want some more thorough explanations as well. Path events are numbered in 
CodeChecker, you can use them to explain why you think the report is a false 
positive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137790

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

[PATCH] D137722: [clang][analyzer] No new nodes when bug is detected in StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:219
+  /// @param State The state of the generated node.
+  /// @param Pred The transition will be generated from the specified Pred node
+  /// to the newly generated node.

What I'm missing here is some guidance. Why would I pick this overload instead 
of the 2-parameter one? Especially for beginners, this is very confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137722

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

[PATCH] D137790: [clang][analyzer] Remove report of null stream from StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D137790#3992216 , @balazske wrote:

> Some reports can be found here 
>  (if the 
> link works and the data does not expire), the runs stored on 2022-12-09.
>
> Results appeared only projects "postgres" and "curl" (from 
> memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres,libwebm) from 
> checkers **StdCLibraryFunctionArgs** and **Errno**.
>
> On the postgres results, the second is one that can be fixed in the checker 
> (add special cases to `StdLibraryFunctionsChecker` for zero `len` or `size` 
> `fread` and `fwrite` arguments). The others are false positives because the 
> error path is impossible because implicit constraints (what is not known to 
> the analyzer) on variables.
>
> These curl 
> 
>  results look faulty, the last `fclose` call looks not recognized.

Please make the link a little more reviewer friendly. You can make a 
CodeChecker URL where the appropriate runs are already selected, with the 
appropriate checkers being filtered, and uniquing turned on, like this:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=libwebm_libwebm-1.0.0.27_baseline&run=postgres_REL_13_0_baseline&run=ffmpeg_n4.3.1_baseline&run=sqlite_version-3.33.0_baseline&run=openssl_openssl-3.0.0-alpha7_baseline&run=vim_v8.2.1920_baseline&run=twin_v0.8.1_baseline&run=curl_curl-7_66_0_baseline&run=tmux_2.6_baseline&run=memcached_1.6.8_baseline&is-unique=on&diff-type=New&checker-name=alpha.unix.Errno&checker-name=alpha.unix.StdCLibraryFunctionArgs

Also, I'd appreciate links to the specific reports you are describing. I'm not 
sure what postgres results you are talking about, because I suspect I listed 
them differently. You made a link to the curl result, so please do it for the 
others as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137790

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D135360#3981420 , @balazske wrote:

> My current approach is that the POSIX is more strict than the C standard 
> (POSIX allows a subset of what C allows). I do not see (errno related) 
> contradiction between these standards

Alright, so if we wanna support the C standard thinking, we would just create 
an option that would be a little more noisy. Sounds fair enough.

> (except initialization to zero of errno missing from POSIX, and possible 
> negative value in errno in POSIX). One problem exists now, that `errno` is 
> set to a non-zero (but not positive only) value at error. Probably we should 
> change this to be always positive at error, according to the C standard 
> `errno` is positive only, and POSIX does not tell that errno can be negative.

According to this (which I assume you've seen as well): 
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

> Description
> ===
>
> [...]
> Some of the functionality described on this reference page extends the ISO C 
> standard. Any conflict between the requirements described here and the ISO C 
> standard is unintentional. [...]
> [...]
>
> Issue 6
> ===
>
> Values for errno are now required to be distinct positive values rather than 
> non-zero values. This change is for alignment with the ISO/IEC 9899:1999 
> standard.

So yes, I'd say its reasonable to assume in POSIX modthat if `errno` is 
non-zero, it is **positive**.

> The checker currently works only in POSIX mode for every function, the 
> **ModelPOSIX** setting controls only if additional functions are added (all 
> with POSIX rules) (these functions could be added with C rules too).

I see a lot of mention of POSIX vs. the C standard, yet there seems to be no 
mention of these differences in the comments added by this patch or already 
commited. Maybe we could do better on documenting this important difference in 
behaviour.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1818-1857
+// int fgetpos(FILE *restrict stream, fpos_t *restrict pos);
+// From 'The Open Group Base Specifications Issue 7, 2018 edition':
+// "The fgetpos() function shall not change the setting of errno if
+// successful."
+addToFunctionSummaryMap(
+"fgetpos",
+Signature(ArgTypes{FilePtrRestrictTy, FPosTPtrRestrictTy},

martong wrote:
> It is very good to see these new summaries! Would they be meaningful and 
> usable without the changes in the StreamChecker? If yes, then I think we 
> should split this into 2 patches.
Can you please do this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D137722: [clang][analyzer] No new nodes when bug is detected in StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:953
 if (FailureSt && !SuccessSt) {
-  if (ExplodedNode *N = C.generateErrorNode(NewState))
+  if (ExplodedNode *N = C.generateErrorNode(NewState, NewNode))
 reportBug(Call, N, Constraint.get(), Summary, C);

Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Let me know if I got this right. The reason behind `generateErrorNode` 
> > > not behaving like it usually does for other checkers is because of the 
> > > explicitly supplied `NewState` parameter -- in its absence, the 
> > > //current// path of execution is sunk. With this parameter, a new 
> > > parallel node is. Correct?
> > The `NewState` only sets the state of the new error node, if it is nullptr 
> > the current state is used. A new node is always added. The other new node 
> > functions (`addTransition`, `generateNonFatalErrorNode`, `generateSink` and 
> > `addSink`) have a version that can take a predecessor node, only 
> > `generateErrorNode` did not have this (and I can not find out why part of 
> > these is called "generate" and other part "add" instead of using only 
> > "generate" or "add").
> > 
> > The new function is used when a node sequence 
> > `CurrentNode->A->B->ErrorNode` is needed. Without the new function it is 
> > only possible to make a `CurrentNode->ErrorNode` transition, and the 
> > following incorrect graph is created:
> > ```
> > CurrentNode->A->B
> >   |->ErrorNode
> > ```
> > The code here does exactly this (before the fix), in `NewNode` a sequence 
> > of nodes is appended (like A and B above), and if then an error node is 
> > created it is added to the CurrentNode. Not this is needed here, the error 
> > node should come after B. Otherwise analysis can continue after node B 
> > (that path is invalid because a constraint violation was found).
> > (The "CurrentNode" is a `Pred` value that is stored in `CheckerContext` and 
> > not changed if other nodes are added.)
> I've been wondering that, especially looking at the test case. Seems like 
> this loop runs only once, how come that new nodes are added on top of 
> `CurrentNode` (which, in this case refers to `C.getPredecessor()`, right?)? I 
> checked the checker's code, and I can't really see why `A` and `B` would ever 
> appear. Isn't that a bug?
My thinking was that each checker, unless it does state splits, should really 
only create a single node per callback, right? The new state, however many 
changes it contains, should be added all at once in the single callback, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137722

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

[PATCH] D137722: [clang][analyzer] No new nodes when bug is detected in StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:953
 if (FailureSt && !SuccessSt) {
-  if (ExplodedNode *N = C.generateErrorNode(NewState))
+  if (ExplodedNode *N = C.generateErrorNode(NewState, NewNode))
 reportBug(Call, N, Constraint.get(), Summary, C);

balazske wrote:
> Szelethus wrote:
> > Let me know if I got this right. The reason behind `generateErrorNode` not 
> > behaving like it usually does for other checkers is because of the 
> > explicitly supplied `NewState` parameter -- in its absence, the //current// 
> > path of execution is sunk. With this parameter, a new parallel node is. 
> > Correct?
> The `NewState` only sets the state of the new error node, if it is nullptr 
> the current state is used. A new node is always added. The other new node 
> functions (`addTransition`, `generateNonFatalErrorNode`, `generateSink` and 
> `addSink`) have a version that can take a predecessor node, only 
> `generateErrorNode` did not have this (and I can not find out why part of 
> these is called "generate" and other part "add" instead of using only 
> "generate" or "add").
> 
> The new function is used when a node sequence `CurrentNode->A->B->ErrorNode` 
> is needed. Without the new function it is only possible to make a 
> `CurrentNode->ErrorNode` transition, and the following incorrect graph is 
> created:
> ```
> CurrentNode->A->B
>   |->ErrorNode
> ```
> The code here does exactly this (before the fix), in `NewNode` a sequence of 
> nodes is appended (like A and B above), and if then an error node is created 
> it is added to the CurrentNode. Not this is needed here, the error node 
> should come after B. Otherwise analysis can continue after node B (that path 
> is invalid because a constraint violation was found).
> (The "CurrentNode" is a `Pred` value that is stored in `CheckerContext` and 
> not changed if other nodes are added.)
I've been wondering that, especially looking at the test case. Seems like this 
loop runs only once, how come that new nodes are added on top of `CurrentNode` 
(which, in this case refers to `C.getPredecessor()`, right?)? I checked the 
checker's code, and I can't really see why `A` and `B` would ever appear. Isn't 
that a bug?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137722

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

[PATCH] D137790: [clang][analyzer] Remove report of null stream from StreamChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

IIRC we talked about it would only really make sense to evaluate this patch 
stack as a whole, not piece by piece, but I'm not seeing results on open source 
projects here either. Can you please post them?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137790

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

The patch looks OK now, I'll get to inspecting the others.

In D135247#3977403 , @balazske wrote:

> The "strange" test failures that showed up earlier were probably caused by a 
> bug that is fixed in the D137722 . I just 
> read that this patch is rebased to D137722  
> too, fixed the dependency stack.

Very well!

> There was another problem with circular dependencies (because 
> **StdCLibraryFunctionArgsChecker** had a dependency to StreamChecker, this is 
> removed in the last patch). The checker option must be not a problem, the 
> checker (`StdLibraryFunctionsChecker`) can be disabled (but is normally not 
> because it is an **apiModeling** checker) or the **ModelPOSIX** option turned 
> off independently if `StreamChecker` is enabled or not.

Okay, so the checker behaves OK if `StdLibraryFunctionsChecker` is disabled. As 
long as it doesn't crash, this is fine, you shouldn't disable it in practice 
anyways!

> The goal (should work at the end of this patch stack) is that StreamChecker 
> can report all bugs that it can find, and there is no case when both checkers 
> report a bug (in different way). If **ModelPOSIX** is turned off and 
> StreamChecker is enabled, for `fseek` for example no bug is found if stream 
> is NULL, and value of `errno` is just invalidated in all cases (like it works 
> if StreamChecker is disabled too), but the stream state and file position is 
> still checked by StreamChecker for all functions.

This sounds reasonable. It means that no parts of `StdLibraryFunctionsChecker` 
(including its option) is a "hard" dependency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D137722: [clang][analyzer] No new nodes when bug is detected in StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:953
 if (FailureSt && !SuccessSt) {
-  if (ExplodedNode *N = C.generateErrorNode(NewState))
+  if (ExplodedNode *N = C.generateErrorNode(NewState, NewNode))
 reportBug(Call, N, Constraint.get(), Summary, C);

Let me know if I got this right. The reason behind `generateErrorNode` not 
behaving like it usually does for other checkers is because of the explicitly 
supplied `NewState` parameter -- in its absence, the //current// path of 
execution is sunk. With this parameter, a new parallel node is. Correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137722

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

[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D135360#3862260 , @martong wrote:

> In D135360#3839890 , @balazske 
> wrote:
>
>> I found some anomalies during development:
>>
>> - If the checker **StdCLibraryFunctions** is added as dependency for 
>> **alpha.unix.Stream** in //checkers.td// I get some "unexplainable" test 
>> failures.
>
> Could you please elaborate? I don't see how to help you with it without 
> seeing more details.

Mind that dependencies also establish the order of callbacks (dependent 
checkers are called after their dependencies).

> When it is ambiguous then I'd check the latest standards of both POSIX and C. 
> If it is still doubtful then I'd vote for the C standard and would report a 
> defect towards the POSIX community.



In D135360#3888632 , @balazske wrote:

> About the "ftell" problem: The POSIX rules are really an extension of the C 
> standard rules. At `ftell` according to C standard `errno` should be set to a 
> positive value if error occurs. The POSIX rules extend this: `errno` is not 
> changed if no error occurs. [...] It may be best to use the POSIX rules for 
> the checker, because the C standard does not say much and may need to require 
> setting of `errno` to 0 before a standard function call.

Interesting pair of perspectives, I think a reasonable checker should be a 
little more conservative, more akin to what POSIX seems to specify.

In D135360#3885556 , @martong wrote:

> In D135360#3885494 , @balazske 
> wrote:
>
>> [...] Probably this should be a discourse question?
>
> Okay then, I think it is worth to have a discourse question. But you could 
> ask the wider "Clang" community, so I would not post the question as 
> something that is related strictly to the static analyzer.

Did this thread ever materialize? I admit I didn't follow :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135360

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Sorry abour my previous reply, I messed up the thread I was replying to. I 
better see what is going on.

Do you have a better handle on @martong's previous comment (D135247#3867603 
)? Do we know why this strange 
behaviour occurs with StreamChecker? I assumed that the stream related modeling 
is done in parallel to that. Is it about the return value?

In D135247#3865357 , @balazske wrote:

> Probably if the `StdLibraryFunctionsChecker` object can be get from 
> `StreamChecker` (a new function is needed) it is possible to check the option 
> and use `CheckerManager::reportInvalidCheckerOptionValue` (for the 
> `StdLibraryFunctionsChecker`). But not sure if it does what we want here.

You can only retrieve a checker object if you can access the class definition 
of it (as you are aware), so unless you move `StdLibraryFunctionsChecker` to a 
header file, which doesn't seem like the correct option now (kinda goes against 
the independent module architecture  (despite us talking about intertwining 
these "independent" modules)).

For me, the right course seems to be to detach this option from 
`StdLibraryFunctionsChecker`, and make it an analyzer level option. If this 
option so severely impacts both checkers, it feels more like an analyzer level 
option anyways. That will enable you to query the option from 
`shouldRegisterStreamChecker`. Whether you'd prefer to hard error or only warn 
about the failure to enable a checker is something I also struggle with, but 
you could start out with the hard error first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Do I understand correctly that for the time being, the strategy is to assume 
apiModeling to be enabled by default and not enforce it explicitly?

I suppose I have the ol' reliable question: did you evaluate this on any 
project yet?

In D135247#3839543 , @martong wrote:

>> The two checkers work together and 'apiModeling.StdCLibraryFunctions'
>> and its 'ModelPOSIX=true' option should be now a dependency of
>> checker 'alpha.unix.Stream'.
>
> Is there a way to stop the analysis with an error if "ModelPOSIX=true" is not 
> set but the  'alpha.unix.Stream' checker is enabled? @Szelethus, maybe you 
> have some insights with this?

The problem is, Dependencies and Weakdependencies both enforce a strict order 
of checker registration. What is discussed here is the following order:

1. StdCLibraryFunctionsChecker (modelling part)
2. StreamChecker
3. StdCLibraryFunctionArgsChecker (reporting part)

This falls apart because the modeling and reporting checkers are, in fact, the 
same checker in implementation. The "proper" solution would be to split up 
StdLibraryFunctions not in terms of a new checker entry in Checkers.td, but to 
split up the checker in implementation.




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:786
   /// constraints (and different return value constraints).
-  const NoErrnoConstraint ErrnoIrrelevant{};
+  const NoErrnoConstraint ErrnoUnchanged{};
+  const ResetErrnoConstraint ErrnoIrrelevant{};

Hmm, do we need to specify this? Can't this be the default?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Szelethus marked an inline comment as done.
Closed by commit rGa504ddc8bf9d: [analyzer] Initialize regions returned by 
CXXNew to undefined (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D135375?vs=465779&id=470821#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/NewDelete-checker-test.cpp
  clang/test/Analysis/cxx-member-initializer-const-field.cpp
  clang/test/Analysis/new-ctor-conservative.cpp
  clang/test/Analysis/new-ctor-recursive.cpp
  clang/test/Analysis/new.cpp
  clang/test/Analysis/placement-new.cpp
  clang/test/Analysis/reinterpret-cast.cpp
  clang/test/Analysis/uninit-const.cpp

Index: clang/test/Analysis/uninit-const.cpp
===
--- clang/test/Analysis/uninit-const.cpp
+++ clang/test/Analysis/uninit-const.cpp
@@ -21,9 +21,11 @@
 
 int f10(void) {
   int *ptr;
-
-  ptr = new int; //
-  if(*ptr) {
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+  if(*ptr) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
+ // expected-note@-1 {{Branch condition evaluates to a garbage value}}
 doStuff4(*ptr);
   }
   delete ptr;
@@ -32,10 +34,12 @@
 
 int f9(void) {
   int *ptr;
-
-  ptr = new int; //
-
-  doStuff_uninit(ptr); // no warning
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+ // expected-note@-1{{Value assigned to 'ptr'}}
+  doStuff_uninit(ptr); // expected-warning{{1st function call argument is a pointer to uninitialized value [core.CallAndMessage]}}
+   // expected-note@-1{{1st function call argument is a pointer to uninitialized value}}
   delete ptr;
   return 0;
 }
Index: clang/test/Analysis/reinterpret-cast.cpp
===
--- clang/test/Analysis/reinterpret-cast.cpp
+++ clang/test/Analysis/reinterpret-cast.cpp
@@ -77,15 +77,11 @@
   class Derived : public Base {};
 
   void test() {
-	Derived* p;
-	*(reinterpret_cast(&p)) = new C;
-	p->f();
-
-// We should still be able to do some reasoning about bindings.
-p->x = 42;
-clang_analyzer_eval(p->x == 42); // expected-warning{{TRUE}}
+Derived* p;
+*(reinterpret_cast(&p)) = new C;
+p->f(); // expected-warning{{Called function pointer is an uninitialized pointer value [core.CallAndMessage]}}
   };
-}
+} // namespace PR15345
 
 int trackpointer_std_addressof() {
   int x;
Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -112,6 +112,10 @@
 namespace testHeapAllocatedBuffer {
 void g2() {
   char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+   // FIXME: The message is misleading -- we should
+   // state that a pointer to an uninitialized value
+   // is stored.
+   // expected-note@-4{{Storing uninitialized value}}
   long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
   (void)lp;
 }
Index: clang/test/Analysis/new.cpp
===
--- clang/test/Analysis/new.cpp
+++ clang/test/Analysis/new.cpp
@@ -177,15 +177,10 @@
   clang_analyzer_eval(p.y == 2); // expected-warning{{TRUE}}
 }
 
-//
-// Incorrectly-modelled behavior
-//
-
 int testNoInitialization() {
   int *n = new int;
 
-  // Should warn that *n is uninitialized.
-  if (*n) { // no-warning
+  if (*n) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
 delete n;
 return 0;
   }
@@ -193,6 +188,10 @@
   return 1;
 }
 
+//===--===//
+// Incorrectly-modelled behavior.
+//===--===//
+
 int testNoInitializationPlacement() {
   int n;
   new (&n) int;
Index: clang/test/Analysis/new-ctor-recursive.cpp
===
--- clang/test/Analysis/new-ctor-recursive.cpp
+++ clang/test/Analysis/new-ctor-recursive.cpp
@@ -51,11

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Seems like the issues mentioned above are real, but orthogonal to this patch. 
Would it be okay to address them in followup patches? @martong @NoQ




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:927
 SVal RetVal = State->getSVal(CNE, LCtx);
+State = State->bindDefaultInitial(RetVal, UndefinedVal{}, LCtx);
 

steakhal wrote:
> Yeey, finally we will have this :D
> 
> I wonder if we could query from the `ASTContext` if we have a trivially 
> constructible class typeor something as a first approximation.
And a result skip the rest of this function?



Comment at: clang/test/Analysis/NewDelete-checker-test.cpp:388-392
+  ~DerefClass() {
+int i = 0;
+x = &i;
+*x = 1;
+  }

steakhal wrote:
> This change seems unrelated.
> Could you elaborate on that?
The test case in its original version would have emitted an uninitialized use 
report, which is fine, but the intention is to test double deletes, not 
uninitialized use, hence the seemingly unrelated change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Some early results:

https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&is-unique=on&run=%2acxxnew_baseline&newcheck=%2acxxnew_patched&diff-type=New

I'm still waiting on a few projects, but this is something. A few reports from 
`alpha.security.ArrayBound`, I still need to look into that.

This 

 report from `core.uninitialized.Assign` is interesting. First off, I'd 
appreciate if the "storing uninitialized value" was placed //inside// the notes 
about the call to `QScopedArrayPointer`'s constructor. Otherwise, the report 
looks correct.

Not too happy about this 

 report by `core.CallAndMessage`. Message 9 is placed on the line

  applyColorTransform(d->colorSpace.transformationToColorSpace(colorSpace));

Messag 10 talks about what happens in `applyColorTransform`, but the important 
thing happens at the evaluation of the argument, which is not described, so 
this isn't a very good bug report, can't tell whether its false.

Reports from `core.UndefinedBinaryOperatorResult` here 

 and `alpha.core.Conversion` here 

 are interesting, because there doesn't need to be caused by uninitialized 
dynamic memory, but rather the fact that the memory is being modeled at all. I 
suspect this is due us maybe discarding array size information or something 
similar when the value held in `UnknownVal`?

The rest of the reports seem nobrainder gains.

Interstingly, 3 reports are lost 

 from `alpha.security.ArrayBound`, but I'm not sure how seriously do we take 
this checker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Just a note on the test files -- I've diverged from the usual stance of just 
changing what the new output is, to modifying the test files. The reason is 
that reading an undefined value is a fatal error, leading to the analyzer to 
stop analyzing prematurely, and I think these cases were trying to test 
something else, not uninitialized value usage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135375

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

[PATCH] D135375: [analyzer] Initialize regions returned by CXXNew to undefined

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, martong, steakhal, balazske, isuckatcs.
Szelethus added a project: clang.
Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.

Discourse mail: 
https://discourse.llvm.org/t/analyzer-why-do-we-suck-at-modeling-c-dynamic-memory/65667

`malloc()` returns a piece of uninitialized dynamic memory. We were (almost) 
always to model this behaviour. Its C++ counterpart, `operator new` is a lot 
more complex, because it allows for initialization, the most complicated of 
which the usage of constructors.

We gradually became better in modeling constructors, but for some reason, most 
likely for reasons lost in history, we never actually modeled the case when the 
memory returned by `operator new` was just simply uninitialized. This patch 
(attempts) to fix this tiny little error.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135375

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/NewDelete-checker-test.cpp
  clang/test/Analysis/cxx-member-initializer-const-field.cpp
  clang/test/Analysis/new-ctor-conservative.cpp
  clang/test/Analysis/new-ctor-recursive.cpp
  clang/test/Analysis/new.cpp
  clang/test/Analysis/placement-new.cpp
  clang/test/Analysis/reinterpret-cast.cpp
  clang/test/Analysis/uninit-const.cpp

Index: clang/test/Analysis/uninit-const.cpp
===
--- clang/test/Analysis/uninit-const.cpp
+++ clang/test/Analysis/uninit-const.cpp
@@ -21,9 +21,11 @@
 
 int f10(void) {
   int *ptr;
-
-  ptr = new int; //
-  if(*ptr) {
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+  if(*ptr) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
+ // expected-note@-1 {{Branch condition evaluates to a garbage value}}
 doStuff4(*ptr);
   }
   delete ptr;
@@ -32,10 +34,12 @@
 
 int f9(void) {
   int *ptr;
-
-  ptr = new int; //
-
-  doStuff_uninit(ptr); // no warning
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+ // expected-note@-1{{Value assigned to 'ptr'}}
+  doStuff_uninit(ptr); // expected-warning{{1st function call argument is a pointer to uninitialized value [core.CallAndMessage]}}
+   // expected-note@-1{{1st function call argument is a pointer to uninitialized value}}
   delete ptr;
   return 0;
 }
Index: clang/test/Analysis/reinterpret-cast.cpp
===
--- clang/test/Analysis/reinterpret-cast.cpp
+++ clang/test/Analysis/reinterpret-cast.cpp
@@ -77,15 +77,11 @@
   class Derived : public Base {};
 
   void test() {
-	Derived* p;
-	*(reinterpret_cast(&p)) = new C;
-	p->f();
-
-// We should still be able to do some reasoning about bindings.
-p->x = 42;
-clang_analyzer_eval(p->x == 42); // expected-warning{{TRUE}}
+Derived* p;
+*(reinterpret_cast(&p)) = new C;
+p->f(); // expected-warning{{Called function pointer is an uninitialized pointer value [core.CallAndMessage]}}
   };
-}
+} // namespace PR15345
 
 int trackpointer_std_addressof() {
   int x;
Index: clang/test/Analysis/placement-new.cpp
===
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -112,6 +112,10 @@
 namespace testHeapAllocatedBuffer {
 void g2() {
   char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+   // FIXME: The message is misleading -- we should
+   // state that a pointer to an uninitialized value
+   // is stored.
+   // expected-note@-4{{Storing uninitialized value}}
   long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
   (void)lp;
 }
Index: clang/test/Analysis/new.cpp
===
--- clang/test/Analysis/new.cpp
+++ clang/test/Analysis/new.cpp
@@ -184,8 +184,7 @@
 int testNoInitialization() {
   int *n = new int;
 
-  // Should warn that *n is uninitialized.
-  if (*n) { // no-warning
+  if (*n) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
 delete n;
 return 0;
   }
Index: clang/test/Analysis/new

[PATCH] D130055: Clang extensions yolo, woot & kaboom

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a subscriber: balazske.
Szelethus added a comment.

Yeah, I'm afraid no fun is allowed on this block. On another note, `kaboom` is 
interesting, shouldn't we assume all functions to be `kaboom` unless proven to 
be `woot`?

@balazske's work on checking the return value of certain function calls is 
related to this as well!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130055

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

[PATCH] D127643: [Static Analyzer] Structured bindings to data members

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I read https://en.cppreference.com/w/cpp/language/structured_binding carefully, 
and there are a number of interesting rules that might deserve their own test 
case, even if this isn't the patch where you solve that issue, or believe that 
the solution handles it without the need for special case handling.

Just to name a few, you seem to not have test cases for when the bindings are 
cv-qualified. If you declare structured bindings like this:

  struct s {
int a;
int b;
  };
  
  void a(void) {
s tst;
  
auto [i, j](tst); // Syntax (3) according to cppreference
  
int x = i; // expected-warning{{Assigned value is garbage or undefined}}
  }

then the bindings are direct initialized, not copy initialized.

I'm not sure that the actual standard 
 has 
anything that cppreference doesn't, but maybe we could give it a glance.




Comment at: clang/test/Analysis/uninit-structured-binding-struct.cpp:10
+
+void a(void) {
+  s tst;

In the long run, it might make your own life easier to create more talkative 
test function names, like `testPODDecomp` or smt like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127643

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

[PATCH] D128064: [Static Analyzer] Small array binding policy

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.
Herald added a subscriber: steakhal.

No need for post commit fixes, just general observations since I noticed them.




Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2427
+  uint64_t ArrSize = CAT->getSize().getLimitedValue();
+  if (ArrSize > SmallArrayLimit)
+return None;

Unless we use this a lot, it'd be better to just use `AnalyzerOptions` 
natively, it'd be more visible that this is directly configurable by the user.

There is no need to change this once its here, just a general design 
observation.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2466
   // Handle lazy compound values.
-  if (isa(Init))
+  if (Optional LCV =
+  Init.getAs()) {

For `SVal::getAs`, we usually use `auto` for the return value type (which has 
its history: D54877#inline-484319!).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128064

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

[PATCH] D116597: [analyzer] Don't track function calls as control dependencies

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Szelethus marked an inline comment as done.
Closed by commit rGfd8e5762f86f: [analyzer] Don't track function calls as 
control dependencies (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D116597?vs=419132&id=421450#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116597

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -1,10 +1,10 @@
-// RUN: %clang_analyze_cc1 %s -std=c++14 \
+// RUN: %clang_analyze_cc1 %s -std=c++17 \
 // RUN:   -verify=expected,tracking \
 // RUN:   -analyzer-config track-conditions=true \
 // RUN:   -analyzer-output=text \
 // RUN:   -analyzer-checker=core
 
-// RUN: not %clang_analyze_cc1 -std=c++14 -verify %s \
+// RUN: not %clang_analyze_cc1 -std=c++17 -verify %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-config track-conditions=false \
 // RUN:   -analyzer-config track-conditions-debug=true \
@@ -14,14 +14,14 @@
 // CHECK-INVALID-DEBUG-SAME:'track-conditions-debug', that expects
 // CHECK-INVALID-DEBUG-SAME:'track-conditions' to also be enabled
 //
-// RUN: %clang_analyze_cc1 %s -std=c++14 \
+// RUN: %clang_analyze_cc1 %s -std=c++17 \
 // RUN:   -verify=expected,tracking,debug \
 // RUN:   -analyzer-config track-conditions=true \
 // RUN:   -analyzer-config track-conditions-debug=true \
 // RUN:   -analyzer-output=text \
 // RUN:   -analyzer-checker=core
 
-// RUN: %clang_analyze_cc1 %s -std=c++14 -verify \
+// RUN: %clang_analyze_cc1 %s -std=c++17 -verify \
 // RUN:   -analyzer-output=text \
 // RUN:   -analyzer-config track-conditions=false \
 // RUN:   -analyzer-checker=core
@@ -149,14 +149,13 @@
   int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
 
   if (ConvertsToBool())
-// debug-note-re@-1^}}Tracking condition 'ConvertsToBool()'{{$
-// expected-note-re@-2^}}Assuming the condition is true{{$
-// expected-note-re@-3^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
 }
 
-} // end of namespace variable_declaration_in_condition
+} // namespace conversion_to_bool
 
 namespace note_from_different_but_not_nested_stackframe {
 
@@ -186,14 +185,13 @@
 int *getIntPtr();
 
 void storeValue(int **i) {
-  *i = getIntPtr(); // tracking-note-re^}}Value assigned to 'i', which participates in a condition later{{$
+  *i = getIntPtr();
 }
 
 int *conjurePointer() {
   int *i;
-  storeValue(&i); // tracking-note-re^}}Calling 'storeValue'{{$
-  // tracking-note-re@-1^}}Returning from 'storeValue'{{$
-  return i; // tracking-note-re^}}Returning pointer (loaded from 'i'), which participates in a condition later{{$
+  storeValue(&i);
+  return i;
 }
 
 void f(int *ptr) {
@@ -201,11 +199,8 @@
// expected-note-re@-1^}}Taking false branch{{$
 ;
   if (!conjurePointer())
-// tracking-note-re@-1^}}Calling 'conjurePointer'{{$
-// tracking-note-re@-2^}}Returning from 'conjurePointer'{{$
-// debug-note-re@-3^}}Tracking condition '!conjurePointer()'{{$
-// expected-note-re@-4^}}Assuming the condition is true{{$
-// expected-note-re@-5^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *ptr = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
 }
@@ -226,9 +221,8 @@
// expected-note-re@-1^}}Taking false branch{{$
 ;
   if (!conjurePointer())
-// debug-note-re@-1^}}Tracking condition '!conjurePointer()'{{$
-// expected-note-re@-2^}}Assuming the condition is true{{$
-// expected-note-re@-3^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *ptr = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
 }
@@ -246,9 +240,8 @@
   int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
 
   if (cast(conjure()))
-// debug-note-re@-1^}}Tra

[PATCH] D116597: [analyzer] Don't track function calls as control dependencies

[Kristóf Umann via Phabricator via cfe-commits]

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



Comment at: clang/lib/Analysis/CFG.cpp:5908-5909
 
+void CFG::dump(bool ShowColors) const { dump(LangOptions{}, ShowColors); }
+
 /// print - A simple pretty printer of a CFG that outputs to an ostream.

steakhal wrote:
> How are these `dump()` changes related?
I'll commit them in a separate patch.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1968
+static const Expr *peelOffOuterExpr(const Expr *Ex, const ExplodedNode *N) {
+
   Ex = Ex->IgnoreParenCasts();

steakhal wrote:
> extra blank line
That is intentional -- I think it makes the code more readable. Separates the 
function signature from the implementation.



Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:1036
+  x = nullptr;// expected-note {{Null pointer value stored to 'x'}}
+  if (!alwaysFalse()) // expected-note {{Taking true branch}}
+*x = 5;   // expected-warning {{Dereference of null pointer 
(loaded from variable 'x') [core.NullDereference]}}

steakhal wrote:
> What if this expression is enclosed by a logical operator such as `&&`?
For each of those operators, a different CFGBlock would be created:

```
if (A && B) 
  C;
D;

  C
/   \
  B--->
 / \
A-> D
```

This means that operands of || and && is retrievable through 
`CFGBlock::getLastCondition()`, so I shouldn't need to tear the AST apart to 
that extent. Though, I admit, you likely don't need to go very far to fool my 
implementation for realizing whether the condition boils down to a function 
call.


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

https://reviews.llvm.org/D116597

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

[PATCH] D116597: [analyzer] Don't track function calls as control dependencies

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 419132.
Szelethus marked an inline comment as done.
Szelethus added a comment.
Herald added a project: All.

Fixes according to reviewer comments.


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

https://reviews.llvm.org/D116597

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -1,10 +1,10 @@
-// RUN: %clang_analyze_cc1 %s -std=c++14 \
+// RUN: %clang_analyze_cc1 %s -std=c++17 \
 // RUN:   -verify=expected,tracking \
 // RUN:   -analyzer-config track-conditions=true \
 // RUN:   -analyzer-output=text \
 // RUN:   -analyzer-checker=core
 
-// RUN: not %clang_analyze_cc1 -std=c++14 -verify %s \
+// RUN: not %clang_analyze_cc1 -std=c++17 -verify %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-config track-conditions=false \
 // RUN:   -analyzer-config track-conditions-debug=true \
@@ -14,14 +14,14 @@
 // CHECK-INVALID-DEBUG-SAME:'track-conditions-debug', that expects
 // CHECK-INVALID-DEBUG-SAME:'track-conditions' to also be enabled
 //
-// RUN: %clang_analyze_cc1 %s -std=c++14 \
+// RUN: %clang_analyze_cc1 %s -std=c++17 \
 // RUN:   -verify=expected,tracking,debug \
 // RUN:   -analyzer-config track-conditions=true \
 // RUN:   -analyzer-config track-conditions-debug=true \
 // RUN:   -analyzer-output=text \
 // RUN:   -analyzer-checker=core
 
-// RUN: %clang_analyze_cc1 %s -std=c++14 -verify \
+// RUN: %clang_analyze_cc1 %s -std=c++17 -verify \
 // RUN:   -analyzer-output=text \
 // RUN:   -analyzer-config track-conditions=false \
 // RUN:   -analyzer-checker=core
@@ -38,7 +38,7 @@
   int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
   flag = 1;
 
-  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  foo();// TODO: Add nodes here about flag's value being invalidated.
   if (flag) // expected-note-re   ^}}Assuming 'flag' is 0{{$
 // expected-note-re@-1^}}Taking false branch{{$
 x = new int;
@@ -105,9 +105,9 @@
   foo(); // tracking-note-re^}}Calling 'foo'{{$
  // tracking-note-re@-1^}}Returning from 'foo'{{$
 
-  if (bar) // expected-note-re   ^}}Assuming 'bar' is not equal to 0{{$
-   // expected-note-re@-1^}}Taking true branch{{$
-   // debug-note-re@-2^}}Tracking condition 'bar'{{$
+  if (bar)// expected-note-re   ^}}Assuming 'bar' is not equal to 0{{$
+  // expected-note-re@-1^}}Taking true branch{{$
+  // debug-note-re@-2^}}Tracking condition 'bar'{{$
 if (flag) // expected-note-re   ^}}Assuming 'flag' is not equal to 0{{$
   // expected-note-re@-1^}}Taking true branch{{$
   // debug-note-re@-2^}}Tracking condition 'flag'{{$
@@ -149,14 +149,13 @@
   int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
 
   if (ConvertsToBool())
-// debug-note-re@-1^}}Tracking condition 'ConvertsToBool()'{{$
-// expected-note-re@-2^}}Assuming the condition is true{{$
-// expected-note-re@-3^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
 }
 
-} // end of namespace variable_declaration_in_condition
+} // namespace conversion_to_bool
 
 namespace note_from_different_but_not_nested_stackframe {
 
@@ -186,14 +185,13 @@
 int *getIntPtr();
 
 void storeValue(int **i) {
-  *i = getIntPtr(); // tracking-note-re^}}Value assigned to 'i', which participates in a condition later{{$
+  *i = getIntPtr();
 }
 
 int *conjurePointer() {
   int *i;
-  storeValue(&i); // tracking-note-re^}}Calling 'storeValue'{{$
-  // tracking-note-re@-1^}}Returning from 'storeValue'{{$
-  return i; // tracking-note-re^}}Returning pointer (loaded from 'i'), which participates in a condition later{{$
+  storeValue(&i);
+  return i;
 }
 
 void f(int *ptr) {
@@ -201,11 +199,8 @@
// expected-note-re@-1^}}Taking false branch{{$
 ;
   if (!conjurePointer())
-// tracking-note-re@-1^}}Calling 'conjurePointer'{{$
-// tracking-note-re@-2^}}Returning from 'conjurePointer'{{$
-// debug-note-re@-3^}}Tracking condition '!conjurePointer()'{{$
-// expected-note-re@-4^}}Assuming the condition is true{{$
-// expected-note-

[PATCH] D120992: [analyzer] ReverseNull: New checker to warn for pointer value conditions, if the pointer value is unconditionally non-null

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus abandoned this revision.
Szelethus added a comment.

Very well :) Let's abandon this in its current state, I share this sentiment:

In D120992#3368118 , @NoQ wrote:

> What I'm trying to say here is, I honestly think re-doing it as CFG-based 
> should be relatively easy. We couldn't make any progress at all without those 
> state splits but in this case it's much less of a fundamental issue so I 
> think it's not worth it to hard-commit to a flawed solution from the start.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120992

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

[PATCH] D122244: [analyzer] Turn missing tablegen doc entry of a checker into fatal error

[Kristóf Umann via Phabricator via cfe-commits]

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

LGTM! You did check whether a missing doc field will actually trigger this 
error, right?




Comment at: clang/utils/TableGen/ClangSACheckersEmitter.cpp:82
+PrintFatalError(R.getLoc(),
+"missing Documentation for " + getCheckerFullName(&R));
+

We might as well give an extra hand, and state that a `Documentation<> field` 
is missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122244

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

[PATCH] D122285: [analyzer] Add path note tags to standard library function summaries.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

LGTM on my end, this is awesome!

In D122285#3401754 , @steakhal wrote:

>> The notes are prunable, i.e. they won't bring-in entire stack frames worth 
>> of notes just because they're there, but they will be always visible 
>> regardless of whether the value is of interest to the bug report. I think 
>> this is debatable, the arguably better solution is to make them non-prunable 
>> but conditional to the value being tracked back to the call, which would 
>> probably need a better tracking infrastructure.
>
> I was thinking of passing a lambda and doing the rest there. We could have 
> lambda factories to make it less cumbersome to define - and also reuse code.

Could you think of a scenario where a lambda would be required? It sure is more 
general, but I don't immediately see the gain.


Repository:
  rC Clang

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

https://reviews.llvm.org/D122285

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

[PATCH] D121197: [clang][dataflow] Add analysis that detects unsafe accesses to optionals

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Seems like all new files are missing the header blurb about the licence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121197

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

[PATCH] D121387: [analyzer] ClangSA should tablegen doc urls refering to the main doc page

[Kristóf Umann via Phabricator via cfe-commits]

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

Nice!




Comment at: clang/utils/TableGen/ClangSACheckersEmitter.cpp:27
 
-static std::string getPackageFullName(const Record *R);
+static std::string getPackageFullName(const Record *R, StringRef Sep = ".");
 

~~Do you foresee a change in the default separator? I think an eventual shift 
away from packages is more likely (see 
https://discourse.llvm.org/t/analyzer-refactoring-analyzeroptions/50160/5). ~~

~~I this an unnecessary complexity, but its not a hill I will die on.~~

edit: I just realized that you need this for the url, so never mind. I grave 
dug a 3 yr old topic on discourse because of it :^) Oops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121387

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

[PATCH] D120992: [analyzer] ReverseNull: New checker to warn for pointer value conditions, if the pointer value is unconditionally non-null

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D120992#3364804 , @NoQ wrote:

> This check checks must-properties/all-paths properties. This has to be a data 
> flow / CFG-based warning. I don't think there's a way around.

Oof. I concede on that. Do you think there is any point in turning this into an 
optin checker? Or the philosophy should be that this is just way too wild?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120992

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

[PATCH] D120992: [analyzer] ReverseNull: New checker to warn for pointer value conditions, if the pointer value is unconditionally non-null

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp:166
+/// child is a sink node.
+static bool unconditionallyLeadsHere(const ExplodedNode *N) {
+  size_t NonSinkNodeCount = llvm::count_if(

xazax.hun wrote:
> xazax.hun wrote:
> > Consider the following code snippet:
> > ```
> > void f(int *p, bool b)
> > {
> >   if (b) {
> > *p = 4;
> >   }
> >   if (p) {
> >...
> >   }
> > }
> > ```
> > 
> > I suspect that we would get a warning for the code above. I think warning 
> > on the code above might be reasonable (the values of `b` and `p` might be 
> > correlated but in some cases the analyzer has no way to know this, probably 
> > some assertions could make the code clearer in that case).
> > 
> > My problem is with the wording of the error message.
> > The warning `Pointer is unconditionally non-null here` on the null check is 
> > not true for the code above.
> Also, if the check would warn for the code snippet above, the note "suggest 
> moving the condition here" would also be incorrect.
What if we demand that the the `CFGBlock` of the dereference must dominate the 
`CFGBlock` of the condition point?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120992

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

[PATCH] D120992: [analyzer] ReverseNull: New checker to warn for pointer value conditions, if the pointer value is unconditionally non-null

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, steakhal, xazax.hun, balazske, martong.
Szelethus added a project: clang.
Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, mgorny.
Herald added a project: All.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.

Loosely tied to EXP34-C.

Suppose we do a pointer operation, such as dereferencing, or passing it to a 
function whose parameter is attributed with non null (like `_Nonnull` or 
`clang::nonnull`). Then, we write an if branch whose condition is this very 
pointer.

  void tp1(int *p) {
*p = 5;
// expected-note@-1{{Pointer assumed non-null here}}
// expected-note@-2{{Consider moving the condition here}}
if (p)
  // expected-note@-1{{Pointer is unconditionally non-null here}}
  // expected-warning@-2{{Pointer is unconditionally non-null here 
[alpha.core.ReverseNull]}}
  return;
  }

This is a sign of code smell: We either inteded to place the condition before 
the operation that already expects the pointer to be non-null, or there are 
some other high level issues in our code. The patch adds a new checker to find 
and warn for similar cases.

One of the fundamental ideas here is that this is an interprocedural checker. 
We want to ensure that the derefence (or similar operation) unconditionally 
leads to the condition without interleaving assignments. This sounds a lot like 
a dataflow analysis! Indeed, it would be great if we had a dataflow framework 
already in the bag, and although there is a work on it (a lot of patches can be 
seen on @ymandel's profile: https://reviews.llvm.org/people/revisions/17749/), 
its not quite there yet.

Another thought could be, that this checker is (somewhat) redundant with 
DeadCodeChecker, because this checker also finds dead code, but restricted to 
pointers. This wasn't a deterrent in developing this alpha checker, because if 
an 'else' branch is not present, the dead code will not be found (despite the 
sode smell being there), and restricting ourselves to pointer analysis takes a 
lot of weight off from our constraint solvers.

I've ran the checker on a lot of projects, and the results so far look 
promising, and I'm looking forward to publishing them ASAP.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120992

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/NullPtrInterferenceChecker.cpp
  clang/test/Analysis/NSString.m
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/null-pointer-interference.c
  clang/test/Analysis/null-pointer-interference.m

Index: clang/test/Analysis/null-pointer-interference.m
===
--- /dev/null
+++ clang/test/Analysis/null-pointer-interference.m
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -triple i386-apple-darwin10 -Wno-objc-root-class -fblocks \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=osx.cocoa.NilArg \
+// RUN:   -analyzer-checker=osx.cocoa.RetainCount \
+// RUN:   -analyzer-checker=alpha.core.ReverseNull
+
+#include "Inputs/system-header-simulator-objc.h"
+
+typedef const struct __CFDictionary * CFDictionaryRef;
+const void *CFDictionaryGetValue(CFDictionaryRef theDict, const void *key);
+
+// From NSString.m:
+NSString* f11(CFDictionaryRef dict, const char* key) {
+  NSString* s = (NSString*) CFDictionaryGetValue(dict, key);
+  // expected-note@-1{{'s' initialized here}}
+  [s retain];
+  // expected-note@-1{{Pointer assumed non-null here}}
+  // expected-note@-2{{Consider moving the condition here}}
+  if (s) {
+// expected-warning@-1{{Pointer is unconditionally non-null here [alpha.core.ReverseNull]}}
+// expected-note@-2{{Pointer is unconditionally non-null here}}
+[s release];
+  }
+  return 0;
+}
Index: clang/test/Analysis/null-pointer-interference.c
===
--- /dev/null
+++ clang/test/Analysis/null-pointer-interference.c
@@ -0,0 +1,92 @@
+// RUN: %clang_analyze_cc1 -verify %s -analyzer-output=text\
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-checker=alpha.core.ReverseNull \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:ModelPOSIX=true \
+// RUN:   -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \
+// RUN:   2>&1 | FileCheck %s
+
+void clang_analyzer_warnIfReached();
+//===--===//
+// True positive test cases.
+//===-

[PATCH] D118880: [analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd832078904c6: [analyzer] Improve 
NoOwnershipChangeVisitor's understanding of deallocators (authored by 
Szelethus).
Herald added a project: All.

Changed prior to commit:
  https://reviews.llvm.org/D118880?vs=407121&id=412644#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118880

Files:
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp

Index: clang/test/Analysis/NewDeleteLeaks.cpp
===
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -35,7 +35,9 @@
 
 } // namespace memory_allocated_in_fn_call
 
-namespace memory_passed_to_fn_call {
+// Realize that sink() intends to deallocate memory, assume that it should've
+// taken care of the leaked object as well.
+namespace memory_passed_to_fn_call_delete {
 
 void sink(int *P) {
   if (coin()) // ownership-note {{Assuming the condition is false}}
@@ -50,7 +52,41 @@
 } // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential leak}}
 
-} // namespace memory_passed_to_fn_call
+} // namespace memory_passed_to_fn_call_delete
+
+namespace memory_passed_to_fn_call_free {
+
+void sink(int *P) {
+  if (coin()) // ownership-note {{Assuming the condition is false}}
+  // ownership-note@-1 {{Taking false branch}}
+free(P);
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}}
+  sink(ptr); // ownership-note {{Calling 'sink'}}
+ // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call_free
+
+// Function pointers cannot be resolved syntactically.
+namespace memory_passed_to_fn_call_free_through_fn_ptr {
+void (*freeFn)(void *) = free;
+
+void sink(int *P) {
+  if (coin())
+freeFn(P);
+}
+
+void foo() {
+  int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}}
+  sink(ptr);
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call_free_through_fn_ptr
 
 namespace memory_shared_with_ptr_of_shorter_lifetime {
 
@@ -123,6 +159,24 @@
 
 } // namespace memory_passed_into_fn_that_doesnt_intend_to_free
 
+namespace memory_passed_into_fn_that_doesnt_intend_to_free2 {
+
+void bar();
+
+void sink(int *P) {
+  // Correctly realize that calling bar() doesn't mean that this function would
+  // like to deallocate anything.
+  bar();
+}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_into_fn_that_doesnt_intend_to_free2
+
 namespace refkind_from_unoallocated_to_allocated {
 
 // RefKind of the symbol changed from nothing to Allocated. We don't want to
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -398,6 +398,9 @@
   };
 
   bool isFreeingCall(const CallEvent &Call) const;
+  static bool isFreeingOwnershipAttrCall(const FunctionDecl *Func);
+
+  friend class NoOwnershipChangeVisitor;
 
   CallDescriptionMap AllocatingMemFnMap{
   {{"alloca", 1}, &MallocChecker::checkAlloca},
@@ -742,7 +745,9 @@
 
 namespace {
 class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
+  // The symbol whose (lack of) ownership change we are interested in.
   SymbolRef Sym;
+  const MallocChecker &Checker;
   using OwnerSet = llvm::SmallPtrSet;
 
   // Collect which entities point to the allocated memory, and could be
@@ -794,6 +799,29 @@
 return "";
   }
 
+  /// Syntactically checks whether the callee is a deallocating function. Since
+  /// we have no path-sensitive information on this call (we would need a
+  /// CallEvent instead of a CallExpr for that), its possible that a
+  /// deallocation function was called indirectly through a function pointer,
+  /// but we are not able to tell, so this is a best effort analysis.
+  /// See namespace `memory_passed_to_fn_call_free_through_fn_ptr` in
+  /// clang/test/Analysis/NewDeleteLeaks.cpp.
+  bool isFreeingCallAsWritten(const CallExpr &Call) const {
+if (Checker.FreeingMemFnMap.lookupAsWritten(Call) ||

[PATCH] D119004: [NFC][analyzer] Allow CallDescriptions to be matched with CallExprs

[Kristóf Umann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG32ac21d04909: [NFC][analyzer] Allow CallDescriptions to be 
matched with CallExprs (authored by Szelethus).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119004

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
  clang/lib/StaticAnalyzer/Core/CallDescription.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -6,11 +6,18 @@
 //
 //===--===//
 
+#include "CheckerRegistration.h"
 #include "Reusables.h"
 
 #include "clang/AST/ExprCXX.h"
+#include "clang/Analysis/PathDiagnostic.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
 #include 
@@ -543,6 +550,77 @@
   }
 }
 
+//===--===//
+// Testing through a checker interface.
+//
+// Above, the static analyzer isn't run properly, only the bare minimum to
+// create CallEvents. This causes CallEvents through function pointers to not
+// refer to the pointee function, but this works fine if we run
+// AnalysisASTConsumer.
+//===--===//
+
+class CallDescChecker
+: public Checker> {
+  CallDescriptionSet Set = {{"bar", 0}};
+
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+if (Set.contains(Call)) {
+  C.getBugReporter().EmitBasicReport(
+  Call.getDecl(), this, "CallEvent match", categories::LogicError,
+  "CallEvent match",
+  PathDiagnosticLocation{Call.getDecl(), C.getSourceManager()});
+}
+  }
+
+  void checkPreStmt(const CallExpr *CE, CheckerContext &C) const {
+if (Set.containsAsWritten(*CE)) {
+  C.getBugReporter().EmitBasicReport(
+  CE->getCalleeDecl(), this, "CallExpr match", categories::LogicError,
+  "CallExpr match",
+  PathDiagnosticLocation{CE->getCalleeDecl(), C.getSourceManager()});
+}
+  }
+};
+
+void addCallDescChecker(AnalysisASTConsumer &AnalysisConsumer,
+AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.CallDescChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+Registry.addChecker("test.CallDescChecker", "Description",
+ "");
+  });
+}
+
+TEST(CallDescription, CheckCallExprMatching) {
+  // Imprecise matching shouldn't catch the call to bar, because its obscured
+  // by a function pointer.
+  constexpr StringRef FnPtrCode = R"code(
+void bar();
+void foo() {
+  void (*fnptr)() = bar;
+  fnptr();
+})code";
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode(FnPtrCode.str(), Diags,
+   /*OnlyEmitWarnings*/ true));
+  EXPECT_EQ("test.CallDescChecker: CallEvent match\n", Diags);
+
+  // This should be caught properly by imprecise matching, as the call is done
+  // purely through syntactic means.
+  constexpr StringRef Code = R"code(
+void bar();
+void foo() {
+  bar();
+})code";
+  Diags.clear();
+  EXPECT_TRUE(runCheckerOnCode(Code.str(), Diags,
+   /*OnlyEmitWarnings*/ true));
+  EXPECT_EQ("test.CallDescChecker: CallEvent match\n"
+"test.CallDescChecker: CallExpr match\n",
+Diags);
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Core/CallDescription.cpp
===
--- clang/lib/StaticAnalyzer/Core/CallDescription.cpp
+++ clang/lib/StaticAnalyzer/Core/CallDescription.cpp
@@ -13,6 +13,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/AST/Decl.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -61,15 +62,32 @@
   if (!FD)
 return false;
 
+  

[PATCH] D120325: [analyzer] Don't crash if the analyzer-constraint is set to Z3, but llvm is not built with it

[Kristóf Umann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5048a58a6792: [analyzer] Don't crash if the 
analyzer-constraint is set to Z3, but llvm is not… (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D120325?vs=410526&id=411418#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120325

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Analysis/missing-z3-nocrash.c
  clang/test/lit.cfg.py


Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -100,6 +100,8 @@
 
 if config.clang_staticanalyzer_z3:
 config.available_features.add('z3')
+else:
+config.available_features.add('no-z3')
 
 check_analyzer_fixit_path = os.path.join(
 config.test_source_root, "Analysis", "check-analyzer-fixit.py")
Index: clang/test/Analysis/missing-z3-nocrash.c
===
--- /dev/null
+++ clang/test/Analysis/missing-z3-nocrash.c
@@ -0,0 +1,5 @@
+// RUN: not %clang_analyze_cc1 -analyzer-constraints=z3 %s 2>&1 | FileCheck %s
+// REQUIRES: no-z3
+
+// CHECK: error: analyzer constraint manager 'z3' is only available if LLVM
+// CHECK: was built with -DLLVM_ENABLE_Z3_SOLVER=ON
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -915,6 +915,11 @@
   Diags.Report(diag::err_drv_invalid_value)
 << A->getAsString(Args) << Name;
 } else {
+#ifndef LLVM_WITH_Z3
+  if (Value == AnalysisConstraints::Z3ConstraintsModel) {
+Diags.Report(diag::err_analyzer_not_built_with_z3);
+  }
+#endif // LLVM_WITH_Z3
   Opts.AnalysisConstraintsOpt = Value;
 }
   }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -438,6 +438,9 @@
   "invalid input for checker option '%0', that expects %1">;
 def err_analyzer_checker_incompatible_analyzer_option : Error<
   "checker cannot be enabled with analyzer option '%0' == %1">;
+def err_analyzer_not_built_with_z3 : Error<
+  "analyzer constraint manager 'z3' is only available if LLVM was built with "
+  "-DLLVM_ENABLE_Z3_SOLVER=ON">;
 
 def warn_drv_needs_hvx : Warning<
   "%0 requires HVX, use -mhvx/-mhvx= to enable it">,


Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -100,6 +100,8 @@
 
 if config.clang_staticanalyzer_z3:
 config.available_features.add('z3')
+else:
+config.available_features.add('no-z3')
 
 check_analyzer_fixit_path = os.path.join(
 config.test_source_root, "Analysis", "check-analyzer-fixit.py")
Index: clang/test/Analysis/missing-z3-nocrash.c
===
--- /dev/null
+++ clang/test/Analysis/missing-z3-nocrash.c
@@ -0,0 +1,5 @@
+// RUN: not %clang_analyze_cc1 -analyzer-constraints=z3 %s 2>&1 | FileCheck %s
+// REQUIRES: no-z3
+
+// CHECK: error: analyzer constraint manager 'z3' is only available if LLVM
+// CHECK: was built with -DLLVM_ENABLE_Z3_SOLVER=ON
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -915,6 +915,11 @@
   Diags.Report(diag::err_drv_invalid_value)
 << A->getAsString(Args) << Name;
 } else {
+#ifndef LLVM_WITH_Z3
+  if (Value == AnalysisConstraints::Z3ConstraintsModel) {
+Diags.Report(diag::err_analyzer_not_built_with_z3);
+  }
+#endif // LLVM_WITH_Z3
   Opts.AnalysisConstraintsOpt = Value;
 }
   }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -438,6 +438,9 @@
   "invalid input for checker option '%0', that expects %1">;
 def err_analyzer_checker_incompatible_analyzer_option : Error<
   "checker cannot be enabled with analyzer option '%0' == %1">;
+def err_analyzer_not_built_with_z3 : Error<
+  "analyzer constraint manager 'z3' is only available if LLVM was built with "
+  "-DLLVM_ENABLE_Z3_SOLVER=ON">;
 
 def warn_drv_needs_hvx : Warning<
   "%0 requires HVX, use -mhvx/-mhvx= to enable it">,
___
cfe-commits mailing list
c

[PATCH] D119004: [NFC][analyzer] Allow CallDescriptions to be matched with CallExprs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h:102-103
 
+  /// Returns true if the CallEvent is a call to a function that matches
+  /// the CallDescription.
+  ///

steakhal wrote:
> NoQ wrote:
> > 
> ping
This is actually the `CallEvent` variant, I corrected the `CallExpr` one :)



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h:111
 
-  /// \copydoc clang::ento::matchesAny(const CallEvent &, const 
CallDescription &)
+  /// \copydoc clang::ento::CallDescription::matchesAny(const CallEvent &, 
const CallDescription &)
   template 

steakhal wrote:
> I think it's a free function. I know that copydoc did not work for this 
> example.
> Are you sure adding the `::CallDescription` fixes the doc comment?
Yes! Its still in `CallDescription`s "namespace", as its defined in-class. I 
ran `doxygen-clang` and can confirm that this works, but only without line 
breaks (or at least I haven't figured out how to do it without it, not even 
with backslash)



Comment at: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp:501
+//===--===//
+// Testing through a checker interface.
+//

steakhal wrote:
> You could have define `ResultMap` ad a virtual base class, which would be 
> implemented by two different classes. One of which would use the `asWritten` 
> lookups, etc.
> You could `make_unique` of the required one and inject it to the `Action`.
> 
> That way those tests would look just the same as the previous ones.
My rationale was that reusing an already existing machinery which is already 
used in many of the static analyzer unit tests is far friendlier for beginners. 
I think fracturing the file specific stuff here would increase the barrier of 
entry for very little gain.

One of the things I like a lot on unit tests is that they demonstrate on a 
small scale a part of the analyzer's core machinery. I think dividing this up 
would go against that as well.


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

https://reviews.llvm.org/D119004

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

[PATCH] D119004: [NFC][analyzer] Allow CallDescriptions to be matched with CallExprs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 410833.
Szelethus marked 5 inline comments as done.
Szelethus added a comment.

Remove a newline.


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

https://reviews.llvm.org/D119004

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
  clang/lib/StaticAnalyzer/Core/CallDescription.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -6,11 +6,18 @@
 //
 //===--===//
 
+#include "CheckerRegistration.h"
 #include "Reusables.h"
 
 #include "clang/AST/ExprCXX.h"
+#include "clang/Analysis/PathDiagnostic.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
 #include 
@@ -543,6 +550,77 @@
   }
 }
 
+//===--===//
+// Testing through a checker interface.
+//
+// Above, the static analyzer isn't run properly, only the bare minimum to
+// create CallEvents. This causes CallEvents through function pointers to not
+// refer to the pointee function, but this works fine if we run
+// AnalysisASTConsumer.
+//===--===//
+
+class CallDescChecker
+: public Checker> {
+  CallDescriptionSet Set = {{"bar", 0}};
+
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+if (Set.contains(Call)) {
+  C.getBugReporter().EmitBasicReport(
+  Call.getDecl(), this, "CallEvent match", categories::LogicError,
+  "CallEvent match",
+  PathDiagnosticLocation{Call.getDecl(), C.getSourceManager()});
+}
+  }
+
+  void checkPreStmt(const CallExpr *CE, CheckerContext &C) const {
+if (Set.containsAsWritten(*CE)) {
+  C.getBugReporter().EmitBasicReport(
+  CE->getCalleeDecl(), this, "CallExpr match", categories::LogicError,
+  "CallExpr match",
+  PathDiagnosticLocation{CE->getCalleeDecl(), C.getSourceManager()});
+}
+  }
+};
+
+void addCallDescChecker(AnalysisASTConsumer &AnalysisConsumer,
+AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.CallDescChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+Registry.addChecker("test.CallDescChecker", "Description",
+ "");
+  });
+}
+
+TEST(CallDescription, CheckCallExprMatching) {
+  // Imprecise matching shouldn't catch the call to bar, because its obscured
+  // by a function pointer.
+  constexpr StringRef FnPtrCode = R"code(
+void bar();
+void foo() {
+  void (*fnptr)() = bar;
+  fnptr();
+})code";
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode(FnPtrCode.str(), Diags,
+   /*OnlyEmitWarnings*/ true));
+  EXPECT_EQ("test.CallDescChecker: CallEvent match\n", Diags);
+
+  // This should be caught properly by imprecise matching, as the call is done
+  // purely through syntactic means.
+  constexpr StringRef Code = R"code(
+void bar();
+void foo() {
+  bar();
+})code";
+  Diags.clear();
+  EXPECT_TRUE(runCheckerOnCode(Code.str(), Diags,
+   /*OnlyEmitWarnings*/ true));
+  EXPECT_EQ("test.CallDescChecker: CallEvent match\n"
+"test.CallDescChecker: CallExpr match\n",
+Diags);
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Core/CallDescription.cpp
===
--- clang/lib/StaticAnalyzer/Core/CallDescription.cpp
+++ clang/lib/StaticAnalyzer/Core/CallDescription.cpp
@@ -13,6 +13,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/AST/Decl.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -61,15 +62,32 @@
   if (!FD)
 return false;
 
+  return matchesImpl(FD, Call.getNumArgs(), Call.parameters().size());
+}
+
+bool ento::CallDescription::matchesAsWritten(const CallExpr &CE) const {
+  const 

[PATCH] D119128: [analyzer] Fix taint propagation by remembering to the location context

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Can we reopen this if the code is not upstream at this time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119128

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

[PATCH] D120325: [analyzer] Don't crash if the analyzer-constraint is set to Z3, but llvm is not built with it

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, dkrupp, steakhal, martong, balazske.
Szelethus added a project: clang.
Herald added subscribers: ASDenysPetrov, gamesh411, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.

Exactly what it says on the tin! We had a nasty crash with the following 
incovation:

  $ clang --analyze -Xclang -analyzer-constraints=z3 test.c
  fatal error: error in backend: LLVM was not compiled with Z3 support, rebuild 
with -DLLVM_ENABLE_Z3_SOLVER=ON
  ...  ...


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120325

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Analysis/missing-z3-nocrash.c
  clang/test/lit.cfg.py


Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -100,6 +100,8 @@
 
 if config.clang_staticanalyzer_z3:
 config.available_features.add('z3')
+else:
+config.available_features.add('no-z3')
 
 check_analyzer_fixit_path = os.path.join(
 config.test_source_root, "Analysis", "check-analyzer-fixit.py")
Index: clang/test/Analysis/missing-z3-nocrash.c
===
--- /dev/null
+++ clang/test/Analysis/missing-z3-nocrash.c
@@ -0,0 +1,5 @@
+// RUN: not %clang_analyze_cc1 -analyzer-constraints=z3 %s 2>&1 | FileCheck %s
+// REQUIRES: no-z3
+
+// CHECK: error: analyzer constraint manager 'z3' is only available if LLVM
+// CHECK: was built with -DLLVM_ENABLE_Z3_SOLVER=ON
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -915,6 +915,11 @@
   Diags.Report(diag::err_drv_invalid_value)
 << A->getAsString(Args) << Name;
 } else {
+#if !LLVM_WITH_Z3
+  if (Value == AnalysisConstraints::Z3ConstraintsModel) {
+Diags.Report(diag::err_analyzer_not_built_with_z3);
+  }
+#endif // LLVM_WITH_Z3
   Opts.AnalysisConstraintsOpt = Value;
 }
   }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -441,6 +441,9 @@
   "invalid input for checker option '%0', that expects %1">;
 def err_analyzer_checker_incompatible_analyzer_option : Error<
   "checker cannot be enabled with analyzer option '%0' == %1">;
+def err_analyzer_not_built_with_z3 : Error<
+  "analyzer constraint manager 'z3' is only available if LLVM was built with "
+  "-DLLVM_ENABLE_Z3_SOLVER=ON">;
 
 def warn_drv_needs_hvx : Warning<
   "%0 requires HVX, use -mhvx/-mhvx= to enable it">,


Index: clang/test/lit.cfg.py
===
--- clang/test/lit.cfg.py
+++ clang/test/lit.cfg.py
@@ -100,6 +100,8 @@
 
 if config.clang_staticanalyzer_z3:
 config.available_features.add('z3')
+else:
+config.available_features.add('no-z3')
 
 check_analyzer_fixit_path = os.path.join(
 config.test_source_root, "Analysis", "check-analyzer-fixit.py")
Index: clang/test/Analysis/missing-z3-nocrash.c
===
--- /dev/null
+++ clang/test/Analysis/missing-z3-nocrash.c
@@ -0,0 +1,5 @@
+// RUN: not %clang_analyze_cc1 -analyzer-constraints=z3 %s 2>&1 | FileCheck %s
+// REQUIRES: no-z3
+
+// CHECK: error: analyzer constraint manager 'z3' is only available if LLVM
+// CHECK: was built with -DLLVM_ENABLE_Z3_SOLVER=ON
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -915,6 +915,11 @@
   Diags.Report(diag::err_drv_invalid_value)
 << A->getAsString(Args) << Name;
 } else {
+#if !LLVM_WITH_Z3
+  if (Value == AnalysisConstraints::Z3ConstraintsModel) {
+Diags.Report(diag::err_analyzer_not_built_with_z3);
+  }
+#endif // LLVM_WITH_Z3
   Opts.AnalysisConstraintsOpt = Value;
 }
   }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -441,6 +441,9 @@
   "invalid input for checker option '%0', that expects %1">;
 def err_analyzer_checker_incompatible_analyzer_option : Error<
   "checker cannot be enabled with analyzer option '%0' == %1">;
+def err_analyzer_not_built_with_z3 : Error<
+  "analyzer constraint manager 'z3' is only available if LLVM was buil

[PATCH] D118880: [analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/test/Analysis/NewDeleteLeaks.cpp:152
+  // like to deallocate anything.
+  bar();
+}

steakhal wrote:
> I guess we would not have the warning if `bar` would accept the pointer `P`, 
> since that way it would escape.
> Am I correct?
That is correct, but as an earlier todo states, we don't currently employ any 
heuristics to guess whether `bar` was responsible for storing `P` (as opposed 
to deallocating it, which is what this patch is about), but failed to.


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

https://reviews.llvm.org/D118880

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

[PATCH] D118880: [analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 407121.
Szelethus marked 8 inline comments as done.
Szelethus added a comment.

Fixes according to reviewer comments.


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

https://reviews.llvm.org/D118880

Files:
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp

Index: clang/test/Analysis/NewDeleteLeaks.cpp
===
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -35,7 +35,9 @@
 
 } // namespace memory_allocated_in_fn_call
 
-namespace memory_passed_to_fn_call {
+// Realize that sink() intends to deallocate memory, assume that it should've
+// taken care of the leaked object as well.
+namespace memory_passed_to_fn_call_delete {
 
 void sink(int *P) {
   if (coin()) // ownership-note {{Assuming the condition is false}}
@@ -50,7 +52,41 @@
 } // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential leak}}
 
-} // namespace memory_passed_to_fn_call
+} // namespace memory_passed_to_fn_call_delete
+
+namespace memory_passed_to_fn_call_free {
+
+void sink(int *P) {
+  if (coin()) // ownership-note {{Assuming the condition is false}}
+  // ownership-note@-1 {{Taking false branch}}
+free(P);
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}}
+  sink(ptr); // ownership-note {{Calling 'sink'}}
+ // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call_free
+
+// Function pointers cannot be resolved syntactically.
+namespace memory_passed_to_fn_call_free_through_fn_ptr {
+void (*freeFn)(void *) = free;
+
+void sink(int *P) {
+  if (coin())
+freeFn(P);
+}
+
+void foo() {
+  int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}}
+  sink(ptr);
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call_free_through_fn_ptr
 
 namespace memory_shared_with_ptr_of_shorter_lifetime {
 
@@ -123,6 +159,24 @@
 
 } // namespace memory_passed_into_fn_that_doesnt_intend_to_free
 
+namespace memory_passed_into_fn_that_doesnt_intend_to_free2 {
+
+void bar();
+
+void sink(int *P) {
+  // Correctly realize that calling bar() doesn't mean that this function would
+  // like to deallocate anything.
+  bar();
+}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_into_fn_that_doesnt_intend_to_free2
+
 namespace refkind_from_unoallocated_to_allocated {
 
 // RefKind of the symbol changed from nothing to Allocated. We don't want to
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -398,6 +398,9 @@
   };
 
   bool isFreeingCall(const CallEvent &Call) const;
+  static bool isFreeingOwnershipAttrCall(const FunctionDecl *Func);
+
+  friend class NoOwnershipChangeVisitor;
 
   CallDescriptionMap AllocatingMemFnMap{
   {{"alloca", 1}, &MallocChecker::checkAlloca},
@@ -742,7 +745,9 @@
 
 namespace {
 class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
+  // The symbol whose (lack of) ownership change we are interested in.
   SymbolRef Sym;
+  const MallocChecker &Checker;
   using OwnerSet = llvm::SmallPtrSet;
 
   // Collect which entities point to the allocated memory, and could be
@@ -794,18 +799,50 @@
 return "";
   }
 
+  /// Syntactically checks whether the callee is a deallocating function. Since
+  /// we have no path-sensitive information on this call (we would need a
+  /// CallEvent instead of a CallExpr for that), its possible that a
+  /// deallocation function was called indirectly through a function pointer,
+  /// but we are not able to tell, so this is a best effort analysis.
+  /// See namespace `memory_passed_to_fn_call_free_through_fn_ptr` in
+  /// clang/test/Analysis/NewDeleteLeaks.cpp.
+  bool isFreeingCallAsWritten(const CallExpr &Call) const {
+if (Checker.FreeingMemFnMap.lookupAsWritten(Call) ||
+Checker.ReallocatingMemFnMap.lookupAsWritten(Call))
+  return true;
+
+if (const auto *Func =
+llvm::dyn_cast_or_null(Call.getCalleeDecl()))
+  return MallocChecker::isF

[PATCH] D119004: [NFC][analyzer] Allow CallDescriptions to be matched with CallExprs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 407106.
Szelethus added a comment.

- Rename from `.*Imprecise` to `.*AsWritten`.
- Copy comments to relevant functions.


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

https://reviews.llvm.org/D119004

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
  clang/lib/StaticAnalyzer/Core/CallDescription.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -6,11 +6,18 @@
 //
 //===--===//
 
+#include "CheckerRegistration.h"
 #include "Reusables.h"
 
 #include "clang/AST/ExprCXX.h"
+#include "clang/Analysis/PathDiagnostic.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
 #include 
@@ -40,6 +47,7 @@
 // If it's a function we expected to find, remember that we've found it.
 if (Result && *Result)
   ++Found;
+
 return Result;
   }
 
@@ -489,6 +497,77 @@
   "}"));
 }
 
+//===--===//
+// Testing through a checker interface.
+//
+// Above, the static analyzer isn't run properly, only the bare minimum to
+// create CallEvents. This causes CallEvents through function pointers to not
+// refer to the pointee function, but this works fine if we run
+// AnalysisASTConsumer.
+//===--===//
+
+class CallDescChecker
+: public Checker> {
+  CallDescriptionSet Set = {{"bar", 0}};
+
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+if (Set.contains(Call)) {
+  C.getBugReporter().EmitBasicReport(
+  Call.getDecl(), this, "CallEvent match", categories::LogicError,
+  "CallEvent match",
+  PathDiagnosticLocation{Call.getDecl(), C.getSourceManager()});
+}
+  }
+
+  void checkPreStmt(const CallExpr *CE, CheckerContext &C) const {
+if (Set.containsAsWritten(*CE)) {
+  C.getBugReporter().EmitBasicReport(
+  CE->getCalleeDecl(), this, "CallExpr match", categories::LogicError,
+  "CallExpr match",
+  PathDiagnosticLocation{CE->getCalleeDecl(), C.getSourceManager()});
+}
+  }
+};
+
+void addCallDescChecker(AnalysisASTConsumer &AnalysisConsumer,
+AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.CallDescChecker", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+Registry.addChecker("test.CallDescChecker", "Description",
+ "");
+  });
+}
+
+TEST(CallDescription, CheckCallExprMatching) {
+  // Imprecise matching shouldn't catch the call to bar, because its obscured
+  // by a function pointer.
+  constexpr StringRef FnPtrCode = R"code(
+void bar();
+void foo() {
+  void (*fnptr)() = bar;
+  fnptr();
+})code";
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode(FnPtrCode.str(), Diags,
+   /*OnlyEmitWarnings*/ true));
+  EXPECT_EQ("test.CallDescChecker: CallEvent match\n", Diags);
+
+  // This should be caught properly by imprecise matching, as the call is done
+  // purely through syntactic means.
+  constexpr StringRef Code = R"code(
+void bar();
+void foo() {
+  bar();
+})code";
+  Diags.clear();
+  EXPECT_TRUE(runCheckerOnCode(Code.str(), Diags,
+   /*OnlyEmitWarnings*/ true));
+  EXPECT_EQ("test.CallDescChecker: CallEvent match\n"
+"test.CallDescChecker: CallExpr match\n",
+Diags);
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Core/CallDescription.cpp
===
--- clang/lib/StaticAnalyzer/Core/CallDescription.cpp
+++ clang/lib/StaticAnalyzer/Core/CallDescription.cpp
@@ -13,6 +13,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/AST/Decl.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -61,15 +6

[PATCH] D119004: [NFC][analyzer] Allow CallDescriptions to be matched with CallExprs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Now that I remember, the ever so slightly different overloads of 
`ProgramState::getSVal` is a prime example I think. I always percieved that I 
have the means to invoke several of them at any point, but I never really knew 
which one. Though, to be fair, they were not documented particularly well (at 
least as I remember it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119004

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

[PATCH] D119004: [NFC][analyzer] Allow CallDescriptions to be matched with CallExprs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D119004#3297025 , @steakhal wrote:

> I strongly belive that this should be an overload to the existing 'matches' 
> API. Maybe add a comment that prefer the other overload if can. But having an 
> overload for that alread implies this anyway.

I somewhat disagree. `CallDescription` is one of the interfaces that newcomers 
come by rather fast, and a descriptive name would be a nice piece of guidance. 
I am not sure what can be gained by turning this to an overload.

> That being said, digging out a callexpr from a CallEvent and calling the 
> callexpr overload seems to be too artifical to me to worry about.

Well, a tiny bit more is happening than that with the argument count -- what do 
you mean with this statement exactly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119004

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

[PATCH] D118880: [analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 405979.
Szelethus edited the summary of this revision.
Szelethus added a comment.

Move `CallDescription` specific changes to D119004 
.


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

https://reviews.llvm.org/D118880

Files:
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp

Index: clang/test/Analysis/NewDeleteLeaks.cpp
===
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -35,7 +35,9 @@
 
 } // namespace memory_allocated_in_fn_call
 
-namespace memory_passed_to_fn_call {
+// Realize that sink() intends to deallocate memory, assume that it should've
+// taken care of the leaked object as well.
+namespace memory_passed_to_fn_call_delete {
 
 void sink(int *P) {
   if (coin()) // ownership-note {{Assuming the condition is false}}
@@ -50,7 +52,24 @@
 } // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential leak}}
 
-} // namespace memory_passed_to_fn_call
+} // namespace memory_passed_to_fn_call_delete
+
+namespace memory_passed_to_fn_call_free {
+
+void sink(int *P) {
+  if (coin()) // ownership-note {{Assuming the condition is false}}
+  // ownership-note@-1 {{Taking false branch}}
+free(P);
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}}
+  sink(ptr); // ownership-note {{Calling 'sink'}}
+ // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call_free
 
 namespace memory_shared_with_ptr_of_shorter_lifetime {
 
@@ -123,6 +142,24 @@
 
 } // namespace memory_passed_into_fn_that_doesnt_intend_to_free
 
+namespace memory_passed_into_fn_that_doesnt_intend_to_free2 {
+
+void bar();
+
+void sink(int *P) {
+  // Correctly realize that calling bar() doesn't mean that this function would
+  // like to deallocate anything.
+  bar();
+}
+
+void foo() {
+  int *ptr = new int(5); // expected-note {{Memory is allocated}}
+  sink(ptr);
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_into_fn_that_doesnt_intend_to_free2
+
 namespace refkind_from_unoallocated_to_allocated {
 
 // RefKind of the symbol changed from nothing to Allocated. We don't want to
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -398,6 +398,9 @@
   };
 
   bool isFreeingCall(const CallEvent &Call) const;
+  static bool isFreeingOwnershipAttrCall(const FunctionDecl *Func);
+
+  friend class NoOwnershipChangeVisitor;
 
   CallDescriptionMap AllocatingMemFnMap{
   {{"alloca", 1}, &MallocChecker::checkAlloca},
@@ -742,7 +745,9 @@
 
 namespace {
 class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
+  // The symbol whose (lack of) ownership change we are interested in.
   SymbolRef Sym;
+  const MallocChecker *Checker;
   using OwnerSet = llvm::SmallPtrSet;
 
   // Collect which entities point to the allocated memory, and could be
@@ -794,18 +799,38 @@
 return "";
   }
 
+  bool isFreeingCallImprecise(const CallExpr &Call) const {
+if (Checker->FreeingMemFnMap.lookupImprecise(Call) ||
+Checker->ReallocatingMemFnMap.lookupImprecise(Call))
+  return true;
+
+if (const auto *Func = dyn_cast(Call.getCalleeDecl()))
+  return MallocChecker::isFreeingOwnershipAttrCall(Func);
+
+return false;
+  }
+
   bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) {
 using namespace clang::ast_matchers;
 const FunctionDecl *FD = dyn_cast(Callee);
 if (!FD)
   return false;
-// TODO: Operator delete is hardly the only deallocator -- Can we reuse
+// TODO: Operator delete is hardly the only deallocatr -- Can we reuse
 // isFreeingCall() or something thats already here?
-auto Deallocations = match(
-stmt(hasDescendant(cxxDeleteExpr().bind("delete"))
- ), *FD->getBody(), ACtx);
+auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"),
+callExpr().bind("call",
+ *FD->getBody(), ACtx);
+for (BoundNodes Match : Matches) {
+  if (Match.getNodeAs("delete")) {
+return true;
+  }
+  if (const auto *Call = Match.getNodeAs("call")) {
+

[PATCH] D119004: [NFC][analyzer] Allow CallDescriptions to be matched with CallExprs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, steakhal, xazax.hun, martong, ASDenysPetrov.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Szelethus requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

As of now, we have two interfaces to for defining signatures: 
`StdLibraryFunctionsChecker::Signature` and `CallDescription`. An example for 
how `Signature`s are used can be seen here:

  addToFunctionSummaryMap(
  "isprint", Signature(ArgTypes{IntTy}, RetType{IntTy}),
  Summary(EvalCallAsPure)
  .Case({ArgumentCondition(0U, WithinRange, Range(32, 126)),
 ReturnValueCondition(OutOfRange, SingleValue(0))})
  .Case({ArgumentCondition(0U, OutOfRange, Range(32, 126)),
 ReturnValueCondition(WithinRange, SingleValue(0))}));

The name of the function is searched for in translation unit's identifier 
table, then the `Signature` is matched against each decl `Decl` with the same 
name. Ideally, this yields a `FunctionDecl`, which is mapped to its `Summary`.

This works well for C functions, but doesn't support C++ at all.

`CallDescription` emerged with a strong emphasis on recognizing C++ functions. 
The common example brough up is `std::string`, which in some standard library 
implementations is actually called something like `std::__cxx11::basic_string`, 
but not in others. Matching this can be a nightmare for checker developers. For 
this reason, `CallDescription`s can be defined like this:

  
  InnerPointerChecker()
  : AppendFn({"std", "basic_string", "append"}),
AssignFn({"std", "basic_string", "assign"}),
AddressofFn({"std", "addressof"}),
ClearFn({"std", "basic_string", "clear"}),
CStrFn({"std", "basic_string", "c_str"}), DataFn({"std", "data"}, 1),
DataMemberFn({"std", "basic_string", "data"}),
EraseFn({"std", "basic_string", "erase"}),
InsertFn({"std", "basic_string", "insert"}),
PopBackFn({"std", "basic_string", "pop_back"}),
PushBackFn({"std", "basic_string", "push_back"}),
ReplaceFn({"std", "basic_string", "replace"}),
ReserveFn({"std", "basic_string", "reserve"}),
ResizeFn({"std", "basic_string", "resize"}),
ShrinkToFitFn({"std", "basic_string", "shrink_to_fit"}),
SwapFn({"std", "basic_string", "swap"}) {}

Any identifier which matches //at least// these identifiers are considered a 
match (which sometimes leads to incorrect matching, e.g. D81745 
).

`CallDescription`s are (usually) not used for digging up `FunctionDecl`s from 
the translation unit, but rather during symbolic execution to check in a 
pre/post call event whether the called function matches the `CallDescription`:

  bool InnerPointerChecker::isInnerPointerAccessFunction(
  const CallEvent &Call) const {
return matchesAny(Call, CStrFn, DataFn, DataMemberFn);
  }

Most of the new checkers implementing pre/post condition checks on functions 
now use `CallDescriptionMap` or `CallDescriptionSet`. Its up to debate whether 
the newer `Signature` approach is better, but its not obvious, and converting 
from one to the other may be non-trivial as well.

---

Now, onto this patch. Since `CallDescription`s can only be matched against 
`CallEvent`s that are created during symbolic execution, it was not possible to 
use it in syntactic-only contexts. For example, even though 
`InnerPointerChecker` can check with its set of `CallDescription`s whether a 
function call is interested during analysis, its unable to check without hassle 
whether a non-analyzer piece of code also calls such a function.

The patch adds the ability to use `CallDescription`s in syntactic contexts as 
well. While we already have that in `Signature`, we still want to leverage the 
ability to use dynamic information when we have it (function pointers, for 
example). This could be done with `Signature` as well 
(`StdLibraryFunctionsChecker` does it), but it makes it even less of a drop-in 
replacement.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119004

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
  clang/lib/StaticAnalyzer/Core/CallDescription.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -6,11 +6,18 @@
 //
 //===--===//
 
+#include "CheckerRegistration.h"
 #include "Reusables.h"
 
 #include "clang/AST/ExprCXX.h"
+#include "clang/Analysis/PathDiagnostic.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#

[PATCH] D118880: [analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, steakhal, martong, balazske, ASDenysPetrov.
Szelethus added a project: clang.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, 
yaxunl.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.

The problem with leak bug reports is that the most interesting event in the 
code is likely the one that did //not// happen -- lack of ownership change and 
lack of deallocation, which is often present within the same function that the 
analyzer inlined anyway, but not on the path of execution on which the bug 
occured. We struggle to understand that a function was responsible for freeing 
the memory, but failed.

D105819  added a new visitor to improve 
memory leak bug reports. In addition to inspecting the ExplodedNodes of the bug 
pat, the visitor tries to guess whether the function was supposed to free 
memory, but failed to. Initially (in D108753 
), this was done by checking whether a 
`CXXDeleteExpr` is present in the function. If so, we assume that the function 
was at least party responsible, and prevent the analyzer from pruning bug 
report notes in it. This patch improves this heuristic by recognizing all 
deallocator functions that MallocChecker itself recognizes, by reusing 
`MallocChecker::isFreeingCall`.

However, we are only able to match a `CallEvent` against a `CallDescription`. 
`CallEvent`s are created during symbolic execution, providing a lot of 
additional information about the call, but the grand idea behind this visitor 
that it checks code that was //not// executed smybolically (well, not on the 
path of execution on which the bug was found). For this reason, I added a set 
of functions to allow matching a `CallExpr` against a `CallDescription`.

I am aware that this idea may induce strong opinions -- after all, I'm adding a 
a new interface called `.*Imprecise`, discourage people from using it in the 
comments, while we already have one too many confusing interfaces in the 
analyzer, not to mention that `CallDescription` was meant to be among the 
better documented, newcomer friendlier and more intuitive tools we have. Also, 
its possible that matching function calls in syntactic-only cases should be 
done by `StdLibraryFunctionChecker::Signature` instead.

I argue for this patch because

- A lot of checkers use `CallDescriptionMap` already, and we either never 
intend to change to `Signature`, or it would be an enormous effort upfront
- Leak checkers like `StreamChecker` could benefit from this as well
- The comments I believe resolve the unintuitiveness of the new interface well
- In the case of this patch, matching `CallEvent`s for plain C function calls 
is pretty much matching against a `CallExpr` anyways.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118880

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Core/CallDescription.cpp
  clang/test/Analysis/NewDeleteLeaks.cpp

Index: clang/test/Analysis/NewDeleteLeaks.cpp
===
--- clang/test/Analysis/NewDeleteLeaks.cpp
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -35,7 +35,9 @@
 
 } // namespace memory_allocated_in_fn_call
 
-namespace memory_passed_to_fn_call {
+// Realize that sink() intends to deallocate memory, assume that it should've
+// taken care of the leaked object as well.
+namespace memory_passed_to_fn_call_delete {
 
 void sink(int *P) {
   if (coin()) // ownership-note {{Assuming the condition is false}}
@@ -50,7 +52,24 @@
 } // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
 // expected-note@-1 {{Potential leak}}
 
-} // namespace memory_passed_to_fn_call
+} // namespace memory_passed_to_fn_call_delete
+
+namespace memory_passed_to_fn_call_free {
+
+void sink(int *P) {
+  if (coin()) // ownership-note {{Assuming the condition is false}}
+  // ownership-note@-1 {{Taking false branch}}
+free(P);
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+  int *ptr = (int *)malloc(sizeof(int)); // expected-note {{Memory is allocated}}
+  sink(ptr); // ownership-note {{Calling 'sink'}}
+ // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call_free
 
 namespace memory_shared_with_ptr_of_shorter_lifetime {
 
@@ -123,6 +142,24 @@
 
 } // namespace memory_passed_into_fn_that_doesnt_intend_to_free
 
+namespace memory_passed_into_fn_that_do

[PATCH] D116597: [analyzer] Don't track function calls as control dependencies

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Ping ^-^


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

https://reviews.llvm.org/D116597

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

[PATCH] D116597: [analyzer] Don't track function calls as control dependencies

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 402549.
Szelethus added a comment.

Fix tests, mention that this is purely a heuristic.


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

https://reviews.llvm.org/D116597

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -149,14 +149,13 @@
   int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
 
   if (ConvertsToBool())
-// debug-note-re@-1^}}Tracking condition 'ConvertsToBool()'{{$
-// expected-note-re@-2^}}Assuming the condition is true{{$
-// expected-note-re@-3^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
 }
 
-} // end of namespace variable_declaration_in_condition
+} // namespace conversion_to_bool
 
 namespace note_from_different_but_not_nested_stackframe {
 
@@ -186,14 +185,13 @@
 int *getIntPtr();
 
 void storeValue(int **i) {
-  *i = getIntPtr(); // tracking-note-re^}}Value assigned to 'i', which participates in a condition later{{$
+  *i = getIntPtr();
 }
 
 int *conjurePointer() {
   int *i;
-  storeValue(&i); // tracking-note-re^}}Calling 'storeValue'{{$
-  // tracking-note-re@-1^}}Returning from 'storeValue'{{$
-  return i; // tracking-note-re^}}Returning pointer (loaded from 'i'), which participates in a condition later{{$
+  storeValue(&i);
+  return i;
 }
 
 void f(int *ptr) {
@@ -201,11 +199,8 @@
// expected-note-re@-1^}}Taking false branch{{$
 ;
   if (!conjurePointer())
-// tracking-note-re@-1^}}Calling 'conjurePointer'{{$
-// tracking-note-re@-2^}}Returning from 'conjurePointer'{{$
-// debug-note-re@-3^}}Tracking condition '!conjurePointer()'{{$
-// expected-note-re@-4^}}Assuming the condition is true{{$
-// expected-note-re@-5^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *ptr = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
 }
@@ -226,9 +221,8 @@
// expected-note-re@-1^}}Taking false branch{{$
 ;
   if (!conjurePointer())
-// debug-note-re@-1^}}Tracking condition '!conjurePointer()'{{$
-// expected-note-re@-2^}}Assuming the condition is true{{$
-// expected-note-re@-3^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *ptr = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
 }
@@ -246,9 +240,8 @@
   int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
 
   if (cast(conjure()))
-// debug-note-re@-1^}}Tracking condition 'cast(conjure())'{{$
-// expected-note-re@-2^}}Assuming the condition is false{{$
-// expected-note-re@-3^}}Taking false branch{{$
+// expected-note-re@-1^}}Assuming the condition is false{{$
+// expected-note-re@-2^}}Taking false branch{{$
 return;
   *x = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
@@ -266,9 +259,8 @@
// expected-note-re@-1^}}Taking false branch{{$
 ;
   if (!flipCoin())
-// debug-note-re@-1^}}Tracking condition '!flipCoin()'{{$
-// expected-note-re@-2^}}Assuming the condition is true{{$
-// expected-note-re@-3^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *ptr = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
 }
@@ -278,11 +270,9 @@
 bool coin();
 
 bool flipCoin() {
-  if (coin()) // tracking-note-re^}}Assuming the condition is false{{$
-  // tracking-note-re@-1^}}Taking false branch{{$
-  // debug-note-re@-2^}}Tracking condition 'coin()'{{$
+  if (coin())
 return true;
-  return coin(); // tracking-note-re^}}Returning value, wh

[PATCH] D115521: [Templight] Don't display empty strings for names of unnamed template parameters

[Kristóf Umann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3ad35ba4dea5: [Templight] Don't display empty strings 
for names of unnamed template parameters (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D115521?vs=397054&id=402527#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115521

Files:
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Templight/templight-empty-entries-fix.cpp

Index: clang/test/Templight/templight-empty-entries-fix.cpp
===
--- /dev/null
+++ clang/test/Templight/templight-empty-entries-fix.cpp
@@ -0,0 +1,333 @@
+// RUN: %clang_cc1 -templight-dump -Wno-unused-value %s 2>&1 | FileCheck %s
+
+void a() {
+  [] {};
+}
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^kind:[ ]+Memoization$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^kind:[ ]+Memoization$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+
+template  void a() { a(); }
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template non-type parameter 0 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:15'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template non-type parameter 0 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:15'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'a<0>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'a<0>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+
+template  struct b { typedef int c; };
+template ::c> void a() { a(); }
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:63'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+d$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:16'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+d$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:16'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template type parameter 1 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:32'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'b<1>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:59:23'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:43'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'b<1>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+En

[PATCH] D115521: [Templight] Don't display empty strings for names of unnamed template parameters

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/Frontend/FrontendActions.cpp:501
+
+if (const auto *Decl = dyn_cast(NamedTemplate)) {
+  if (const auto *R = dyn_cast(Decl)) {

Szelethus wrote:
> martong wrote:
> > martong wrote:
> > > Should this handle `EnumDecl`s as well? An enum declaration itself cannot 
> > > be a primary template, however, it can be 1) a member of a specialization 
> > > of a templated class 2)  an instantiation of a member enumeration of a 
> > > class template specialization.
> > > 
> > > 
> > Should this handle variable templates (VarTemplateDecl) as well? Or it  is 
> > not possible to have unnamed variable templates?
> `EnumDecl` is a subclass of `TagDecl`, unless I misunderstood what you meant?
Right now, yes, it seems to that unnamed variable templates are not a thing.



Comment at: clang/lib/Frontend/FrontendActions.cpp:501-510
+if (const auto *Decl = dyn_cast(NamedTemplate)) {
+  if (const auto *R = dyn_cast(Decl)) {
+if (R->isLambda()) {
+  OS << "lambda at ";
+  Decl->getLocation().print(OS, TheSema.getSourceManager());
+  return;
+}

martong wrote:
> martong wrote:
> > Should this handle `EnumDecl`s as well? An enum declaration itself cannot 
> > be a primary template, however, it can be 1) a member of a specialization 
> > of a templated class 2)  an instantiation of a member enumeration of a 
> > class template specialization.
> > 
> > 
> Should this handle variable templates (VarTemplateDecl) as well? Or it  is 
> not possible to have unnamed variable templates?
`EnumDecl` is a subclass of `TagDecl`, unless I misunderstood what you meant?


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

https://reviews.llvm.org/D115521

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

[PATCH] D116597: [analyzer] Don't track function calls as control dependencies

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: dkrupp, NoQ, steakhal, gamesh411, martong, balazske.
Szelethus added a project: clang.
Herald added subscribers: manas, ASDenysPetrov, donat.nagy, mikhail.ramalho, 
a.sidorin, rnkovacs, szepet, baloghadamsoftware, kristof.beyls, xazax.hun.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.

I recently evaluated ~150 of bug reports on open source projects relating to my 
GSoC'19 project , which was about 
tracking control dependencies that were relevant to a bug report.

Here is what I found: when the condition is a function call, the extra notes 
were almost always unimportant, and often times intrusive:

  void f(int *x) {
x = nullptr;
if (alwaysTrue()) // We don't need a whole lot of explanation
  // here, the function name is good enough.
  *x = 5;
  }

It almost always boiled down to a few "Returning null pointer, which 
participates in a condition later", or similar notes. I struggled to find a 
single case where the notes revealed anything interesting or some previously 
hidden correlation, which is kind of the point of condition tracking.

This patch checks whether the condition is a function call, and if so, bails 
out.

The argument against the patch is the popular feedback we hear from some of our 
users, namely that they can never have too much information. I was specifically 
fishing for examples that display best that my contribution did more good than 
harm, so admittedly I set the bar high, and one can argue that there can be 
non-trivial trickery inside functions, and function names may not be that 
descriptive.

My argument for the patch is all those reports that got longer without any 
notable improvement in the report intelligibility. I think the few exceptional 
cases where this patch would remove notable information are an acceptable 
sacrifice in favor of more reports being leaner.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116597

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -149,14 +149,13 @@
   int *x = 0; // expected-note-re^}}'x' initialized to a null pointer value{{$
 
   if (ConvertsToBool())
-// debug-note-re@-1^}}Tracking condition 'ConvertsToBool()'{{$
-// expected-note-re@-2^}}Assuming the condition is true{{$
-// expected-note-re@-3^}}Taking true branch{{$
+// expected-note-re@-1^}}Assuming the condition is true{{$
+// expected-note-re@-2^}}Taking true branch{{$
 *x = 5; // expected-warning{{Dereference of null pointer}}
 // expected-note@-1{{Dereference of null pointer}}
 }
 
-} // end of namespace variable_declaration_in_condition
+} // namespace conversion_to_bool
 
 namespace note_from_different_but_not_nested_stackframe {
 
@@ -186,14 +185,13 @@
 int *getIntPtr();
 
 void storeValue(int **i) {
-  *i = getIntPtr(); // tracking-note-re^}}Value assigned to 'i', which participates in a condition later{{$
+  *i = getIntPtr();
 }
 
 int *conjurePointer() {
   int *i;
-  storeValue(&i); // tracking-note-re^}}Calling 'storeValue'{{$
-  // tracking-note-re@-1^}}Returning from 'storeValue'{{$
-  return i; // tracking-note-re^}}Returning pointer (loaded from 'i'), which participates in a condition later{{$
+  storeValue(&i);
+  return i;
 }
 
 void f(int *ptr) {
@@ -201,11 +199,9 @@
// expected-note-re@-1^}}Taking false branch{{$
 ;
   if (!conjurePointer())
-// tracking-note-re@-1^}}Calling 'conjurePointer'{{$
-// tracking-note-re@-2^}}Returning from 'conjurePointer'{{$
-// debug-note-re@-3^}}Tracking condition '!conjurePointer()'{{$
-// expected-note-re@-4^}}Assuming the condition is true{{$
-// expected-note-re@-5^}}Taking true branch{{$
+// debug-note-re@-1^}}Tracking condition '!conjurePointer()'{{$
+// expected-note-re@-2^}}Assuming the condition is true{{$
+// expected-note-re@-3^}}Taking true branch{{$
 *ptr = 5; // expected-warning{{Dereference of null pointer}}
   // expected-note@-1{{Dereference of null pointer}}
 }
@@ -226,9 +222,8 @@
// expected-note-re@-1^}}Taking false branch{{$
 ;
   if (!conjurePointer())
-// debug-note-re@-1^}}Tracking condition '!conjurePointer()'{{$
-// expected-note-re@-2^}}Assuming the condition

[PATCH] D115716: [Analyzer][BugReporter] Replace the example bug report with the one used to generate PathDiagnostic

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:76
+ std::unique_ptr> {
+  const BugReport *Report = nullptr;
+};

Some comments about this field would be welcome!



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:3076-3077
 
   ArrayRef Consumers = getPathDiagnosticConsumers();
+  return generateDiagnosticForConsumerMap(EQ, Consumers);
+}

Considering that the consumers are already a part of the `BugReport`, the 
overload on `generateDiagnosticForConsumerMap` feels too arbitrary. Wouldn't a 
predicate function instead of an overload be more fitting?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:3210
+BugReportEquivClass &EQ, ArrayRef consumers) {
+  SmallVector _;
+  auto *report = BugReporter::findReportInEquivalenceClass(EQ, _);

Ohh, a variable with its pants down!



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:3286-3290
 std::unique_ptr
 PathSensitiveBugReporter::generateDiagnosticForConsumerMap(
-BugReport *exampleReport, ArrayRef consumers,
-ArrayRef bugReports) {
+BugReportEquivClass &EQ, ArrayRef consumers) {
+  SmallVector bugReports;
+  const BugReport *report = findReportInEquivalenceClass(EQ, bugReports);

Really happy to see `exampleReport` gone as a parameter.


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

https://reviews.llvm.org/D115716

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

[PATCH] D115716: [Analyzer][BugReporter] Replace the example bug report with the one used to generate PathDiagnostic

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

First off, your patch is great, and I'm pretty sure we want it!

I remember working around here, and I either have never quite understood what 
makes `exampleReport` an example, or have long forgotten. Can we not just 
rename it to `chosenReport`, or simply `report`?




Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:3068-3078
+  const BugReport *report = findReportInEquivalenceClass(EQ, bugReports);
   if (!report)
 return;
 
   // See whether we need to silence the checker/package.
   for (const std::string &CheckerOrPackage :
getAnalyzerOptions().SilencedCheckersAndPackages) {

Wouldn't it make more sense to create initialize (instead of resetting) 
`report` at the call to `generateDiagnosticForConsumerMap()`? This just looks a 
bit convoluted. We could turn everything up to that point into a 
[[https://llvm.org/docs/CodingStandards.html#turn-predicate-loops-into-predicate-functions
 | predicate function]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115716

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

[PATCH] D115521: [Templight] Don't display empty strings for names of unnamed template parameters

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 397054.
Szelethus added a comment.

Add a default text, if another, unhandled unnamed identifier pops up.


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

https://reviews.llvm.org/D115521

Files:
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Templight/templight-empty-entries-fix.cpp

Index: clang/test/Templight/templight-empty-entries-fix.cpp
===
--- /dev/null
+++ clang/test/Templight/templight-empty-entries-fix.cpp
@@ -0,0 +1,333 @@
+// RUN: %clang_cc1 -templight-dump -Wno-unused-value %s 2>&1 | FileCheck %s
+
+void a() {
+  [] {};
+}
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^kind:[ ]+Memoization$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^kind:[ ]+Memoization$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+
+template  void a() { a(); }
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template non-type parameter 0 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:15'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template non-type parameter 0 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:15'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'a<0>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'a<0>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+
+template  struct b { typedef int c; };
+template ::c> void a() { a(); }
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:63'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+d$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:16'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+d$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:16'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template type parameter 1 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:32'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'b<1>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:59:23'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:43'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'b<1>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:59:23'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:43'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'b<1>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}

[PATCH] D115521: [Templight] Don't return string for name for unnamed template parameters

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: sabel83, xazax.hun, rsmith, whisperity, martong, 
mikael-s-persson, mclow.lists.
Szelethus added a project: clang.
Herald added subscribers: steakhal, gamesh411, dkrupp, rnkovacs.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.

Patch by oktal3000 : 
https://github.com/mikael-s-persson/templight/pull/40

When a template parameter is unnamed, the `name` of `-templight-dump` might 
return an empty string. This is fine, they are unnamed after all, but it might 
be more user friendly to at least describe what entity is unnamed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115521

Files:
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Templight/templight-empty-entries-fix.cpp

Index: clang/test/Templight/templight-empty-entries-fix.cpp
===
--- /dev/null
+++ clang/test/Templight/templight-empty-entries-fix.cpp
@@ -0,0 +1,333 @@
+// RUN: %clang_cc1 -templight-dump -Wno-unused-value %s 2>&1 | FileCheck %s
+
+void a() {
+  [] {};
+}
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^kind:[ ]+Memoization$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'lambda at .*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^kind:[ ]+Memoization$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:4:3'$}}
+
+template  void a() { a(); }
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template non-type parameter 0 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:15'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template non-type parameter 0 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:15'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'a<0>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'a<0>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:20:25'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:20:31'$}}
+
+template  struct b { typedef int c; };
+template ::c> void a() { a(); }
+
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+a$}}
+// CHECK: {{^kind:[ ]+DeducedTemplateArgumentSubstitution$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:63'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+d$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:16'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+d$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+End$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:16'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+unnamed template type parameter 1 of a$}}
+// CHECK: {{^kind:[ ]+DefaultTemplateArgumentInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templight-empty-entries-fix.cpp:60:32'$}}
+// CHECK: {{^poi:[ ]+'.*templight-empty-entries-fix.cpp:60:57'$}}
+// CHECK-LABEL: {{^---$}}
+// CHECK: {{^name:[ ]+'b<1>'$}}
+// CHECK: {{^kind:[ ]+TemplateInstantiation$}}
+// CHECK: {{^event:[ ]+Begin$}}
+// CHECK: {{^orig:[ ]+'.*templig

[PATCH] D113201: [clang-tidy] Fix a crash in modernize-loop-convert around conversion operators

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Szelethus marked 2 inline comments as done.
Closed by commit rG29a8d45c5a23: [clang-tidy] Fix a crash in 
modernize-loop-convert around conversion operators (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D113201?vs=385459&id=387206#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113201

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
  clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
  clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-basic.cpp
@@ -273,6 +273,16 @@
   // CHECK-FIXES: for (int & It : Tt)
   // CHECK-FIXES-NEXT: printf("I found %d\n", It);
 
+  // Do not crash because of Qq.begin() converting. Q::iterator converts with a
+  // conversion operator, which has no name, to Q::const_iterator.
+  Q Qq;
+  for (Q::const_iterator It = Qq.begin(), E = Qq.end(); It != E; ++It) {
+printf("I found %d\n", *It);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : Qq)
+  // CHECK-FIXES-NEXT: printf("I found %d\n", It);
+
   T *Pt;
   for (T::iterator It = Pt->begin(), E = Pt->end(); It != E; ++It) {
 printf("I found %d\n", *It);
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
===
--- clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h
@@ -53,6 +53,23 @@
   iterator end();
 };
 
+struct Q {
+  typedef int value_type;
+  struct const_iterator {
+value_type &operator*();
+const value_type &operator*() const;
+const_iterator &operator++();
+bool operator!=(const const_iterator &other);
+void insert(value_type);
+value_type X;
+  };
+  struct iterator {
+operator const_iterator() const;
+  };
+  iterator begin();
+  iterator end();
+};
+
 struct U {
   struct iterator {
 Val& operator*();
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
===
--- clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
@@ -275,7 +275,7 @@
 typedef llvm::SmallVector UsageResult;
 
 // General functions used by ForLoopIndexUseVisitor and LoopConvertCheck.
-const Expr *digThroughConstructors(const Expr *E);
+const Expr *digThroughConstructorsConversions(const Expr *E);
 bool areSameExpr(ASTContext *Context, const Expr *First, const Expr *Second);
 const DeclRefExpr *getDeclRef(const Expr *E);
 bool areSameVariable(const ValueDecl *First, const ValueDecl *Second);
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
===
--- clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
@@ -152,20 +152,21 @@
   return true;
 }
 
-/// Look through conversion/copy constructors to find the explicit
-/// initialization expression, returning it is found.
+/// Look through conversion/copy constructors and member functions to find the
+/// explicit initialization expression, returning it is found.
 ///
 /// The main idea is that given
 ///   vector v;
 /// we consider either of these initializations
 ///   vector::iterator it = v.begin();
 ///   vector::iterator it(v.begin());
+///   vector::const_iterator it(v.begin());
 /// and retrieve `v.begin()` as the expression used to initialize `it` but do
 /// not include
 ///   vector::iterator it;
 ///   vector::iterator it(v.begin(), 0); // if this constructor existed
 /// as being initialized from `v.begin()`
-const Expr *digThroughConstructors(const Expr *E) {
+const Expr *digThroughConstructorsConversions(const Expr *E) {
   if (!E)
 return nullptr;
   E = E->IgnoreImplicit();
@@ -178,8 +179,13 @@
 E = ConstructExpr->getArg(0);
 if (const auto *Temp = dyn_cast(E))
   E = Temp->getSubExpr();
-return digThroughConstructors(E);
+return digThroughConstructorsConversions(E);
   }
+  // If this is a conversion (as iterators commonly convert into their const
+  // iterator counterparts), dig through that as well.
+  if (const auto *ME = dyn_cast(E))
+if (const auto *D = d

[PATCH] D113201: [clang-tidy] Fix a crash in modernize-loop-convert around conversion operators

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a reviewer: Eugene.Zelenko.
Szelethus added a comment.

I'll intend to land this by friday unless there are objections!


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

https://reviews.llvm.org/D113201

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

[PATCH] D113397: [analyzer][docs] Fix the incorrect structure of the checker docs

[Kristóf Umann via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8cc2de667ec2: [analyzer][docs] Fix the incorrect structure 
of the checker docs (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D113397?vs=385486&id=385795#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113397

Files:
  clang/docs/analyzer/checkers.rst

Index: clang/docs/analyzer/checkers.rst
===
--- clang/docs/analyzer/checkers.rst
+++ clang/docs/analyzer/checkers.rst
@@ -2064,90 +2064,6 @@
 alpha.security
 ^^
 
-
-alpha.security.cert
-^^^
-
-SEI CERT checkers which tries to find errors based on their `C coding rules `_.
-
-.. _alpha-security-cert-pos-checkers:
-
-alpha.security.cert.pos
-^^^
-
-SEI CERT checkers of `POSIX C coding rules `__.
-
-.. _alpha-security-cert-pos-34c:
-
-alpha.security.cert.pos.34c
-"""
-Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument.
-
-.. code-block:: c
-
-  int func(const char *var) {
-char env[1024];
-int retval = snprintf(env, sizeof(env),"TEST=%s", var);
-if (retval < 0 || (size_t)retval >= sizeof(env)) {
-/* Handle error */
-}
-
-return putenv(env); // putenv function should not be called with auto variables
-  }
-
-alpha.security.cert.env
-^^^
-
-SEI CERT checkers of `POSIX C coding rules `__.
-
-.. _alpha-security-cert-env-InvalidPtr:
-
-alpha.security.cert.env.InvalidPtr
-""
-
-Corresponds to SEI CERT Rules ENV31-C and ENV34-C.
-
-ENV31-C:
-Rule is about the possible problem with `main` function's third argument, environment pointer,
-"envp". When enviornment array is modified using some modification function
-such as putenv, setenv or others, It may happen that memory is reallocated,
-however "envp" is not updated to reflect the changes and points to old memory
-region.
-
-ENV34-C:
-Some functions return a pointer to a statically allocated buffer.
-Consequently, subsequent call of these functions will invalidate previous
-pointer. These functions include: getenv, localeconv, asctime, setlocale, strerror
-
-.. code-block:: c
-
-  int main(int argc, const char *argv[], const char *envp[]) {
-if (setenv("MY_NEW_VAR", "new_value", 1) != 0) {
-  // setenv call may invalidate 'envp'
-  /* Handle error */
-}
-if (envp != NULL) {
-  for (size_t i = 0; envp[i] != NULL; ++i) {
-puts(envp[i]);
-// envp may no longer point to the current environment
-// this program has unanticipated behavior, since envp
-// does not reflect changes made by setenv function.
-  }
-}
-return 0;
-  }
-
-  void previous_call_invalidation() {
-char *p, *pp;
-
-p = getenv("VAR");
-pp = getenv("VAR2");
-// subsequent call to 'getenv' invalidated previous one
-
-*p;
-// dereferencing invalid pointer
-  }
-
 .. _alpha-security-ArrayBound:
 
 alpha.security.ArrayBound (C)
@@ -2299,6 +2215,95 @@
return x; // warn: undefined or garbage returned
  }
 
+
+alpha.security.cert
+^^^
+
+SEI CERT checkers which tries to find errors based on their `C coding rules `_.
+
+.. _alpha-security-cert-pos-checkers:
+
+alpha.security.cert.pos
+^^^
+
+SEI CERT checkers of `POSIX C coding rules `_.
+
+.. _alpha-security-cert-pos-34c:
+
+alpha.security.cert.pos.34c
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument.
+
+.. code-block:: c
+
+  int func(const char *var) {
+char env[1024];
+int retval = snprintf(env, sizeof(env),"TEST=%s", var);
+if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+}
+
+return putenv(env); // putenv function should not be called with auto variables
+  }
+
+alpha.security.cert.env
+^^^
+
+SEI CERT checkers of `Environment C coding rules `_.
+
+.. _alpha-security-cert-env-InvalidPtr:
+
+alpha.security.cert.env.InvalidPtr
+""
+
+Corresponds to SEI CERT Rules ENV31-C and ENV34-C.
+
+ENV31-C:
+Rule is about the possible problem with `main` function's third argument, environment pointer,
+"envp". When enviornment array is modified using some modification function
+such as putenv, setenv or others, It may happen that memory is

[PATCH] D113397: [analyzer][docs] Fix the incorrect structure of the checker docs

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 385486.
Szelethus added a comment.

Add context.


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

https://reviews.llvm.org/D113397

Files:
  clang/docs/analyzer/checkers.rst

Index: clang/docs/analyzer/checkers.rst
===
--- clang/docs/analyzer/checkers.rst
+++ clang/docs/analyzer/checkers.rst
@@ -2048,90 +2048,6 @@
 alpha.security
 ^^
 
-
-alpha.security.cert
-^^^
-
-SEI CERT checkers which tries to find errors based on their `C coding rules `_.
-
-.. _alpha-security-cert-pos-checkers:
-
-alpha.security.cert.pos
-^^^
-
-SEI CERT checkers of `POSIX C coding rules `_.
-
-.. _alpha-security-cert-pos-34c:
-
-alpha.security.cert.pos.34c
-"""
-Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument.
-
-.. code-block:: c
-
-  int func(const char *var) {
-char env[1024];
-int retval = snprintf(env, sizeof(env),"TEST=%s", var);
-if (retval < 0 || (size_t)retval >= sizeof(env)) {
-/* Handle error */
-}
-
-return putenv(env); // putenv function should not be called with auto variables
-  }
-
-alpha.security.cert.env
-^^^
-
-SEI CERT checkers of `POSIX C coding rules `_.
-
-.. _alpha-security-cert-env-InvalidPtr:
-
-alpha.security.cert.env.InvalidPtr
-"""
-
-Corresponds to SEI CERT Rules ENV31-C and ENV34-C.
-
-ENV31-C:
-Rule is about the possible problem with `main` function's third argument, environment pointer,
-"envp". When enviornment array is modified using some modification function
-such as putenv, setenv or others, It may happen that memory is reallocated,
-however "envp" is not updated to reflect the changes and points to old memory
-region.
-
-ENV34-C:
-Some functions return a pointer to a statically allocated buffer.
-Consequently, subsequent call of these functions will invalidate previous
-pointer. These functions include: getenv, localeconv, asctime, setlocale, strerror
-
-.. code-block:: c
-
-  int main(int argc, const char *argv[], const char *envp[]) {
-if (setenv("MY_NEW_VAR", "new_value", 1) != 0) {
-  // setenv call may invalidate 'envp'
-  /* Handle error */
-}
-if (envp != NULL) {
-  for (size_t i = 0; envp[i] != NULL; ++i) {
-puts(envp[i]);
-// envp may no longer point to the current environment
-// this program has unanticipated behavior, since envp
-// does not reflect changes made by setenv function.
-  }
-}
-return 0;
-  }
-
-  void previous_call_invalidation() {
-char *p, *pp;
-
-p = getenv("VAR");
-pp = getenv("VAR2");
-// subsequent call to 'getenv' invalidated previous one
-
-*p;
-// dereferencing invalid pointer
-  }
-
 .. _alpha-security-ArrayBound:
 
 alpha.security.ArrayBound (C)
@@ -2283,6 +2199,95 @@
return x; // warn: undefined or garbage returned
  }
 
+
+alpha.security.cert
+^^^
+
+SEI CERT checkers which tries to find errors based on their `C coding rules `_.
+
+.. _alpha-security-cert-pos-checkers:
+
+alpha.security.cert.pos
+^^^
+
+SEI CERT checkers of `POSIX C coding rules `_.
+
+.. _alpha-security-cert-pos-34c:
+
+alpha.security.cert.pos.34c
+"""
+Finds calls to the ``putenv`` function which pass a pointer to an automatic variable as the argument.
+
+.. code-block:: c
+
+  int func(const char *var) {
+char env[1024];
+int retval = snprintf(env, sizeof(env),"TEST=%s", var);
+if (retval < 0 || (size_t)retval >= sizeof(env)) {
+/* Handle error */
+}
+
+return putenv(env); // putenv function should not be called with auto variables
+  }
+
+alpha.security.cert.env
+^^^
+
+SEI CERT checkers of `Environment C coding rules `_.
+
+.. _alpha-security-cert-env-InvalidPtr:
+
+alpha.security.cert.env.InvalidPtr
+""
+
+Corresponds to SEI CERT Rules ENV31-C and ENV34-C.
+
+ENV31-C:
+Rule is about the possible problem with `main` function's third argument, environment pointer,
+"envp". When enviornment array is modified using some modification function
+such as putenv, setenv or others, It may happen that memory is reallocated,
+however "envp" is not updated to reflect the changes and points to old memory
+region.
+
+ENV34-C:
+Some functions return a pointer to a statically allocated buffer.
+Consequently, subsequent call of these functions will invalidate previous
+pointer. These functions include: getenv