[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Ali Shuja Siddiqui via Phabricator via cfe-commits]

alishuja added a comment.
Herald added a subscriber: manas.

Based on the comment from @vsavchenko , I had made an addition for a check for 
`__addressof` alongside `addressof` in the checker. What would be the correct 
way of pushing the diff here? Should I reopen this revision or use the update 
diff button on the top? I apologize, I'm completely new to pushing changes to 
LLVM.

Thanks,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D99260#2751967 , @steakhal wrote:

> In D99260#2704102 , @NoQ wrote:
>
>> In https://bugs.llvm.org/show_bug.cgi?id=45786 the godbolt link shows that 
>> there are still problems with `addressof` (yes, their "trunk" clang is fresh 
>> enough). They seem to have `__addressof` instead of `addressof` so maybe we 
>> should cover that case real quick. Or maybe outright suppress reference 
>> invalidation on double-underscore functions because the checker generally 
>> relies on the number of standard functions that don't invalidate references 
>> while taking a non-const reference being very limited but this limit 
>> definitely doesn't take double-underscore functions into account.
>
> What is the status quo of this issue? @vsavchenko

Ah, yep. Just got back from my vacation!
I'm not sure about shooting off double underscore functions because one never 
knows what weird coding conventions people have in their project (all TU-local 
static functions should be named like this for better visibility at their call 
sites, for example). Being more specific and dealing with `std::__` can be 
better, but I think a quick hack specifically for `std::__addressof` is better 
atm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D99260#2704102 , @NoQ wrote:

> In https://bugs.llvm.org/show_bug.cgi?id=45786 the godbolt link shows that 
> there are still problems with `addressof` (yes, their "trunk" clang is fresh 
> enough). They seem to have `__addressof` instead of `addressof` so maybe we 
> should cover that case real quick. Or maybe outright suppress reference 
> invalidation on double-underscore functions because the checker generally 
> relies on the number of standard functions that don't invalidate references 
> while taking a non-const reference being very limited but this limit 
> definitely doesn't take double-underscore functions into account.

What is the status quo of this issue? @vsavchenko


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In https://bugs.llvm.org/show_bug.cgi?id=45786 the godbolt link shows that 
there are still problems with `addressof` (yes, their "trunk" clang is fresh 
enough). They seem to have `__addressof` instead of `addressof` so maybe we 
should cover that case real quick. Or maybe outright suppress reference 
invalidation on double-underscore functions because the checker generally 
relies on the number of standard functions that don't invalidate references 
while taking a non-const reference being very limited but this limit definitely 
doesn't take double-underscore functions into account.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Valeriy Savchenko 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 rG663ac91ed1d6: [analyzer] Fix false positives in inner 
pointer checker (PR49628) (authored by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

Files:
  clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  clang/test/Analysis/inner-pointer.cpp

Index: clang/test/Analysis/inner-pointer.cpp
===
--- clang/test/Analysis/inner-pointer.cpp
+++ clang/test/Analysis/inner-pointer.cpp
@@ -17,6 +17,11 @@
 string my_string = "default";
 void default_arg(int a = 42, string  = my_string);
 
+template 
+T *addressof(T );
+
+char *data(std::string );
+
 } // end namespace std
 
 void consume(const char *) {}
@@ -273,6 +278,15 @@
   // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
 }
 
+void deref_after_std_data() {
+  const char *c;
+  std::string s;
+  c = std::data(s); // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
+  s.push_back('c'); // expected-note {{Inner buffer of 'std::string' reallocated by call to 'push_back'}}
+  consume(c);   // expected-warning {{Inner pointer of container used after re/deallocation}}
+  // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
+}
+
 struct S {
   std::string s;
   const char *name() {
@@ -361,8 +375,24 @@
   // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
 }
 
+void func_addressof() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  addressof(s);
+  consume(c); // no-warning
+}
+
+void func_std_data() {
+  const char *c;
+  std::string s;
+  c = std::data(s);
+  consume(c); // no-warning
+}
+
 struct T {
   std::string to_string() { return s; }
+
 private:
   std::string s;
 };
Index: clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -34,9 +34,9 @@
 class InnerPointerChecker
 : public Checker {
 
-  CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
-  InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
-  ShrinkToFitFn, SwapFn;
+  CallDescription AppendFn, AssignFn, AddressofFn, ClearFn, CStrFn, DataFn,
+  DataMemberFn, EraseFn, InsertFn, PopBackFn, PushBackFn, ReplaceFn,
+  ReserveFn, ResizeFn, ShrinkToFitFn, SwapFn;
 
 public:
   class InnerPointerBRVisitor : public BugReporterVisitor {
@@ -73,9 +73,10 @@
   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", "basic_string", "data"}),
+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"}),
@@ -90,6 +91,9 @@
   /// pointers referring to the container object's inner buffer.
   bool isInvalidatingMemberFunction(const CallEvent ) const;
 
+  /// Check whether the called function returns a raw inner pointer.
+  bool isInnerPointerAccessFunction(const CallEvent ) const;
+
   /// Mark pointer symbols associated with the given memory region released
   /// in the program state.
   void markPtrSymbolsReleased(const CallEvent , ProgramStateRef State,
@@ -130,6 +134,12 @@
   Call.isCalled(SwapFn));
 }
 
+bool InnerPointerChecker::isInnerPointerAccessFunction(
+const CallEvent ) const {
+  return (Call.isCalled(CStrFn) || Call.isCalled(DataFn) ||
+  Call.isCalled(DataMemberFn));
+}
+
 void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent ,
  ProgramStateRef State,
  const MemRegion *MR,
@@ -172,6 +182,11 @@
   if (!ArgRegion)
 continue;
 
+  // std::addressof function accepts a non-const reference as an argument,
+  // but doesn't modify it.
+  if (Call.isCalled(AddressofFn))
+continue;
+
   markPtrSymbolsReleased(Call, State, ArgRegion, C);
 }
   }
@@ -195,36 +210,49 @@
 CheckerContext ) const {
   ProgramStateRef State = C.getState();
 
+  // TODO: Do we need these to be typed?
+  const TypedValueRegion *ObjRegion = nullptr;
+
   if (const auto *ICall = dyn_cast()) {
-// TODO: Do we need these to be typed?
-const auto *ObjRegion = dyn_cast_or_null(
+

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I'm still not satisfied with the `addressof`, but I won't block this either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Artem Dergachev via Phabricator via cfe-commits]

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

Looks great now, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko updated this revision to Diff 336077.
vsavchenko added a comment.

Require std::data to accept exactly 1 argument


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

Files:
  clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  clang/test/Analysis/inner-pointer.cpp

Index: clang/test/Analysis/inner-pointer.cpp
===
--- clang/test/Analysis/inner-pointer.cpp
+++ clang/test/Analysis/inner-pointer.cpp
@@ -17,6 +17,11 @@
 string my_string = "default";
 void default_arg(int a = 42, string  = my_string);
 
+template 
+T *addressof(T );
+
+char *data(std::string );
+
 } // end namespace std
 
 void consume(const char *) {}
@@ -273,6 +278,15 @@
   // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
 }
 
+void deref_after_std_data() {
+  const char *c;
+  std::string s;
+  c = std::data(s); // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
+  s.push_back('c'); // expected-note {{Inner buffer of 'std::string' reallocated by call to 'push_back'}}
+  consume(c);   // expected-warning {{Inner pointer of container used after re/deallocation}}
+  // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
+}
+
 struct S {
   std::string s;
   const char *name() {
@@ -361,8 +375,24 @@
   // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
 }
 
+void func_addressof() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  addressof(s);
+  consume(c); // no-warning
+}
+
+void func_std_data() {
+  const char *c;
+  std::string s;
+  c = std::data(s);
+  consume(c); // no-warning
+}
+
 struct T {
   std::string to_string() { return s; }
+
 private:
   std::string s;
 };
Index: clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -34,9 +34,9 @@
 class InnerPointerChecker
 : public Checker {
 
-  CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
-  InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
-  ShrinkToFitFn, SwapFn;
+  CallDescription AppendFn, AssignFn, AddressofFn, ClearFn, CStrFn, DataFn,
+  DataMemberFn, EraseFn, InsertFn, PopBackFn, PushBackFn, ReplaceFn,
+  ReserveFn, ResizeFn, ShrinkToFitFn, SwapFn;
 
 public:
   class InnerPointerBRVisitor : public BugReporterVisitor {
@@ -73,9 +73,10 @@
   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", "basic_string", "data"}),
+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"}),
@@ -90,6 +91,9 @@
   /// pointers referring to the container object's inner buffer.
   bool isInvalidatingMemberFunction(const CallEvent ) const;
 
+  /// Check whether the called function returns a raw inner pointer.
+  bool isInnerPointerAccessFunction(const CallEvent ) const;
+
   /// Mark pointer symbols associated with the given memory region released
   /// in the program state.
   void markPtrSymbolsReleased(const CallEvent , ProgramStateRef State,
@@ -130,6 +134,12 @@
   Call.isCalled(SwapFn));
 }
 
+bool InnerPointerChecker::isInnerPointerAccessFunction(
+const CallEvent ) const {
+  return (Call.isCalled(CStrFn) || Call.isCalled(DataFn) ||
+  Call.isCalled(DataMemberFn));
+}
+
 void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent ,
  ProgramStateRef State,
  const MemRegion *MR,
@@ -172,6 +182,11 @@
   if (!ArgRegion)
 continue;
 
+  // std::addressof function accepts a non-const reference as an argument,
+  // but doesn't modify it.
+  if (Call.isCalled(AddressofFn))
+continue;
+
   markPtrSymbolsReleased(Call, State, ArgRegion, C);
 }
   }
