[Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

[ahatanak]

Summary

This fixes a bug which causes clang to emit incorrect IR for the following code in C++:

int foo(void) {
  unsigned a = get_size();
  volatile char b[a];
  b;
  return b[0];
}

The code crashes because the destination of the memcpy that is generated to do the lvalue-to-rvalue conversion of discarded-value expression b is incorrect. To fix this bug, I've added a check to Sema which prevents an lvalue cast expression to be emitted if the expression has an array type. My understanding is that lvalue-to-rvalue conversion isn't applied to array type expressions, so I think we should detect this in Sema rather than fixing the code in IRGen that emits the lvalue for the destination of the lvalue-to-rvalue conversion. I've also made changes to Sema::DiagnoseUnusedExprResult so that clang doesn't tell users to assign a reference to a volatile array to a variable to force its value to be loaded.

[rjmccall]

C++ says:

[expr.context]p2:

In some contexts, an expression only appears for its side effects. Such an expression is called a discarded-value expression. The array-to-pointer (7.3.2) and function-to-pointer (7.3.3) standard conversions are not applied. The lvalue-to-rvalue conversion (7.3.1) is applied if and only if the expression is a glvalue of volatile-qualified type and it is one of the following:

• (list of special forms)

[conv.lval]p1:

A glvalue of a non-function, non-array type T can be converted to a prvalue.

So I think the right fix is probably to make sure that Sema::DefaultLvalueConversion ignores gl-values of array type.

[ahatanak]

My first patch just did that and it seems to work. But the comment for DefaultLvalueConversion says "This is DefaultFunctionArrayLvalueConversion, except that it assumes the operand isn't of function or array type". It doesn't even have assertions to check that though.

[rjmccall]

That doesn't seem to be how we use it in practice; I'd just change the comment.

[rsmith]

It seems to me that there's as bug in the C++ specification here. The rules for discarded-value expressions say the lvalue-to-rvalue conversion is applied, whereas the rules for lvalue-to-rvalue conversions say one cannot be applied in this case. It's hard to guess what the correct behavior is (do we copy from the array or not?). But the rules do explicitly say "The array-to-pointer (7.3.2) and function-to-pointer (7.3.3) standard conversions are not applied." and it's unclear what effect that would have other than to cause a volatile copy of the entire array in a case such as this. I'm going to take this to CWG.

[rsmith]

In D78134#1981866, @rsmith wrote:

I'm going to take this to CWG.

So far, the direction the wind is blowing is that attempting to perform an lvalue-to-rvalue conversion on an array should be a no-op. (That's weirdly different from the behavior on a class, but so be it.)

[rjmccall]

In D78134#1982291, @rsmith wrote:

In D78134#1981866, @rsmith wrote:

I'm going to take this to CWG.

So far, the direction the wind is blowing is that attempting to perform an lvalue-to-rvalue conversion on an array should be a no-op. (That's weirdly different from the behavior on a class, but so be it.)

Okay. It's probably easiest and most correct to do that check in DefaultLvalueConversion, since otherwise callers would also have to do placeholder conversions. We use DefaultLvalueConversion a lot in Sema, apparently mostly in places where subsequent type restrictions disallow both arrays and decayed arrays; I'll admit to running out of energy after checking the first half-dozen matches, though.

[ahatanak]

In D78134#1982291, @rsmith wrote:

In D78134#1981866, @rsmith wrote:

I'm going to take this to CWG.

So far, the direction the wind is blowing is that attempting to perform an lvalue-to-rvalue conversion on an array should be a no-op. (That's weirdly different from the behavior on a class, but so be it.)

Is this the final decision? Or should I wait until we know for sure that this is a no-op and doesn't cause an lvalue-to-rvalue conversion?

[rsmith]

In D78134#1985467, @ahatanak wrote:

In D78134#1982291, @rsmith wrote:

So far, the direction the wind is blowing is that attempting to perform an lvalue-to-rvalue conversion on an array should be a no-op. (That's weirdly different from the behavior on a class, but so be it.)

Is this the final decision? Or should I wait until we know for sure that this is a no-op and doesn't cause an lvalue-to-rvalue conversion?

We won't know for months, but it seems reasonable to assume that this will be the final decision.

[ahatanak]

Make Sema::DefaultLvalueConversion ignore gl-values of array type

[ahatanak]

Rebase and ping.

I audited the calls to DefaultLvalueConversion. As far as I can tell, either an array type is never passed to the function call or there are checks that reject array types after the function is called, so the changes in this patch seem safe to me.

[rsmith]

clang/lib/Sema/SemaExpr.cpp

601–603

Given this comment, should we be checking for function types below too?

[rjmccall]

Alright, let's go with this.

[ahatanak]

Check function types in DefaultLvalueConversion.

clang/include/clang/Sema/Sema.h

public:
 
   // DefaultFunctionArrayLvalueConversion - converts functions and
   // arrays to their respective pointers and performs the
   // lvalue-to-rvalue conversion.
   ExprResult DefaultFunctionArrayLvalueConversion(Expr *E,
                                                   bool Diagnose = true);
 
   // DefaultLvalueConversion - performs lvalue-to-rvalue conversion on
-  // the operand.  This is DefaultFunctionArrayLvalueConversion,
-  // except that it assumes the operand isn't of function or array
-  // type.
+  // the operand. This function is a no-op if the operand has a function type
+  // or an array type.
   ExprResult DefaultLvalueConversion(Expr *E);
 
   // DefaultArgumentPromotion (C99 6.5.2.2p6). Used for function calls that
   // do not have a prototype. Integer promotions are performed on each
   // argument, and arguments that have type float are promoted to double.
   ExprResult DefaultArgumentPromotion(Expr *E);
 
   /// If \p E is a prvalue denoting an unmaterialized temporary, materialize

clang/lib/Sema/SemaExpr.cpp

 ExprResult Sema::DefaultLvalueConversion(Expr *E) {
   // Handle any placeholder expressions which made it here.
   if (E->getType()->isPlaceholderType()) {
     ExprResult result = CheckPlaceholderExpr(E);
     if (result.isInvalid()) return ExprError();
     E = result.get();
   }
 
   // C++ [conv.lval]p1:
   //   A glvalue of a non-function, non-array type T can be
   //   converted to a prvalue.
   if (!E->isGLValue()) return E;
 
   QualType T = E->getType();
   assert(!T.isNull() && "r-value conversion on typeless expression?");
 
+  // lvalue-to-rvalue conversion cannot be applied to function or array types.
+  if (T->isFunctionType() || T->isArrayType())
+    return E;
+
   // We don't want to throw lvalue-to-rvalue casts on top of
   // expressions of certain types in C++.
   if (getLangOpts().CPlusPlus &&
       (E->getType() == Context.OverloadTy ||
        T->isDependentType() ||
        T->isRecordType()))
     return E;
 

clang/lib/Sema/SemaStmt.cpp

if (T == Context.VoidPtrTy) {
       PointerTypeLoc TL = TI->getTypeLoc().castAs<PointerTypeLoc>();
 
       Diag(Loc, diag::warn_unused_voidptr)
         << FixItHint::CreateRemoval(TL.getStarLoc());
       return;
     }
   }
 
-  if (E->isGLValue() && E->getType().isVolatileQualified()) {
+  // Tell the user to assign it into a variable to force a volatile load if this
+  // isn't an array.
+  if (E->isGLValue() && E->getType().isVolatileQualified() &&
+      !E->getType()->isArrayType()) {
     Diag(Loc, diag::warn_unused_volatile) << R1 << R2;
     return;
   }
 
   DiagRuntimeBehavior(Loc, nullptr, PDiag(DiagID) << R1 << R2);
 }
 
 void Sema::ActOnStartOfCompoundStmt(bool IsStmtExpr) {

clang/test/CXX/expr/p10-0x.cpp

 // CHECK: define void @_Z2f2PVi
 // CHECK-NOT: load volatile
 // CHECK: ret
 void f2(volatile int *x) {
   // We shouldn't perform the load in these cases.
   refcall();
   1 ? refcall() : *x;
 }
+
+// CHECK: define void @_Z2f3v()
+// CHECK-NOT: load
+// CHECK-NOT: memcpy
+
+void f3(void) {
+  volatile char a[10];
+  a;
+}

clang/test/SemaCXX/warn-unused-value-cxx11.cpp

void j() {
   (void)noexcept(i = 5); // expected-warning {{expression with side effects has no effect in an unevaluated context}}
   (void)noexcept(s = 5); // Ok
 
   (void)sizeof(s.f()); // Ok
   (void)sizeof(s.f() = 5); // expected-warning {{expression with side effects has no effect in an unevaluated context}}
   (void)noexcept(s.g() = 5); // Ok
 }
 
-}
+}
+
+namespace volatile_array {
+void test() {
+  char a[10];
+  volatile char b[10];
+  a; // expected-warning-re {{expression result unused{{$}}}}
+  b; // expected-warning-re {{expression result unused{{$}}}}
+}
+}