[Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[brunodf]

Summary

The invocation of a unary or binary operator for type-dependent expressions is represented as a CXXOperatorCallExpr. Upon template instantiation, TreeTransform::RebuildCXXOperatorCallExpr checks for the case of an overloaded operator, but not for a (non-ObjC) PseudoObject, and will directly create a UnaryOperator or BinaryOperator.

Generalizing commit 0f99537ecac40 from @akyrtzi to handle non-ObjC pseudo objects (and also handle the case of unary pseudo object inc/dec).

This fixes https://bugs.llvm.org/show_bug.cgi?id=51855

[rnk]

There are some invariants about what family of APIs TreeTransform methods should call, and I've forgotten what they are, so I'm hesitant to approve this. Test case looks good, though.

[brunodf]

In D111639#3065241, @rnk wrote:

There are some invariants about what family of APIs TreeTransform methods should call, and I've forgotten what they are, so I'm hesitant to approve this. Test case looks good, though.

I am only aware of this paragraph from https://clang.llvm.org/docs/InternalsManual.html :

Semantic analysis should always involve two functions: an ActOnXXX function that will be called directly from the parser, and a BuildXXX function that performs the actual semantic analysis and will (eventually!) build the AST node. It’s fairly common for the ActOnCXX function to do very little (often just some minor translation from the parser’s representation to Sema’s representation of the same thing), but the separation is still important: C++ template instantiation, for example, should always call the BuildXXX variant.

I noticed that e.g. TreeTransform::RebuildBinaryOperator is also calling BuildBinOp, in line with the above description that C++ template instantiation should call the BuildXXX functions

[brunodf]

Apply clang-format to diff.

[rnk]

So, after reading through Build(Unary|Bin)Op, I think the duplication of logic between TreeTransform is intentional, and the better fix is to generalize the existing logic for placeholder handling. If you enter into the Build*Op codepaths, you will re-run a lot of redundant checks. Maybe +@rjmccall can provide a second opinion.

clang/lib/Sema/TreeTransform.h

14571–14572

I notice that ObjC pseudo objects are handled here. Is there a way to unify things? Can this code be removed if we route builtin operators over to Sema::Build(Bin|Unary)Op?

14571–14572

This is also pseudo object handling code

[rjmccall]

I don't think we currently hold very strongly to that ActOn vs. Build philosophy across all productions, but it does seem like a good idea.

I don't really know why there's this much duplication between the code paths. The only semantic distinction I know of is that IIRC we need to make sure we use the unqualified operator lookup results from the template instead of reperforming the lookup.

[brunodf]

@rnk raises the remark that TreeTransform::RebuildCXXOperatorCallExpr already contains code to handle ObjC pseudo objects (including checking pseudo object assignment) and that calling Build*Op will re-run a lot of redundant checks.

To address this, in this updated diff, I tried to generalize the code for ObjC pseudo object handling (which originates from @akyrtzi in commit 0f99537ecac40) to handle other pseudo object kinds in addition to ObjC. I think calling checkPseudoObjectAssignment and checkPseudoObjectIncDec is critical for the PR I'm trying to solve, not sure about CheckPlaceholderExpr, but I retained that from @akyrtzi his patch.

Adding @akyrtzi as reviewer.

[rnk]

clang/lib/Sema/TreeTransform.h

14576

Can we come here to subscript pointer types? Something like:

struct Foo {
  void putM(int* rhs) { _m = rhs; }
  int* getM() { return _m; }
  __declspec(property(get = getM, put = putM)) int* theData;
};
void bar(Foo *p, int idx) {
  p->theData[idx] = 42;
}

14578

Similarly, do we need to cover pseudo objects in this codepath? p->theData->m2?

clang/test/SemaCXX/PR51855.cpp

27

There are many platforms with varying default calling conventions. You will either need to generalize these checks to match such conventions, or choose a specific triple to test with.

I can think of two targets to try that might fail these check lines: ARM targets, and i686-windows-gnu, which will use x86_thiscallcc.

71

Please add test coverage for the interesting operators in the code under edit: OO_Subscript, OO_Arrow, OO_Amp. Please also add equivalent coverage to the ObjC test cases, or give me a link to show that it's already covered.

Maybe OO_Amp is unnecessary, I'm not sure how it is supposed to work for psuedo objects.

[rjmccall]

clang/test/SemaCXX/PR51855.cpp

71

Different pseudo-object extensions can define their own rules for different syntactic use patterns, but currently there aren't any that have special treatment for either unary or binary &; you just take the default "r-value" path and then see if you can apply the operator to the result.

[brunodf]

I'm adding a new patch to (partially) address the comments from @rnk.

An ObjC test case was included in the commit from @akyrtzi, I've updated it to also cover the case of a unary operator (and the increment/decrement), and the case of a type dependent on a template parameter. I moved my changes to the original position of the code for ObjC properties in RebuildCXXOperatorCallExpr.

With regard to OO_Subscript, OO_Arrow and OO_Amp, I have tried a number of things, but I have not succeeded in triggering that an CXXOperatorCallExpr is created for these operators (they end up as an ArraySubscriptExpr, CXXDependentScopeMemberExpr and UnaryOperator respectively). At the moment, I don't know how to test the code paths for these operators in RebuildCXXOperatorCallExpr.

[rnk]

lgtm

Sorry for the delay

clang/lib/Sema/TreeTransform.h

14576

I would still like to see a test for subscripts, but it's not a blocking issue.

[brunodf]

In D111639#3081504, @brunodf wrote:

With regard to OO_Subscript, OO_Arrow and OO_Amp, I have tried a number of things, but I have not succeeded in triggering that an CXXOperatorCallExpr is created for these operators (they end up as an ArraySubscriptExpr, CXXDependentScopeMemberExpr and UnaryOperator respectively). At the moment, I don't know how to test the code paths for these operators in RebuildCXXOperatorCallExpr.

After my submission, I still looked further into cases with these operators, but a CXXOperatorCallExpr is only used for these operators in case an overloaded operator has been determined (so not in case of a dependent type). Also, these operators are not allowed as non-members functions. While a property member could be of a type that has such an overloaded member operator, this would be dubious since it would work on a reference to the temporary returned by the getter of the property member.

So while it would be possible to add tests for CXXOperatorCallExpr involving invocation of these operators on property members, I think this would be artificial and not really contribute to the patch.

[rnk]

Sounds good. This patch is still pending, right? I don't see it in the logs. Do you need someone to push it?

[jeroen.dobbelaere]

A number of buildbots started failing after submission. Reverted for now so we can investigate if the new produced errors are valid or not.

[brunodf]

New version of patch to address problems detected by buildbots.

(Will describe in more detail in follow-up comment.)

[brunodf]

When the previous diff was committed, two problems were discovered in the buildbots:

1. One pattern in the PR51855.cpp test failed on certain targets due to a difference in calling convention: https://lab.llvm.org/buildbot/#/builders/171/builds/6632
2. In multistage buildbots, an overload resolution failure was reported while compiling the LLVM codebase in stage 2: https://lab.llvm.org/buildbot/#/builders/187/builds/3136

The pattern in the PR51855.cpp test is straightforward to generalize.

The stage 2 issue revealed a problem that is now reduced in the D111639.cpp test. Here, a unary address-of & operator is used on a DependentScopeDeclRef (which has dependent type), and the presence of a binary operator& overload triggers the use of a CXXOperatorCallExpr. When the CXXOperatorCallExpr is rebuilt for template instantiation, the argument is an overload set. It has a placeholder type (BuiltinType::Overload), but does not respond well to CheckPlaceHolderExpr invocation: this triggers the "reference to overloaded function could not be resolved" error.

In Sema::BuildUnaryOp there is an extra test clause for UO_AddrOf operators in the logic for placeholder types, before calling CheckPlaceHolderExpr. I've adopted the same clause here.

Before this diff, there was no initial check for placeholder types (only for OK_ObjCProperty) and this would be handled by lines 14661-14669 of TreeTransform.h (left-hand side numbering), also resulting in a call to CreateBuiltinUnaryOp.

[brunodf]

A stage2 build (outside of the buildloops) is now working correctly. Pinging for new review.

[rjmccall]

clang/lib/Sema/TreeTransform.h

14571–14572

Hmm. Am I wrong to be concerned about folding overload placeholders too early in these clauses? Surely overloads can be resolved by the operator call in some cases.

I agree with Reid that it would be really nice if we could make this share the normal paths for C++ operator resolution instead of duplicating so much of them.

[brunodf]

clang/lib/Sema/TreeTransform.h

14571–14572

Hmm. Am I wrong to be concerned about folding overload placeholders too early in these clauses? Surely overloads can be resolved by the operator call in some cases.

This would have to be reviewed against the paths in Sema::BuildBinOp? There are some cases for a RHS placeholder that does not end in calling CheckPlaceholderExpr:

// Handle pseudo-objects in the RHS.
 if (const BuiltinType *pty = RHSExpr->getType()->getAsPlaceholderType()) {
   // An overload in the RHS can potentially be resolved by the type
   // being assigned to.
   if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
     if (getLangOpts().CPlusPlus &&
         (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent() ||
          LHSExpr->getType()->isOverloadableType()))
       return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);

     return CreateBuiltinBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
   }

   // Don't resolve overloads if the other type is overloadable.
   if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload &&
       LHSExpr->getType()->isOverloadableType())
     return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);

   ExprResult resolvedRHS = CheckPlaceholderExpr(RHSExpr);
   if (!resolvedRHS.isUsable()) return ExprError();
   RHSExpr = resolvedRHS.get();
 }

