This is an archive of the discontinued LLVM Phabricator instance.

[Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation
Needs ReviewPublic

Authored by brunodf on Oct 12 2021, 6:26 AM.

Details

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

Diff Detail

Event Timeline

brunodf requested review of this revision.Oct 12 2021, 6:26 AM
brunodf created this revision.
jeroen.dobbelaere retitled this revision from [PR51855] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation to [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation.Oct 12 2021, 6:47 AM
jeroen.dobbelaere edited the summary of this revision. (Show Details)
rnk added a comment.Oct 14 2021, 1:41 PM

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.

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 updated this revision to Diff 379958.Oct 15 2021, 3:55 AM

Apply clang-format to diff.

rnk added a subscriber: rjmccall.Oct 15 2021, 12:35 PM

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
14632–14633

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?

14663

This is also pseudo object handling code

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 updated this revision to Diff 380885.Oct 20 2021, 3:11 AM
brunodf edited the summary of this revision. (Show Details)
brunodf added a reviewer: akyrtzi.
brunodf added a subscriber: akyrtzi.

@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 added inline comments.Oct 20 2021, 11:16 AM
clang/lib/Sema/TreeTransform.h
14674

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;
}
14676

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

clang/test/SemaCXX/PR51855.cpp
28

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.

72

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 added inline comments.Oct 20 2021, 11:52 AM
clang/test/SemaCXX/PR51855.cpp
72

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 updated this revision to Diff 381639.Oct 22 2021, 1:02 PM
brunodf marked 3 inline comments as done.

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 accepted this revision.Nov 3 2021, 3:06 PM

lgtm

Sorry for the delay

clang/lib/Sema/TreeTransform.h
14674

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

This revision is now accepted and ready to land.Nov 3 2021, 3:06 PM

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 added a comment.Nov 30 2021, 11:47 AM

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

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 updated this revision to Diff 392429.Dec 7 2021, 8:49 AM

New version of patch to address problems detected by buildbots.

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

brunodf reopened this revision.Dec 7 2021, 1:42 PM

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.

This revision is now accepted and ready to land.Dec 7 2021, 1:42 PM
brunodf requested review of this revision.Dec 10 2021, 3:47 AM

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

rjmccall added inline comments.Dec 10 2021, 1:38 PM
clang/lib/Sema/TreeTransform.h
14663

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 added inline comments.Jan 20 2022, 1:00 PM
clang/lib/Sema/TreeTransform.h
14663

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.