[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D64256#1626329 , @xazax.hun wrote:

> In D64256#1626279 , @leonardchan 
> wrote:
>
> > In D64256#1626025 , @xazax.hun 
> > wrote:
> >
> > > In D64256#1625998 , @leonardchan 
> > > wrote:
> > >
> > > > Hi. I noticed in our builders that both of the warnings introduced in 
> > > > this patch are being diagnosed for pointers that don't use GSL at all. 
> > > > I'm attempting to make a small reproducer now.
> > >
> > >
> > > Hi!
> > >
> > > Clang now apply GSL annotations for some STL types automatically. So this 
> > > is the expected behavior. Are any of those warnings unwanted/false 
> > > positive with the newest Clang version? If so, please share the 
> > > reproducers and I will either fix them ASAP or revert/turn off the 
> > > warnings.
> >
> >
> > Thanks! I didn't know that they were applied to stl types. This could 
> > explain why the warning is raised in my case (std::string), but I don't 
> > think there's anything wrong in this example that would lead to a warning:
> >
> >   leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat 
> > ~/misc/test.cpp
> >   #include 
> >  
> >   std::string MakeStr();
> >   void other_func(const char *s);
> >  
> >   void func(std::string s) {
> > const char *x = MakeStr().c_str();
> > other_func(x);
> >   }
> >   leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ 
> > ~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
> >   /usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object 
> > backing the pointer will be destroyed at the end of the full-expression 
> > [-Wdangling]
> > const char *x = MakeStr().c_str();
> > ^
> >   1 warning generated.
> >
> >
> > This was made using a release build from ToT clang.
>
>
> Thanks for the repro! I think this is a true positive. `MakeStr`  will create 
> a temporary object, and `c_str()` will create a pointer that points into the 
> buffer owned by the temporary object. At the end of the full expression the 
> temporary object is destroyed and the result of `c_str` will dangle.


Ah you're right! I accidentally glossed over that. This means your patch is 
working as expected then. Thanks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D64256#1626279 , @leonardchan wrote:

> In D64256#1626025 , @xazax.hun wrote:
>
> > In D64256#1625998 , @leonardchan 
> > wrote:
> >
> > > Hi. I noticed in our builders that both of the warnings introduced in 
> > > this patch are being diagnosed for pointers that don't use GSL at all. 
> > > I'm attempting to make a small reproducer now.
> >
> >
> > Hi!
> >
> > Clang now apply GSL annotations for some STL types automatically. So this 
> > is the expected behavior. Are any of those warnings unwanted/false positive 
> > with the newest Clang version? If so, please share the reproducers and I 
> > will either fix them ASAP or revert/turn off the warnings.
>
>
> Thanks! I didn't know that they were applied to stl types. This could explain 
> why the warning is raised in my case (std::string), but I don't think there's 
> anything wrong in this example that would lead to a warning:
>
>   leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat 
> ~/misc/test.cpp
>   #include 
>  
>   std::string MakeStr();
>   void other_func(const char *s);
>  
>   void func(std::string s) {
> const char *x = MakeStr().c_str();
> other_func(x);
>   }
>   leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ 
> ~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
>   /usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object 
> backing the pointer will be destroyed at the end of the full-expression 
> [-Wdangling]
> const char *x = MakeStr().c_str();
> ^
>   1 warning generated.
>
>
> This was made using a release build from ToT clang.


Thanks for the repro! I think this is a true positive. `MakeStr`  will create a 
temporary object, and `c_str()` will create a pointer that points into the 
buffer owned by the temporary object. At the end of the full expression the 
temporary object is destroyed and the result of `c_str` will dangle.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D64256#1626025 , @xazax.hun wrote:

> In D64256#1625998 , @leonardchan 
> wrote:
>
> > Hi. I noticed in our builders that both of the warnings introduced in this 
> > patch are being diagnosed for pointers that don't use GSL at all. I'm 
> > attempting to make a small reproducer now.
>
>
> Hi!
>
> Clang now apply GSL annotations for some STL types automatically. So this is 
> the expected behavior. Are any of those warnings unwanted/false positive with 
> the newest Clang version? If so, please share the reproducers and I will 
> either fix them ASAP or revert/turn off the warnings.


Thanks! I didn't know that they were applied to stl types. This could explain 
why the warning is raised in my case (std::string), but I don't think there's 
anything wrong in this example that would lead to a warning:

  leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat 
~/misc/test.cpp
  #include 
  
  std::string MakeStr();
  void other_func(const char *s);
  
  void func(std::string s) {
const char *x = MakeStr().c_str();
other_func(x);
  }
  leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ 
~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
  /usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object 
backing the pointer will be destroyed at the end of the full-expression 
[-Wdangling]
const char *x = MakeStr().c_str();
^
  1 warning generated.

This was made using a release build from ToT clang.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D64256#1625998 , @leonardchan wrote:

> Hi. I noticed in our builders that both of the warnings introduced in this 
> patch are being diagnosed for pointers that don't use GSL at all. I'm 
> attempting to make a small reproducer now.


Hi!

Clang now apply GSL annotations for some STL types automatically. So this is 
the expected behavior. Are any of those warnings unwanted/false positive with 
the newest Clang version? If so, please share the reproducers and I will either 
fix them ASAP or revert/turn off the warnings.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Hi. I noticed in our builders that both of the warnings introduced in this 
patch are being diagnosed for pointers that don't use GSL at all. I'm 
attempting to make a small reproducer now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7077
+//   someContainer.add(std::move(localOWner));
+//   return p;
+if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) &&

gribozavr wrote:
> xazax.hun wrote:
> > gribozavr wrote:
> > > Why is it a false positive? `std::move` left memory owned by `localOwner` 
> > > in unspecified state.
> > I saw user code relying on the semantics of certain classes. E.g. they 
> > assume if a `std::unique_ptr` is moved the pointee is still in place, so it 
> > is safe to return a reference to the pointee. Do you think those cases 
> > should be diagnosed too?
> It is... debatable. It is not obvious whether the lifetime of the pointed-to 
> memory has ended or not without more detailed lifetime annotations. I think 
> it is fair to silence it, however, I think the comment should be updated to 
> explain the situation in a more detailed way, since without context it looks 
> like a use-after-move.
Do you think renaming `localOwner` to `uniquePtr` would be sufficient or do you 
want me to extend the text too?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7077
+//   someContainer.add(std::move(localOWner));
+//   return p;
+if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) &&

xazax.hun wrote:
> gribozavr wrote:
> > Why is it a false positive? `std::move` left memory owned by `localOwner` 
> > in unspecified state.
> I saw user code relying on the semantics of certain classes. E.g. they assume 
> if a `std::unique_ptr` is moved the pointee is still in place, so it is safe 
> to return a reference to the pointee. Do you think those cases should be 
> diagnosed too?
It is... debatable. It is not obvious whether the lifetime of the pointed-to 
memory has ended or not without more detailed lifetime annotations. I think it 
is fair to silence it, however, I think the comment should be updated to 
explain the situation in a more detailed way, since without context it looks 
like a use-after-move.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368072: Teach some warnings to respect gsl::Pointer and 
gsl::Owner attributes (authored by xazax, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64256?vs=212205=213682#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaInit.cpp
  cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8107,6 +8107,10 @@
   "%select{binds to|is}2 a temporary object "
   "whose lifetime is shorter than the lifetime of the constructed object">,
   InGroup;
+def warn_dangling_lifetime_pointer_member : Warning<
+  "initializing pointer member %0 to point to a temporary object "
+  "whose lifetime is shorter than the lifetime of the constructed object">,
+  InGroup;
 def note_lifetime_extending_member_declared_here : Note<
   "%select{%select{reference|'std::initializer_list'}0 member|"
   "member with %select{reference|'std::initializer_list'}0 subobject}1 "
@@ -8125,6 +8129,10 @@
   "temporary bound to reference member of allocated object "
   "will be destroyed at the end of the full-expression">,
   InGroup;
+def warn_dangling_lifetime_pointer : Warning<
+  "object backing the pointer "
+  "will be destroyed at the end of the full-expression">,
+  InGroup;
 def warn_new_dangling_initializer_list : Warning<
   "array backing "
   "%select{initializer list subobject of the allocated object|"
Index: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7077
+//   someContainer.add(std::move(localOWner));
+//   return p;
+if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) &&