@@ -195,36 +210,49 @@
 CheckerContext ) const {
   ProgramStateRef State = C.getState();
 
+  // TODO: Do we need these to be typed?
+  const TypedValueRegion *ObjRegion = nullptr;
+
   if (const auto *ICall = dyn_cast()) {
-// TODO: Do we need these to be typed?
-const auto *ObjRegion = dyn_cast_or_null(
+ObjRegion = dyn_cast_or_null(
 ICall->getCXXThisVal().getAsRegion());
-if (!ObjRegion)
-  return;
 
-if 

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> It does make sense to split these in two, but I'm not so sure about 
> `evalCall`.

`evalCall` should work great for this purpose but definitely not in this 
checker. Also this checker would still exercise its `checkPostCall` so it would 
still need to be silenced even if `evalCall` is implemented.




Comment at: clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp:234
+  ObjRegion =
+  dyn_cast_or_null(Call.getArgSVal(0).getAsRegion());
 }

This will crash if someone overloads `std::data` with 0 arguments because your 
`CallDescription`s don't require a specific number of arguments. (They're not 
allowed to do that, yes, but we're still not allowed to crash.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/test/Analysis/inner-pointer.cpp:23
+
+char *data(std::string );
+

vsavchenko wrote:
> martong wrote:
> > Seems like all test are exercising with std::string, this looks like a 
> > legacy in this Checker.
> > Still, I miss a bit at least one test for the other overloads of 
> > `std::data`, maybe in a follow up patch?
> I can add it here, but what other test you suggest to add?
For example,
```
char a[20];
auto c = std::data(s);
consume(c);
```
Would this produce a warning?

Similarly to initializer list:
```
template  
constexpr const E* data(std::initializer_list il) noexcept
{
return il.begin();
}

int test() {
auto IL = {0,1,2};
const auto I = data(IL);
consume(I);
return 0;
}
```




Comment at: clang/test/Analysis/inner-pointer.cpp:378-392
+void func_addressof() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  addressof(s);
+  consume(c); // no-warning
+}

vsavchenko wrote:
> martong wrote:
> > So these are the FP cases that you are trying to solve?
> > Would be nice to see more details about the bug report (rdar://73463300) if 
> > that is not proprietary.
> ```
> std::optional str = "example";
> char* dup = strndup(str->c_str(), str->size());
> ```
> `std::optional::operator->` uses `std::addressof` and the analyzer thinks 
> that the pointer might get changed and raises the alarm.
> I decided not to replicate `std::optional` in tests, and get straight to the 
> point.
Okay, thanks, makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/test/Analysis/inner-pointer.cpp:23
+
+char *data(std::string );
+

martong wrote:
> Seems like all test are exercising with std::string, this looks like a legacy 
> in this Checker.
> Still, I miss a bit at least one test for the other overloads of `std::data`, 
> maybe in a follow up patch?
I can add it here, but what other test you suggest to add?



Comment at: clang/test/Analysis/inner-pointer.cpp:378-392
+void func_addressof() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  addressof(s);
+  consume(c); // no-warning
+}

martong wrote:
> So these are the FP cases that you are trying to solve?
> Would be nice to see more details about the bug report (rdar://73463300) if 
> that is not proprietary.
```
std::optional str = "example";
char* dup = strndup(str->c_str(), str->size());
```
`std::optional::operator->` uses `std::addressof` and the analyzer thinks that 
the pointer might get changed and raises the alarm.
I decided not to replicate `std::optional` in tests, and get straight to the 
point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/test/Analysis/inner-pointer.cpp:23
+
+char *data(std::string );
+

Seems like all test are exercising with std::string, this looks like a legacy 
in this Checker.
Still, I miss a bit at least one test for the other overloads of `std::data`, 
maybe in a follow up patch?



Comment at: clang/test/Analysis/inner-pointer.cpp:378-392
+void func_addressof() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  addressof(s);
+  consume(c); // no-warning
+}

So these are the FP cases that you are trying to solve?
Would be nice to see more details about the bug report (rdar://73463300) if 
that is not proprietary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D99260#2650201 , @steakhal wrote:

> I recommend splitting this into two. I would happily accept the part about 
> `std::data()`.
>
> IMO `std::addressof()` should be modelled via `evalCall` as a pure function, 
> instead of hacking it into the `InnerPtrChecker`.

It does make sense to split these in two, but I'm not so sure about `evalCall`. 
 It is definitely not something that `InnerPtrChecker` should be bothered with. 
 Another point is that `std::addressof` might be only one of the function 
receiving a non-const reference and not modifying it, so it would
be nice to have a place in this checker where such exception is made.

> It is overloaded to accept `const T&` as well, so the comment about 
> //"std::addressof function accepts a non-const reference as an argument"// is 
> not entirely true.

But it has a `T &` overload as well, and this is the one causing the FP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I recommend splitting this into two. I would happily accept the part about 
`std::data()`.

IMO `std::addressof()` should be modelled via `evalCall` as a pure function, 
instead of hacking it into the `InnerPtrChecker`.
It is overloaded to accept `const T&` as well, so the comment about 
//"std::addressof function accepts a non-const reference as an argument"// is 
not entirely true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99260

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

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

[Valeriy Savchenko via Phabricator via cfe-commits]

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

This patch supports std::data and std::addressof functions.

rdar://73463300


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99260

Files:
  clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  clang/test/Analysis/inner-pointer.cpp

Index: clang/test/Analysis/inner-pointer.cpp
===
--- clang/test/Analysis/inner-pointer.cpp
+++ clang/test/Analysis/inner-pointer.cpp
@@ -17,6 +17,11 @@
 string my_string = "default";
 void default_arg(int a = 42, string  = my_string);
 
+template 
+T *addressof(T );
+
+char *data(std::string );
+
 } // end namespace std
 
 void consume(const char *) {}
@@ -273,6 +278,15 @@
   // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
 }
 