I agree with Reid that it would be really nice if we could make this share the normal paths for C++ operator resolution instead of duplicating so much of them.

I think extracting the common placeholder logic from Build(Unary|Bin)Op and this function is beyond the scope of this patch (and probably outside my capabilities, I'm afraid). Still I hope to get things in a better shape with this patch than it was before (see PR51855). A follow up that would attempt to remove the duplication would indeed be nice.

clang/lib/Sema/TreeTransform.h

 ExprResult
 TreeTransform<Derived>::RebuildCXXOperatorCallExpr(OverloadedOperatorKind Op,
                                                    SourceLocation OpLoc,
                                                    Expr *OrigCallee,
                                                    Expr *First,
                                                    Expr *Second) {
   Expr *Callee = OrigCallee->IgnoreParenCasts();
   bool isPostIncDec = Second && (Op == OO_PlusPlus || Op == OO_MinusMinus);
 
-  if (First->getObjectKind() == OK_ObjCProperty) {
-    BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
-    if (BinaryOperator::isAssignmentOp(Opc))
-      return SemaRef.checkPseudoObjectAssignment(/*Scope=*/nullptr, OpLoc, Opc,
-                                                 First, Second);
-    ExprResult Result = SemaRef.CheckPlaceholderExpr(First);
-    if (Result.isInvalid())
-      return ExprError();
-    First = Result.get();
-  }
-
-  if (Second && Second->getObjectKind() == OK_ObjCProperty) {
-    ExprResult Result = SemaRef.CheckPlaceholderExpr(Second);
-    if (Result.isInvalid())
-      return ExprError();
-    Second = Result.get();
-  }
-
   // Determine whether this should be a builtin operation.
   if (Op == OO_Subscript) {
     if (!First->getType()->isOverloadableType() &&
         !Second->getType()->isOverloadableType())
       return getSema().CreateBuiltinArraySubscriptExpr(
           First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
     // -> is never a builtin operation.
     return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {
     if (!First->getType()->isOverloadableType() ||
         (Op == OO_Amp && getSema().isQualifiedMemberAccess(First))) {
       // The argument is not of overloadable type, or this is an expression
       // of the form &Class::member, so try to create a built-in unary
       // operation.
+      //
+      // But handle pseudo-objects in the LHS, and increment and decrement
+      // of pseudo-object l-value (see Sema::BuildUnaryOp).
       UnaryOperatorKind Opc
         = UnaryOperator::getOverloadedOpcode(Op, isPostIncDec);
+      if (const BuiltinType *pty = First->getType()->getAsPlaceholderType()) {
+        if (pty->getKind() == BuiltinType::PseudoObject &&
+            UnaryOperator::isIncrementDecrementOp(Opc))
+          return SemaRef.checkPseudoObjectIncDec(/*Scope=*/nullptr, OpLoc, Opc,
+                                                 First);
+        ExprResult Result = SemaRef.CheckPlaceholderExpr(First);
+        if (Result.isInvalid())
+          return ExprError();
+        First = Result.get();
+      }
 
       return getSema().CreateBuiltinUnaryOp(OpLoc, Opc, First);
     }
   } else {
     if (!First->getType()->isOverloadableType() &&
         !Second->getType()->isOverloadableType()) {
       // Neither of the arguments is an overloadable type, so try to
       // create a built-in binary operation.
+      //
+      // But handle pseudo-objects in the LHS/RHS, and assignments with a
+      // pseudo-object l-value (see Sema::BuildBinOp).
       BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
+      if (const BuiltinType *pty = First->getType()->getAsPlaceholderType()) {
+        if (pty->getKind() == BuiltinType::PseudoObject &&
+            BinaryOperator::isAssignmentOp(Opc))
+          return SemaRef.checkPseudoObjectAssignment(/*Scope=*/nullptr, OpLoc,
+                                                     Opc, First, Second);
+        ExprResult Result = SemaRef.CheckPlaceholderExpr(First);
+        if (Result.isInvalid())
+          return ExprError();
+        First = Result.get();
+      }
+      if (Second->getType()->isPlaceholderType()) {
+        ExprResult Result = SemaRef.CheckPlaceholderExpr(Second);
+        if (Result.isInvalid())
+          return ExprError();
+        Second = Result.get();
+      }
       ExprResult Result
         = SemaRef.CreateBuiltinBinOp(OpLoc, Opc, First, Second);
       if (Result.isInvalid())
         return ExprError();
 
       return Result;
     }
   }

clang/test/SemaCXX/PR51855.cpp

+// RUN: %clang_cc1 -S -triple %itanium_abi_triple -fms-extensions -emit-llvm %s -o - | FileCheck %s
+
+struct F {};
+
+F operator*=(F &lhs, int rhs);
+
+F operator++(F &lhs);
+
+struct S {
+  short _m;
+  S(short _m) : _m(_m) {}
+
+  void putM(short rhs) { _m = rhs; }
+  short getM() { return _m; }
+
+  __declspec(property(get = getM, put = putM)) short theData;
+};
+
+int test1a(int i) {
+  S tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test1ai(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+
+template <typename T>
+int test1b(int i) {
+  T tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+template int test1b<S>(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test1bI1SEii(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+
+int test2a(int i) {
+  S tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2ai(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+
+template <typename T>
+int test2b(int i) {
+  T tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+template int test2b<S>(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2bI1SEii(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv