[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-06-04 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC333941: [CFG] Fix automatic destructors when a member is 
bound to a reference. (authored by dergachev, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D44238

Files:
  lib/Analysis/CFG.cpp
  test/Analysis/auto-obj-dtors-cfg-output.cpp

Index: test/Analysis/auto-obj-dtors-cfg-output.cpp
===
--- test/Analysis/auto-obj-dtors-cfg-output.cpp
+++ test/Analysis/auto-obj-dtors-cfg-output.cpp
@@ -1,7 +1,11 @@
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
-// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,WARNINGS %s
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
-// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,ANALYZER %s
+// RUN: %clang_analyze_cc1 -std=c++98 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX98,WARNINGS,CXX98-WARNINGS %s
+// RUN: %clang_analyze_cc1 -std=c++98 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX98,ANALYZER,CXX98-ANALYZER %s
+// RUN: %clang_analyze_cc1 -std=c++11 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,WARNINGS,CXX11-WARNINGS %s
+// RUN: %clang_analyze_cc1 -std=c++11 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,ANALYZER,CXX11-ANALYZER %s
 
 // This file tests how we construct two different flavors of the Clang CFG -
 // the CFG used by the Sema analysis-based warnings and the CFG used by the
@@ -14,6 +18,8 @@
 
 class A {
 public:
+  int x;
+
 // CHECK:  [B1 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B0
 // CHECK:  [B0 (EXIT)]
@@ -70,6 +76,287 @@
 // CHECK:  [B2 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B1
 // CHECK:  [B1]
+// WARNINGS-NEXT:   1: A() (CXXConstructExpr, class A)
+// CXX98-ANALYZER-NEXT:   1: A() (CXXConstructExpr, [B1.2], class A)
+// CXX11-ANALYZER-NEXT:   1: A() (CXXConstructExpr, [B1.2], [B1.3], class A)
+// CHECK-NEXT:   2: [B1.1] (BindTemporary)
+// CXX98-NEXT:   3: [B1.2].x
+// CXX98-NEXT:   4: [B1.3]
+// CXX98-NEXT:   5: const int &x = A().x;
+// CXX98-NEXT:   6: [B1.5].~A() (Implicit destructor)
+// CXX11-NEXT:   3: [B1.2]
+// CXX11-NEXT:   4: [B1.3].x
+// CXX11-NEXT:   5: [B1.4] (ImplicitCastExpr, NoOp, const int)
+// CXX11-NEXT:   6: const int &x = A().x;
+// CXX11-NEXT:   7: [B1.6].~A() (Implicit destructor)
+// CHECK-NEXT:   Preds (1): B2
+// CHECK-NEXT:   Succs (1): B0
+// CHECK:  [B0 (EXIT)]
+// CHECK-NEXT:   Preds (1): B1
+void test_const_ref_to_field() {
+  const int &x = A().x;
+}
+
+// CHECK:[B2 (ENTRY)]
+// CHECK-NEXT: Succs (1): B1
+// CHECK:[B1]
+// WARNINGS-NEXT: 1: A() (CXXConstructExpr, class A)
+// CXX98-ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], class A)
+// CXX11-ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], [B1.3], class A)
+// CHECK-NEXT: 2: [B1.1] (BindTemporary)
+// CXX98-NEXT: 3: A::x
+// CXX98-NEXT: 4: &[B1.3]
+// CXX98-NEXT: 5: [B1.2] .* [B1.4]
+// CXX98-NEXT: 6: [B1.5]
+// CXX98-NEXT: 7: const int &x = A() .* &A::x;
+// CXX98-NEXT: 8: [B1.7].~A() (Implicit destructor)
+// CXX11-NEXT: 3: [B1.2]
+// CXX11-NEXT: 4: A::x
+// CXX11-NEXT: 5: &[B1.4]
+// CXX11-NEXT: 6: [B1.3] .* [B1.5]
+// CXX11-NEXT: 7: [B1.6] (ImplicitCastExpr, NoOp, const int)
+// CXX11-NEXT: 8: const int &x = A() .* &A::x;
+// CXX11-NEXT: 9: [B1.8].~A() (Implicit destructor)
+// CHECK-NEXT: Preds (1): B2
+// CHECK-NEXT: Succs (1): B0
+// CHECK:[B0 (EXIT)]
+// CHECK-NEXT: Preds (1): B1
+void test_pointer_to_member() {
+  const int &x = A().*&A::x;
+}
+
+// FIXME: There should be automatic destructors at the end of scope.
+// CHECK:[B2 (ENTRY)]
+// CHECK-NEXT: Succs (1): B1
+// CHECK:[B1]
+// WARNINGS-NEXT: 1: A() (CXXConstructExpr, class A)
+// ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], [B1.4], class A)
+// CHECK-NEXT: 2: [B1.1] (BindTemporary)
+// CHECK-NEXT: 3: [B1.2] (ImplicitCastExpr, NoOp, const class A)
+// CHECK-NEXT: 4: [B1.3]
+// CHECK-NEXT: 5: {[B1.4]}
+// CHECK-NEXT: 6: B b = {A()};
+// WARNINGS-NEXT: 7: A() (CXXConstructExpr, class A)
+// ANALYZER-NEXT: 7: A() (CXXConstructExpr, [B1.8], [B1.10], class A)
+// CHECK-NEXT: 8: [B1.7] (BindTemporary)
+// CHECK-N

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-05-30 Thread George Karpenkov via Phabricator via cfe-commits 
george.karpenkov accepted this revision.
george.karpenkov added a comment.
This revision is now accepted and ready to land.

I'm not an expert in CFG construction, but looks good to me.


https://reviews.llvm.org/D44238



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

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-05-18 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ updated this revision to Diff 147635.
NoQ added a comment.

Switch to `skipRValueSubobjectAdjustments`. Yup, that's much better.

Add FIXME tests for lifetime extension through non-references that are still 
not working - that correspond to a nearby FIXME in the code. I'm not planning 
to fix them immediately, but it's nice to know what else isn't working in this 
realm.


https://reviews.llvm.org/D44238

Files:
  lib/Analysis/CFG.cpp
  test/Analysis/auto-obj-dtors-cfg-output.cpp

Index: test/Analysis/auto-obj-dtors-cfg-output.cpp
===
--- test/Analysis/auto-obj-dtors-cfg-output.cpp
+++ test/Analysis/auto-obj-dtors-cfg-output.cpp
@@ -1,7 +1,11 @@
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
-// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,WARNINGS %s
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
-// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,ANALYZER %s
+// RUN: %clang_analyze_cc1 -std=c++98 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX98,WARNINGS,CXX98-WARNINGS %s
+// RUN: %clang_analyze_cc1 -std=c++98 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX98,ANALYZER,CXX98-ANALYZER %s
+// RUN: %clang_analyze_cc1 -std=c++11 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,WARNINGS,CXX11-WARNINGS %s
+// RUN: %clang_analyze_cc1 -std=c++11 -fcxx-exceptions -fexceptions -analyzer-checker=debug.DumpCFG -analyzer-config cfg-rich-constructors=true %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,ANALYZER,CXX11-ANALYZER %s
 
 // This file tests how we construct two different flavors of the Clang CFG -
 // the CFG used by the Sema analysis-based warnings and the CFG used by the
@@ -14,6 +18,8 @@
 
 class A {
 public:
+  int x;
+
 // CHECK:  [B1 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B0
 // CHECK:  [B0 (EXIT)]
@@ -70,6 +76,287 @@
 // CHECK:  [B2 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B1
 // CHECK:  [B1]
+// WARNINGS-NEXT:   1: A() (CXXConstructExpr, class A)
+// CXX98-ANALYZER-NEXT:   1: A() (CXXConstructExpr, [B1.2], class A)
+// CXX11-ANALYZER-NEXT:   1: A() (CXXConstructExpr, [B1.2], [B1.3], class A)
+// CHECK-NEXT:   2: [B1.1] (BindTemporary)
+// CXX98-NEXT:   3: [B1.2].x
+// CXX98-NEXT:   4: [B1.3]
+// CXX98-NEXT:   5: const int &x = A().x;
+// CXX98-NEXT:   6: [B1.5].~A() (Implicit destructor)
+// CXX11-NEXT:   3: [B1.2]
+// CXX11-NEXT:   4: [B1.3].x
+// CXX11-NEXT:   5: [B1.4] (ImplicitCastExpr, NoOp, const int)
+// CXX11-NEXT:   6: const int &x = A().x;
+// CXX11-NEXT:   7: [B1.6].~A() (Implicit destructor)
+// CHECK-NEXT:   Preds (1): B2
+// CHECK-NEXT:   Succs (1): B0
+// CHECK:  [B0 (EXIT)]
+// CHECK-NEXT:   Preds (1): B1
+void test_const_ref_to_field() {
+  const int &x = A().x;
+}
+
+// CHECK:[B2 (ENTRY)]
+// CHECK-NEXT: Succs (1): B1
+// CHECK:[B1]
+// WARNINGS-NEXT: 1: A() (CXXConstructExpr, class A)
+// CXX98-ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], class A)
+// CXX11-ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], [B1.3], class A)
+// CHECK-NEXT: 2: [B1.1] (BindTemporary)
+// CXX98-NEXT: 3: A::x
+// CXX98-NEXT: 4: &[B1.3]
+// CXX98-NEXT: 5: [B1.2] .* [B1.4]
+// CXX98-NEXT: 6: [B1.5]
+// CXX98-NEXT: 7: const int &x = A() .* &A::x;
+// CXX98-NEXT: 8: [B1.7].~A() (Implicit destructor)
+// CXX11-NEXT: 3: [B1.2]
+// CXX11-NEXT: 4: A::x
+// CXX11-NEXT: 5: &[B1.4]
+// CXX11-NEXT: 6: [B1.3] .* [B1.5]
+// CXX11-NEXT: 7: [B1.6] (ImplicitCastExpr, NoOp, const int)
+// CXX11-NEXT: 8: const int &x = A() .* &A::x;
+// CXX11-NEXT: 9: [B1.8].~A() (Implicit destructor)
+// CHECK-NEXT: Preds (1): B2
+// CHECK-NEXT: Succs (1): B0
+// CHECK:[B0 (EXIT)]
+// CHECK-NEXT: Preds (1): B1
+void test_pointer_to_member() {
+  const int &x = A().*&A::x;
+}
+
+// FIXME: There should be automatic destructors at the end of scope.
+// CHECK:[B2 (ENTRY)]
+// CHECK-NEXT: Succs (1): B1
+// CHECK:[B1]
+// WARNINGS-NEXT: 1: A() (CXXConstructExpr, class A)
+// ANALYZER-NEXT: 1: A() (CXXConstructExpr, [B1.2], [B1.4], class A)
+// CHECK-NEXT: 2: [B1.1] (BindTemporary)
+// CHECK-NEXT: 3: [B1.2] (ImplicitCastExpr, NoOp, const class A)
+// CHECK-NEXT: 4: [B1.3]
+// CHECK-NEXT: 5: {[B1.4]}
+// CHECK-NEXT: 6: B b = {A()};
+// WARNINGS-NEXT: 7: A() (CXXConstructExpr,

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-04-04 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: lib/Analysis/CFG.cpp:1435
 /// extended by a local reference with the given initializer.
 static QualType getReferenceInitTemporaryType(ASTContext &Context,
   const Expr *Init,

rsmith wrote:
> NoQ wrote:
> > rsmith wrote:
> > > Can you replace this with `Expr::skipRValueSubobjectAdjustments`? That's 
> > > how we compute this elsewhere. (You'll need to skip a top-level 
> > > `ExprWithCleanups` and check for the search terminating in a 
> > > `MaterializeTemporaryExpr` yourself, but this will reduce duplication and 
> > > fix a couple more cases this function is getting wrong).
> > Hmm, right, ok, yeah. I'm in no rush with this one, so i'll spend some time 
> > trying to understand what //rvalue// adjustments do we still have after 
> > rC288563.
> We still have to deal with rvalue adjustments in C++98 compilations, because 
> we don't create xvalue `MaterializeTemporaryExpr` nodes there (because C++98 
> doesn't have xvalues). I think if/when we carry out the plan to remove 
> `CXXBindTemporaryExpr`, skipping rvalue adjustments will only be interesting 
> to the code that performs lifetime extension, and everything else can just 
> deal with the `MaterializeTemporaryExpr` nodes. But for now, you'll need to 
> skip rvalue adjustments if you want to handle our C++98 ASTs.
> 
> As a quick example, take:
> 
> ```
> struct A { int n; };
> const int &r = A().*&A::n;
> ```
> 
> ... which produces this AST in C++11 onwards:
> 
> ```
> `-VarDecl 0xc2681f0  col:33 r 'const int &' cinit
>   `-ExprWithCleanups 0xc268b28  'const int' xvalue
> `-ImplicitCastExpr 0xc2689f0  'const int' xvalue 
>   `-BinaryOperator 0xc2689c8  'int' xvalue '.*'
> |-MaterializeTemporaryExpr 0xc2689b0  'A' xvalue 
> extended by Var 0xc2681f0 'r' 'const int &'
> | `-CXXTemporaryObjectExpr 0xc2687b0  'A' 'void () 
> noexcept' zeroing
> `-UnaryOperator 0xc268990  'int A::*' prefix '&' 
> cannot overflow
>   `-DeclRefExpr 0xc268928  'int' lvalue Field 
> 0xc268148 'n' 'int'
> ```
> 
> ... but produces this in C++98 mode (which I think this function will 
> mishandle because it doesn't know how to look through the binary `.*` 
> operator):
> 
> ```
> `-VarDecl 0xcd61c90  col:33 r 'const int &' cinit
>   `-ExprWithCleanups 0xcd62398  'const int' lvalue
> `-MaterializeTemporaryExpr 0xcd622a8  'const int' lvalue 
> extended by Var 0xcd61c90 'r' 'const int &'
>   `-BinaryOperator 0xcd62280  'int' '.*'
> |-CXXTemporaryObjectExpr 0xcd62080  'A' 'void () 
> throw()' zeroing
> `-UnaryOperator 0xcd62260  'int A::*' prefix '&' 
> cannot overflow
>   `-DeclRefExpr 0xcd621f8  'int' lvalue Field 
> 0xcd61be8 'n' 'int'
> ```
Aha, yay, thanks, i'll add this one, and also it seems that CFG and Analyzer 
C++ tests need way more different `-std=` run-lines :)


Repository:
  rC Clang

https://reviews.llvm.org/D44238



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

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-04-04 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: lib/Analysis/CFG.cpp:1435
 /// extended by a local reference with the given initializer.
 static QualType getReferenceInitTemporaryType(ASTContext &Context,
   const Expr *Init,

NoQ wrote:
> rsmith wrote:
> > Can you replace this with `Expr::skipRValueSubobjectAdjustments`? That's 
> > how we compute this elsewhere. (You'll need to skip a top-level 
> > `ExprWithCleanups` and check for the search terminating in a 
> > `MaterializeTemporaryExpr` yourself, but this will reduce duplication and 
> > fix a couple more cases this function is getting wrong).
> Hmm, right, ok, yeah. I'm in no rush with this one, so i'll spend some time 
> trying to understand what //rvalue// adjustments do we still have after 
> rC288563.
We still have to deal with rvalue adjustments in C++98 compilations, because we 
don't create xvalue `MaterializeTemporaryExpr` nodes there (because C++98 
doesn't have xvalues). I think if/when we carry out the plan to remove 
`CXXBindTemporaryExpr`, skipping rvalue adjustments will only be interesting to 
the code that performs lifetime extension, and everything else can just deal 
with the `MaterializeTemporaryExpr` nodes. But for now, you'll need to skip 
rvalue adjustments if you want to handle our C++98 ASTs.

As a quick example, take:

```
struct A { int n; };
const int &r = A().*&A::n;
```

... which produces this AST in C++11 onwards:

```
`-VarDecl 0xc2681f0  col:33 r 'const int &' cinit
  `-ExprWithCleanups 0xc268b28  'const int' xvalue
`-ImplicitCastExpr 0xc2689f0  'const int' xvalue 
  `-BinaryOperator 0xc2689c8  'int' xvalue '.*'
|-MaterializeTemporaryExpr 0xc2689b0  'A' xvalue 
extended by Var 0xc2681f0 'r' 'const int &'
| `-CXXTemporaryObjectExpr 0xc2687b0  'A' 'void () 
noexcept' zeroing
`-UnaryOperator 0xc268990  'int A::*' prefix '&' cannot 
overflow
  `-DeclRefExpr 0xc268928  'int' lvalue Field 0xc268148 
'n' 'int'
```

... but produces this in C++98 mode (which I think this function will mishandle 
because it doesn't know how to look through the binary `.*` operator):

```
`-VarDecl 0xcd61c90  col:33 r 'const int &' cinit
  `-ExprWithCleanups 0xcd62398  'const int' lvalue
`-MaterializeTemporaryExpr 0xcd622a8  'const int' lvalue 
extended by Var 0xcd61c90 'r' 'const int &'
  `-BinaryOperator 0xcd62280  'int' '.*'
|-CXXTemporaryObjectExpr 0xcd62080  'A' 'void () 
throw()' zeroing
`-UnaryOperator 0xcd62260  'int A::*' prefix '&' cannot 
overflow
  `-DeclRefExpr 0xcd621f8  'int' lvalue Field 0xcd61be8 
'n' 'int'
```


Repository:
  rC Clang

https://reviews.llvm.org/D44238



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

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-04-04 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: lib/Analysis/CFG.cpp:1435
 /// extended by a local reference with the given initializer.
 static QualType getReferenceInitTemporaryType(ASTContext &Context,
   const Expr *Init,

rsmith wrote:
> Can you replace this with `Expr::skipRValueSubobjectAdjustments`? That's how 
> we compute this elsewhere. (You'll need to skip a top-level 
> `ExprWithCleanups` and check for the search terminating in a 
> `MaterializeTemporaryExpr` yourself, but this will reduce duplication and fix 
> a couple more cases this function is getting wrong).
Hmm, right, ok, yeah. I'm in no rush with this one, so i'll spend some time 
trying to understand what //rvalue// adjustments do we still have after 
rC288563.


Repository:
  rC Clang

https://reviews.llvm.org/D44238



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

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-03-31 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: lib/Analysis/CFG.cpp:1435
 /// extended by a local reference with the given initializer.
 static QualType getReferenceInitTemporaryType(ASTContext &Context,
   const Expr *Init,

Can you replace this with `Expr::skipRValueSubobjectAdjustments`? That's how we 
compute this elsewhere. (You'll need to skip a top-level `ExprWithCleanups` and 
check for the search terminating in a `MaterializeTemporaryExpr` yourself, but 
this will reduce duplication and fix a couple more cases this function is 
getting wrong).


Repository:
  rC Clang

https://reviews.llvm.org/D44238



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

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-03-30 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

Seems fine to me, but you might want someone with analyzer experience to review.


Repository:
  rC Clang

https://reviews.llvm.org/D44238



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

[PATCH] D44238: [CFG] Fix automatic destructors when a member is bound to a reference.

2018-03-07 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ created this revision.
NoQ added reviewers: rsmith, doug.gregor, dcoughlin, xazax.hun, a.sidorin, 
george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.

In code like

  const int &x = A().x;

the destructor for `A()` was not present in the CFG due to two problems in the 
pattern-matching in `getReferenceInitTemporaryType()`:

1. The no-op cast from `T` to `const T` is not necessarily performed on a 
record type. It may be performed on essentially any type. In the current 
example, it is performed on an xvalue of type `int` in order to turn it into a 
`const int`.
2. Since https://reviews.llvm.org/rC288563 member access is no longer performed 
over rvalues, but goes after the `MaterializeTemporaryExpr` instead.

**This is not an analyzer-specific change.** It may add/remove compiler 
warnings, but i don't think it makes any sense to hide this behind an 
analyzer-specific flag.


Repository:
  rC Clang

https://reviews.llvm.org/D44238

Files:
  lib/Analysis/CFG.cpp
  test/Analysis/auto-obj-dtors-cfg-output.cpp


Index: test/Analysis/auto-obj-dtors-cfg-output.cpp
===
--- test/Analysis/auto-obj-dtors-cfg-output.cpp
+++ test/Analysis/auto-obj-dtors-cfg-output.cpp
@@ -14,6 +14,8 @@
 
 class A {
 public:
+  int x;
+
 // CHECK:  [B1 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B0
 // CHECK:  [B0 (EXIT)]
@@ -67,6 +69,25 @@
   const A& c = A();
 }
 
+// CHECK:  [B2 (ENTRY)]
+// CHECK-NEXT:   Succs (1): B1
+// CHECK:  [B1]
+// WARNINGS-NEXT:   1: A() (CXXConstructExpr, class A)
+// ANALYZER-NEXT:   1: A() (CXXConstructExpr, [B1.2], [B1.3], class A)
+// CHECK-NEXT:   2: [B1.1] (BindTemporary)
+// CHECK-NEXT:   3: [B1.2]
+// CHECK-NEXT:   4: [B1.3].x
+// CHECK-NEXT:   5: [B1.4] (ImplicitCastExpr, NoOp, const int)
+// CHECK-NEXT:   6: const int &x = A().x;
+// CHECK-NEXT:   7: [B1.6].~A() (Implicit destructor)
+// CHECK-NEXT:   Preds (1): B2
+// CHECK-NEXT:   Succs (1): B0
+// CHECK:  [B0 (EXIT)]
+// CHECK-NEXT:   Preds (1): B1
+void test_const_ref_to_field() {
+  const int &x = A().x;
+}
+
 // CHECK:  [B2 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B1
 // CHECK:  [B1]
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -1458,16 +1458,15 @@
 if (const CastExpr *CE = dyn_cast(Init)) {
   if ((CE->getCastKind() == CK_DerivedToBase ||
CE->getCastKind() == CK_UncheckedDerivedToBase ||
-   CE->getCastKind() == CK_NoOp) &&
-  Init->getType()->isRecordType()) {
+   CE->getCastKind() == CK_NoOp)) {
 Init = CE->getSubExpr();
 continue;
   }
 }
 
-// Skip member accesses into rvalues.
+// Skip member accesses.
 if (const MemberExpr *ME = dyn_cast(Init)) {
-  if (!ME->isArrow() && ME->getBase()->isRValue()) {
+  if (!ME->isArrow()) {
 Init = ME->getBase();
 continue;
   }
@@ -4873,10 +4872,12 @@
 const VarDecl *VD = DE->getVarDecl();
 Helper.handleDecl(VD, OS);
 
-const Type* T = VD->getType().getTypePtr();
-if (const ReferenceType* RT = T->getAs())
-  T = RT->getPointeeType().getTypePtr();
-T = T->getBaseElementTypeUnsafe();
+ASTContext &ACtx = VD->getASTContext();
+QualType T = VD->getType();
+if (T->isReferenceType())
+  T = getReferenceInitTemporaryType(ACtx, VD->getInit(), nullptr);
+if (const ArrayType *AT = ACtx.getAsArrayType(T))
+  T = ACtx.getBaseElementType(AT);
 
 OS << ".~" << T->getAsCXXRecordDecl()->getName().str() << "()";
 OS << " (Implicit destructor)\n";


Index: test/Analysis/auto-obj-dtors-cfg-output.cpp
===
--- test/Analysis/auto-obj-dtors-cfg-output.cpp
+++ test/Analysis/auto-obj-dtors-cfg-output.cpp
@@ -14,6 +14,8 @@
 
 class A {
 public:
+  int x;
+
 // CHECK:  [B1 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B0
 // CHECK:  [B0 (EXIT)]
@@ -67,6 +69,25 @@
   const A& c = A();
 }
 
+// CHECK:  [B2 (ENTRY)]
+// CHECK-NEXT:   Succs (1): B1
+// CHECK:  [B1]
+// WARNINGS-NEXT:   1: A() (CXXConstructExpr, class A)
+// ANALYZER-NEXT:   1: A() (CXXConstructExpr, [B1.2], [B1.3], class A)
+// CHECK-NEXT:   2: [B1.1] (BindTemporary)
+// CHECK-NEXT:   3: [B1.2]
+// CHECK-NEXT:   4: [B1.3].x
+// CHECK-NEXT:   5: [B1.4] (ImplicitCastExpr, NoOp, const int)
+// CHECK-NEXT:   6: const int &x = A().x;
+// CHECK-NEXT:   7: [B1.6].~A() (Implicit destructor)
+// CHECK-NEXT:   Preds (1): B2
+// CHECK-NEXT:   Succs (1): B0
+// CHECK:  [B0 (EXIT)]
+// CHECK-NEXT:   Preds (1): B1
+void test_const_ref_to_field() {
+  const int &x = A().x;
+}
+
 // CHECK:  [B2 (ENTRY)]
 // CHECK-NEXT:   Succs (1): B1
 // CHECK:  [B1]
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -1458,16 +1458,15 @@