+void deref_after_std_data() {
+  const char *c;
+  std::string s;
+  c = std::data(s); // expected-note {{Pointer to inner buffer of 'std::string' obtained here}}
+  s.push_back('c'); // expected-note {{Inner buffer of 'std::string' reallocated by call to 'push_back'}}
+  consume(c);   // expected-warning {{Inner pointer of container used after re/deallocation}}
+  // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
+}
+
 struct S {
   std::string s;
   const char *name() {
@@ -361,8 +375,24 @@
   // expected-note@-1 {{Inner pointer of container used after re/deallocation}}
 }
 
+void func_addressof() {
+  const char *c;
+  std::string s;
+  c = s.c_str();
+  addressof(s);
+  consume(c); // no-warning
+}
+
+void func_std_data() {
+  const char *c;
+  std::string s;
+  c = std::data(s);
+  consume(c); // no-warning
+}
+
 struct T {
   std::string to_string() { return s; }
+
 private:
   std::string s;
 };
Index: clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -34,9 +34,9 @@
 class InnerPointerChecker
 : public Checker {
 
-  CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
-  InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
-  ShrinkToFitFn, SwapFn;
+  CallDescription AppendFn, AssignFn, AddressofFn, ClearFn, CStrFn, DataFn,
+  DataMemberFn, EraseFn, InsertFn, PopBackFn, PushBackFn, ReplaceFn,
+  ReserveFn, ResizeFn, ShrinkToFitFn, SwapFn;
 
 public:
   class InnerPointerBRVisitor : public BugReporterVisitor {
@@ -73,9 +73,10 @@
   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", "basic_string", "data"}),
+CStrFn({"std", "basic_string", "c_str"}), DataFn({"std", "data"}),
+DataMemberFn({"std", "basic_string", "data"}),
 EraseFn({"std", "basic_string", "erase"}),
 InsertFn({"std", "basic_string", "insert"}),
 PopBackFn({"std", "basic_string", "pop_back"}),
@@ -90,6 +91,9 @@
   /// pointers referring to the container object's inner buffer.
   bool isInvalidatingMemberFunction(const CallEvent ) const;
 
+  /// Check whether the called function returns a raw inner pointer.
+  bool isInnerPointerAccessFunction(const CallEvent ) const;
+
   /// Mark pointer symbols associated with the given memory region released
   /// in the program state.
   void markPtrSymbolsReleased(const CallEvent , ProgramStateRef State,
@@ -130,6 +134,12 @@
   Call.isCalled(SwapFn));
 }
 
+bool InnerPointerChecker::isInnerPointerAccessFunction(
+const CallEvent ) const {
+  return (Call.isCalled(CStrFn) || Call.isCalled(DataFn) ||
+  Call.isCalled(DataMemberFn));
+}
+
 void InnerPointerChecker::markPtrSymbolsReleased(const CallEvent ,
  ProgramStateRef State,
  const MemRegion *MR,
@@ -172,6 +182,11 @@
   if (!ArgRegion)
 continue;
 
+  // std::addressof function accepts a non-const reference as an argument,
+  // but doesn't modify it.
+  if (Call.isCalled(AddressofFn))
+continue;
+
   markPtrSymbolsReleased(Call, State, ArgRegion, C);
 }
   }
@@ -195,36 +210,49 @@
 CheckerContext ) const {
   ProgramStateRef State = C.getState();
 
+  // TODO: Do we need these to be typed?
+  const TypedValueRegion *ObjRegion = nullptr;
+
   if (const auto