gribozavr wrote:
> Why is it a false positive? `std::move` left memory owned by `localOwner` in 
> unspecified state.
I saw user code relying on the semantics of certain classes. E.g. they assume 
if a `std::unique_ptr` is moved the pointee is still in place, so it is safe to 
return a reference to the pointee. Do you think those cases should be diagnosed 
too?


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7031
+  LLVM_FALLTHROUGH;
+case IndirectLocalPathEntry::DefaultInit:
   return Path[I].E->getSourceRange();

This change would be best committed separately (preferably with a test).



Comment at: clang/lib/Sema/SemaInit.cpp:7046
+  }
+  return GslPointerInits;
+}

I think you can go back to llvm::c_find now?



Comment at: clang/lib/Sema/SemaInit.cpp:7076
+//   int  = *localOwner;
+//   someContainer.add(std::move(localOWner));
+//   return p;

"Owner"



Comment at: clang/lib/Sema/SemaInit.cpp:7077
+//   someContainer.add(std::move(localOWner));
+//   return p;
+if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) &&

Why is it a false positive? `std::move` left memory owned by `localOwner` in 
unspecified state.


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Thanks for the review!
Since I did some refactoring I will wait for an additional accept before 
committing.


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7095
   auto *MTE = dyn_cast(L);
+  if (IsGslPtrInitWithGslTempOwner) {
+Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange;

gribozavr wrote:
> It is unclear to me why `if (IsGslPtrInitWithGslTempOwner)` is before `if 
> (!MTE)`, seems like that exclusion should apply to our case, too.
The reason was that I always skipped MTEs and was looking for the expr that 
actually initialized MTEs. Relying on MTEs, however, turned out to simplify the 
code a bit. Updating the diff, thanks!


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 212205.
xazax.hun marked an inline comment as done.
xazax.hun added a comment.

- Actually move the code snippet in question.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyIntPointer returningLocalPointer() {
+  MyIntPointer localPointer;
+  return localPointer; // ok
+}
+
+MyIntPointer daglingGslPtrFromLocalOwner() {
+  MyIntOwner localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() {
+  MyLongOwnerWithConversion localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyIntPointer danglingGslPtrFromTemporary() {
+  return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+int *noFalsePositive(MyIntOwner ) {
+  MyIntPointer p = o;
+  return &*p; // ok
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 212204.
xazax.hun marked an inline comment as done.
xazax.hun added a comment.

- A small refactoring based on an observation from a review comment.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyIntPointer returningLocalPointer() {
+  MyIntPointer localPointer;
+  return localPointer; // ok
+}
+
+MyIntPointer daglingGslPtrFromLocalOwner() {
+  MyIntOwner localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() {
+  MyLongOwnerWithConversion localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyIntPointer danglingGslPtrFromTemporary() {
+  return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+int *noFalsePositive(MyIntOwner ) {
+  MyIntPointer p = o;
+  return &*p; // ok
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7095
   auto *MTE = dyn_cast(L);
+  if (IsGslPtrInitWithGslTempOwner) {
+Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange;

It is unclear to me why `if (IsGslPtrInitWithGslTempOwner)` is before `if 
(!MTE)`, seems like that exclusion should apply to our case, too.


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 211580.
xazax.hun added a comment.

- Fix a false positive case found by running over ~200 open source projects


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyIntPointer returningLocalPointer() {
+  MyIntPointer localPointer;
+  return localPointer; // ok
+}
+
+MyIntPointer daglingGslPtrFromLocalOwner() {
+  MyIntOwner localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() {
+  MyLongOwnerWithConversion localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyIntPointer danglingGslPtrFromTemporary() {
+  return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+int *noFalsePositive(MyIntOwner ) {
+  MyIntPointer p = o;
+  return &*p; // ok
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 211150.
xazax.hun added a comment.

- Fix a false positive from previous change.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,130 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyIntPointer returningLocalPointer() {
+  MyIntPointer localPointer;
+  return localPointer; // ok
+}
+
+MyIntPointer daglingGslPtrFromLocalOwner() {
+  MyIntOwner localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() {
+  MyLongOwnerWithConversion localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyIntPointer danglingGslPtrFromTemporary() {
+  return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p2 = MyLongOwnerWithConversion{}; // TODO ?
+  

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 211142.
xazax.hun added a comment.

- Fix a TODO and add some more tests.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyIntPointer daglingGslPtrFromLocalOwner() {
+  MyIntOwner localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() {
+  MyLongOwnerWithConversion localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyIntPointer danglingGslPtrFromTemporary() {
+  return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p2 = MyLongOwnerWithConversion{}; // TODO ?
+  global2 = MyLongOwnerWithConversion{}; // TODO ?
+}
+
+struct IntVector {
+  int *begin();
+  int *end();
+};
+

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 211127.
xazax.hun marked 5 inline comments as done.
xazax.hun added a comment.

- Address the rest of the review comments.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,116 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwner() {
+  MyLongOwnerWithConversion t;
+  return t; // TODO
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporary() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p2 = MyLongOwnerWithConversion{}; // TODO ?
+  global2 = MyLongOwnerWithConversion{}; // TODO ?
+}
+
+struct IntVector {
+  int *begin();
+  int *end();
+};
+
+void modelIterators() {
+  int *it = IntVector{}.begin(); // TODO ?
+  (void)it;
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6507,6 +6507,8 @@
 VarInit,
 LValToRVal,
 LifetimeBoundCall,
+GslPointerInit,
+GslOwnerTemporaryInit
   } Kind;
   Expr *E;
   const Decl *D 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:25
+
+struct [[gsl::Owner(int)]] OwnerWithConv {
+  OwnerWithConv();

xazax.hun wrote:
> gribozavr wrote:
> > Would be nice if the second pair of types had clearly related names, like 
> > the first pair (MyOwner/MyPointer).
> Would renaming `MyPtrFromConv` to `PointerFromConv ` be enough to suggest the 
> relationship?
Yes, that would be sufficient. However my best suggestion would be:

MyOwner => MyIntOwner
MyPointer => MyIntPointer

OwnerWithConv => MyLongOwnerWithConversion
MyPtrFromConv => MyLongPointerFromConversion

Also makes it clear which two types are related together.


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added a comment.

Thanks for the review! I will update the patch soon. As there is a dependency 
that is not accepted yet Richard (who authored the code I extended) might have 
some chance to take a look at this patch.




Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:25
+
+struct [[gsl::Owner(int)]] OwnerWithConv {
+  OwnerWithConv();

gribozavr wrote:
> Would be nice if the second pair of types had clearly related names, like the 
> first pair (MyOwner/MyPointer).
Would renaming `MyPtrFromConv` to `PointerFromConv ` be enough to suggest the 
relationship?


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

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

I'm not an expert in SemaInit code, but this change LGTM.




Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:8
+
+struct OwnerWithConv;
+

Not needed anymore.



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:25
+
+struct [[gsl::Owner(int)]] OwnerWithConv {
+  OwnerWithConv();

Would be nice if the second pair of types had clearly related names, like the 
first pair (MyOwner/MyPointer).


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 208709.
xazax.hun added a comment.

- Address review comments.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,117 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyOwner {
+  MyOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct OwnerWithConv;
+
+struct [[gsl::Pointer(int)]] MyPointer {
+  MyPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyPointer(const MyOwner &);
+  int *();
+  MyOwner toOwner();
+};
+
+struct [[gsl::Pointer(int)]] MyPtrFromConv {
+  MyPtrFromConv(int *p = nullptr);
+  int *();
+};
+
+struct [[gsl::Owner(int)]] OwnerWithConv {
+  OwnerWithConv();
+  operator MyPtrFromConv();
+  int *();
+  MyPointer releaseAsMyPointer();
+  int *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyPtrFromConv(OwnerWithConv{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyPointer(MyOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyPointer(MyPointer{p});
+}
+
+MyPointer ownershipTransferToMyPointer() {
+  OwnerWithConv t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+int *ownershipTransferToRawPointer() {
+  OwnerWithConv t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyPointer p; // expected-note 2{{pointer member declared here}}
+  MyPtrFromConv p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(OwnerWithConv{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyPtrFromConv daglingGslPtrFromLocalOwner() {
+  OwnerWithConv t;
+  return t; // TODO
+}
+
+MyPtrFromConv danglingGslPtrFromTemporary() {
+  return OwnerWithConv{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyPointer global;
+MyPtrFromConv global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyPointer p = MyOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyOwner{}; // TODO ?
+  global = MyOwner{}; // TODO ?
+  MyPtrFromConv p2 = OwnerWithConv{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p2 = OwnerWithConv{}; // TODO ?
+  global2 = OwnerWithConv{}; // TODO ?
+}
+
+struct IntVector {
+  int *begin();
+  int *end();
+};
+
+void modelIterators() {
+  int *it = IntVector{}.begin(); // TODO ?
+  (void)it;
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6507,6 +6507,8 @@
 VarInit,
 LValToRVal,
 LifetimeBoundCall,
+GslPointerInit,
+GslOwnerTemporaryInit
   } Kind;
   Expr *E;
   const Decl *D = nullptr;
@@ -6551,6 +6553,47 @@
   Expr *Init, ReferenceKind RK,
   LocalVisitor Visit);
 
+template
+static bool isRecordWithAttr(QualType Type) {
+  if (auto *RD = Type->getAsCXXRecordDecl())
+return RD->getCanonicalDecl()->hasAttr();
+  return false;
+}
+
+static void 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 6 inline comments as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7076
+auto Prefix = llvm::makeArrayRef(Path).drop_back();
+  if (pathInitializeLifetimePointer(Prefix))
+IsLifetimePtrInitWithTempOwner = true;

gribozavr wrote:
> Is it important that the whole path only contains gsl::Pointer nodes?
We are trying to figure out what do we initialize a `gsl::Pointer` annotated 
class. The reason why we have multiple `GslPointerInit`s in the path is that we 
usually can see a lot of intermediate temporary objects. Looking at the AST of 
the examples I have so far the other kind of paths does not seem to be 
relevant. I can imagine refining this condition later after examining a false 
negative but I think starting with the most conservative approach should be 
reasonable for warnings. 


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6549
+static bool
+pathInitializeLifetimePointer(llvm::ArrayRef Path) {
+  return Path.size() > 0 && llvm::all_of(Path, [=](IndirectLocalPathEntry E) {

Move closer to the point of usage?



Comment at: clang/lib/Sema/SemaInit.cpp:6549
+static bool
+pathInitializeLifetimePointer(llvm::ArrayRef Path) {
+  return Path.size() > 0 && llvm::all_of(Path, [=](IndirectLocalPathEntry E) {

gribozavr wrote:
> Move closer to the point of usage?
re: name, `pathOnlyContainsGslPointerInit` or `pathOnlyInitializesGslPointer`



Comment at: clang/lib/Sema/SemaInit.cpp:6564
+template
+static bool recordHasAttr(QualType Type) {
+  if (auto *RD = Type->getAsCXXRecordDecl())

"isRecordWithAttr" ?



Comment at: clang/lib/Sema/SemaInit.cpp:7072
+
+bool IsLifetimePtrInitWithTempOwner = false;
+if (!Path.empty() &&

Please adjust the name.



Comment at: clang/lib/Sema/SemaInit.cpp:7076
+auto Prefix = llvm::makeArrayRef(Path).drop_back();
+  if (pathInitializeLifetimePointer(Prefix))
+IsLifetimePtrInitWithTempOwner = true;

Is it important that the whole path only contains gsl::Pointer nodes?



Comment at: clang/lib/Sema/SemaInit.cpp:6510
 LifetimeBoundCall,
+LifetimePointerInit,
+LifetimeTempOwner

xazax.hun wrote:
> gribozavr wrote:
> > What is this name supposed to mean? Initialization of a "lifetime pointer"? 
> > What's a "lifetime pointer"?
> > 
> > If you were imitating "LifetimeBoundCall", it is a reference to the 
> > attribute: 
> > https://clang.llvm.org/docs/AttributeReference.html#lifetimebound .
> When we were working on the lifetime analysis we needed to distinguish raw 
> pointers from the user defined types that are categorized as pointers. We 
> adopted this notion of Lifetime Pointer which stands for the user defined 
> type that is annotated with gsl::Pointer.
> 
> In case this is confusing I could rename it `GslPointerInit` and 
> `GslTempOwner`. What do you think?
My suggestion is to refer to a type that was annotated with gsl::Pointer as a 
"gsl::Pointer type" in prose, or "GslPointer" where you need an identifier.

So I agree about `GslPointerInit`.

As for the second one, it seems like `GslOwnerTemporaryInit` would be more 
fitting -- WDYT? It is an initialization of a temporary that has a gsl::Owner 
type.



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:16
+
+struct [[gsl::Owner(int)]] T {
+  T();

xazax.hun wrote:
> gribozavr wrote:
> > Can `T` and `MyOwner` be the same type?
> > 
> > It is confusing to have two -- for example, `toOwner()` returning `T` 
> > instead of `MyOwner` is confusing.
> I agree that T might not be a great name. The reason why we have two types 
> because one can be converted to a Pointer using a conversion operator the 
> other can be converted using a one argument non-explicit constructor. These 
> two are generating different ASTs, thus I wanted to test both ways. 
Please explain this distinction in the comments in the test. If you didn't tell 
me just now, I would never know.

Maybe it is also a good idea to split the MyPointer type then, let each owner 
have its own pointer. Right now having one pointer with two owners is 
confusing, it suggests that owners are somehow related, but they aren't.


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 208441.
xazax.hun added a comment.

- Fix a typo.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,108 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyOwner {
+  MyOwner();
+  int *();
+};
+
+struct OwnerWithConv;
+
+// Conversion operator and constructor conversion will result in two
+// different shaped ASTs. Thus we have two owner types in the tests.
+struct [[gsl::Pointer(int)]] MyPointer {
+  MyPointer(int *p = nullptr);
+  MyPointer(const MyOwner &);
+  int *();
+  OwnerWithConv toOwner();
+};
+
+struct [[gsl::Owner(int)]] OwnerWithConv {
+  OwnerWithConv();
+  operator MyPointer();
+  int *();
+  MyPointer releaseAsMyPointer();
+  int *releaseAsRawPointer();
+  int *c_str() const;
+};
+
+void danglingHeapObject() {
+  new MyPointer(OwnerWithConv{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyPointer(MyOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyPointer(MyPointer{p});
+}
+
+MyPointer ownershipTransferToMyPointer() {
+  OwnerWithConv t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+int *ownershipTransferToRawPointer() {
+  OwnerWithConv t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  OwnerWithConv t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyPointer p; // expected-note 3{{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p(OwnerWithConv{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyPointer daglingGslPtrFromLocalOwner() {
+  OwnerWithConv t;
+  return t; // TODO
+}
+
+MyPointer danglingGslPtrFromTemporary() {
+  return OwnerWithConv{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyPointer global;
+
+void initLocalGslPtrWithTempOwner() {
+  MyPointer p = MyOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyOwner{}; // TODO ?
+  global = MyOwner{}; // TODO ?
+  p = OwnerWithConv{}; // TODO ?
+  global = OwnerWithConv{}; // TODO ?
+}
+
+struct IntVector {
+  int *begin();
+  int *end();
+};
+
+void modelIterators() {
+  int *it = IntVector{}.begin(); // TODO ?
+  (void)it;
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6507,6 +6507,8 @@
 VarInit,
 LValToRVal,
 LifetimeBoundCall,
+GslPointerInit,
+GslTempOwner
   } Kind;
   Expr *E;
   const Decl *D = nullptr;
@@ -6543,6 +6545,13 @@
   });
 }
 
+static bool
+pathInitializeLifetimePointer(llvm::ArrayRef Path) {
+  return Path.size() > 0 && llvm::all_of(Path, [=](IndirectLocalPathEntry E) {
+return E.Kind == IndirectLocalPathEntry::GslPointerInit;
+  });
+}
+
 static void visitLocalsRetainedByInitializer(IndirectLocalPath ,
  Expr *Init, LocalVisitor Visit,
  bool RevisitSubinits);
@@ -6551,6 +6560,47 @@
   Expr *Init, ReferenceKind RK,
   LocalVisitor Visit);
 
+template
+static bool recordHasAttr(QualType Type) {
+  if (auto *RD = Type->getAsCXXRecordDecl())
+return 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 208439.
xazax.hun marked 12 inline comments as done.
xazax.hun added a comment.

- Address review comments.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,108 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyOwner {
+  MyOwner();
+  int *();
+};
+
+struct OwnerWithConv;
+
+// Conversion operator and constructor conversion will result in two
+// different shaped ASTs. Thus we have two owner types in the tests.
+struct [[gsl::Pointer(int)]] MyPointer {
+  MyPointer(int *p = nullptr);
+  MyPointer(const MyOwner &);
+  int *();
+  OwnerWithConv toOwner();
+};
+
+struct [[gsl::Owner(int)]] OwnerWithConv {
+  OwnerWithConv();
+  operator MyPointer();
+  int *();
+  MyPointer releaseAsMyPointer();
+  int *releaseAsRawPointer();
+  int *c_str() const;
+};
+
+void danglingHeapObject() {
+  new MyPointer(OwnerWithConv{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyPointer(MyOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyPointer(MyPointer{p});
+}
+
+MyPointer ownershipTransferToMyPointer() {
+  OwnerWithConv t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+int *ownershipTransferToRawPointer() {
+  OwnerWithConv t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  OwnerWithConv t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyPointer p; // expected-note 3{{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p(OwnerWithConv{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyPointer daglingGslPtrFromLocalOwner() {
+  OwnerWithConv t;
+  return t; // TODO
+}
+
+MyPointer danglingGslPtrFromTemporary() {
+  return OwnerWithConv{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyPointer global;
+
+void initLocalGslPtrWithTempOwner() {
+  MyPointer p = MyOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyOwner{}; // TODO ?
+  global = MyOwner{}; // TODO ?
+  p = OwnerWithConv{}; // TODO ?
+  global = OwnerWithConv{}; // TODO ?
+}
+
+struct IntVector {
+  int *begin();
+  int *end();
+};
+
+void modelIterators() {
+  int *it = IntVector{}.begin(); // TODO ?
+  (void)it;
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6507,6 +6507,8 @@
 VarInit,
 LValToRVal,
 LifetimeBoundCall,
+GslPointerInit,
+GslTempPointer
   } Kind;
   Expr *E;
   const Decl *D = nullptr;
@@ -6543,6 +6545,13 @@
   });
 }
 
+static bool
+pathInitializeLifetimePointer(llvm::ArrayRef Path) {
+  return Path.size() > 0 && llvm::all_of(Path, [=](IndirectLocalPathEntry E) {
+return E.Kind == IndirectLocalPathEntry::GslPointerInit;
+  });
+}
+
 static void visitLocalsRetainedByInitializer(IndirectLocalPath ,
  Expr *Init, LocalVisitor Visit,
  bool RevisitSubinits);
@@ -6551,6 +6560,47 @@
   Expr *Init, ReferenceKind RK,
   LocalVisitor Visit);
 
+template
+static bool recordHasAttr(QualType Type) {
+  if (auto 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:22
+  int *release2();
+  int *c_str() const;
+};

gribozavr wrote:
> This method is confusing -- is it a name that the warning is supposed to know 
> about?
Not yet, see my other answer below.


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 5 inline comments as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6510
 LifetimeBoundCall,
+LifetimePointerInit,
+LifetimeTempOwner

gribozavr wrote:
> What is this name supposed to mean? Initialization of a "lifetime pointer"? 
> What's a "lifetime pointer"?
> 
> If you were imitating "LifetimeBoundCall", it is a reference to the 
> attribute: https://clang.llvm.org/docs/AttributeReference.html#lifetimebound .
When we were working on the lifetime analysis we needed to distinguish raw 
pointers from the user defined types that are categorized as pointers. We 
adopted this notion of Lifetime Pointer which stands for the user defined type 
that is annotated with gsl::Pointer.

In case this is confusing I could rename it `GslPointerInit` and 
`GslTempOwner`. What do you think?



Comment at: clang/lib/Sema/SemaInit.cpp:7049
 
+// Skipping a chain of initializing lifetime Pointer objects.
+// We are looking only for the final value to find out if it was

gribozavr wrote:
> Sorry, I can't parse this: "... initializing lifetime Pointer ..."
We called gsl::Pointer annotated types lifetime pointers (to distinguish from 
raw pointers). Should we use gsl::Pointer in the comments too? Or do you have 
an alternative in mind?



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:16
+
+struct [[gsl::Owner(int)]] T {
+  T();

gribozavr wrote:
> Can `T` and `MyOwner` be the same type?
> 
> It is confusing to have two -- for example, `toOwner()` returning `T` instead 
> of `MyOwner` is confusing.
I agree that T might not be a great name. The reason why we have two types 
because one can be converted to a Pointer using a conversion operator the other 
can be converted using a one argument non-explicit constructor. These two are 
generating different ASTs, thus I wanted to test both ways. 



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:33
+  MyPointer p{}; // ok
+  new MyPointer(MyPointer{p}); // ok
+}

gribozavr wrote:
> Why is the last line OK? Looks like a false negative to me -- the pointer on 
> the heap is pointing to a local variable, just like in `f`, which produces a 
> warning.
> 
> If it is an intentional false negative, please annotate it as such in 
> comments.
Yeah, this is an intentional false negative as we do not have enough 
information in a statement local analysis to warn about this case. Will fix the 
comment thanks!



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:38
+  T t;
+  return t.release(); // ok
+}

gribozavr wrote:
> Is "release" a magic name that the warning understands?
Method calls are not handled yet, but the idea is to understand some STL 
specific methods, like the `std::unique_ptr::release`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6510
 LifetimeBoundCall,
+LifetimePointerInit,
+LifetimeTempOwner

What is this name supposed to mean? Initialization of a "lifetime pointer"? 
What's a "lifetime pointer"?

If you were imitating "LifetimeBoundCall", it is a reference to the attribute: 
https://clang.llvm.org/docs/AttributeReference.html#lifetimebound .



Comment at: clang/lib/Sema/SemaInit.cpp:6587
+
 static void visitLifetimeBoundArguments(IndirectLocalPath , Expr *Call,
 LocalVisitor Visit) {

I feel like the code would be better off if we defined another function for 
handling gsl::Pointer semantics.

This function is for clang::lifetimebound, and mixing the logic is confusing 
IMO.



Comment at: clang/lib/Sema/SemaInit.cpp:7049
 
+// Skipping a chain of initializing lifetime Pointer objects.
+// We are looking only for the final value to find out if it was

Sorry, I can't parse this: "... initializing lifetime Pointer ..."



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:10
+struct [[gsl::Pointer(int)]] MyPointer {
+  MyPointer(int *p = 0);
+  MyPointer(const MyOwner &);

`= nullptr`



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:16
+
+struct [[gsl::Owner(int)]] T {
+  T();

Can `T` and `MyOwner` be the same type?

It is confusing to have two -- for example, `toOwner()` returning `T` instead 
of `MyOwner` is confusing.



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:20
+  int *();
+  MyPointer release();
+  int *release2();

"ReleaseAsMyPointer'



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:21
+  MyPointer release();
+  int *release2();
+  int *c_str() const;

"ReleaseAsRawPointer"



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:22
+  int *release2();
+  int *c_str() const;
+};

This method is confusing -- is it a name that the warning is supposed to know 
about?



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:25
+
+void f() {
+  new MyPointer(T{}); // expected-warning {{object backing the pointer will be 
destroyed at the end of the full-expression}}

It would be helpful to rename this test function and other functions below 
according to what they test.

`f`, `g`, `g2`, `i`, `i2` -- unclear what the test is supposed to be about, and 
why tests are grouped like that.



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:33
+  MyPointer p{}; // ok
+  new MyPointer(MyPointer{p}); // ok
+}

Why is the last line OK? Looks like a false negative to me -- the pointer on 
the heap is pointing to a local variable, just like in `f`, which produces a 
warning.

If it is an intentional false negative, please annotate it as such in comments.



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:38
+  T t;
+  return t.release(); // ok
+}

Is "release" a magic name that the warning understands?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64256



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