Page MenuHomePhabricator

[clang] extend getCommonSugaredType to merge sugar nodes
ClosedPublic

Authored by mizvekov on Jul 21 2022, 2:52 PM.

Details

Summary

This continues D111283 by extending the getCommonSugaredType
implementation to also merge non-canonical type nodes.

We merge these nodes by going up starting from the canonical
node, calculating their merged properties on the way.

If we reach a pair that is too different, or which we could not
otherwise unify, we bail out and don't try to keep going on to
the next pair, in effect striping out all the remaining top-level
sugar nodes. This avoids mismatching 'companion' nodes, such as
ElaboratedType, so that they don't end up elaborating some other
unrelated thing.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mizvekov updated this revision to Diff 447997.Jul 27 2022, 4:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2022, 4:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mizvekov updated this revision to Diff 452459.Aug 13 2022, 3:52 PM
mizvekov edited the summary of this revision. (Show Details)
davrec added a subscriber: davrec.Aug 14 2022, 11:00 AM
davrec added inline comments.
clang/lib/AST/ASTContext.cpp
12269

I'm not clear on how NExpX could not equal NExpY - could you add a test which demonstrates this case?

12889

Nit: "As" -> "Args", since that's common elsewhere

12953

It would be very helpful to incorporate your description and the description from D111283 as comments in this function. E.g. something like the following ...

12955–12975
// Desugar SX and SY, setting the sugar and qualifiers aside into Xs and Ys/QX and QY, 
// until we reach their underlying "canonical nodes".  (Note these are not necessarily 
// canonical types, as their child types may still be sugared.)
12957
// The canonical nodes differ. Build a common canonical node out of the two, 
// including any sugar shared by their child types.
12959
// The canonical nodes were identical: we may have desugared too much.
// Add any common sugar back in.
12980
// Even though the remaining sugar nodes in Xs and Ys differ, some may be of the same
// type class and have common sugar in their child types.  Walk up these nodes, 
// adding in any such sugar.
davrec added inline comments.Aug 14 2022, 11:24 AM
clang/lib/AST/ASTContext.cpp
12852

This last argument should probably be omitted/nullptr passed, since

  1. We probably won't encounter any owned tag decls in the types passed to this function;
  2. Even if we did, it would not necessarily be true that they both have non-null owned tag decls, and I don't see any nullptr checks in getCommonDecl; and
  3. Even if we checked for nullptr there, and say passed the same argument to X and Y so EX==EY, and that had an owned tag decl, it is not clear to me it would be appropriate to construct a new type with the same owned tag decl as another type.
mizvekov added inline comments.Aug 19 2022, 5:55 PM
clang/lib/AST/ASTContext.cpp
12852
  1. We can see, a one liner like this will do: int e = true ? (struct S*)0 : (struct S*)0;

Though normally in an example like the above, you would only see the owned decl on X, because when we get to Y the name has already been introduced into the scope.

I have an idea for a test case, but it's a bit convoluted:

This also introduces an OwnedTagDecl: (struct S<T>*)0.

So with resugaring, if we resugar T, it might be possible to construct this situatiation. Given if it's appropriate to keep the OwnedTagDecl when resugaring, of course, which goes to 3)

  1. This is handled by declaresSameEntity, if either or both decls are null, we say they are not the same decl.
  1. That I am not sure. I see that this OwnedTagDecl is only used by the type printer, and it replaces printing the rest of the type by printing the OwnedDecl instead. So why do you think it would not be appropriate that the rebuilt type keeps this?
davrec added inline comments.Aug 19 2022, 7:03 PM
clang/lib/AST/ASTContext.cpp
12852

(Re #3) Because I think the sense ownership is respected in how ownedTagDecls are presently handled: at most one ElaboratedType owns a particular TagDecl, *and* that type does not seem to be reused elsewhere in the AST. (This is relied on in https://reviews.llvm.org/D131685, which was on my mind when I looked at this.)

E.g. consider this expansion of your example:

auto e = true ? (struct S*)0 : (true ? (struct S*)0 : (struct S*)0);

The first ElaboratedType has an ownedTagDecl; the second is a distinct ElaboratedType without an ownedTagDecl, and the third equals the second.

Now in practice what this means is that getCommonDecl when used on binary expressions of this sort will always be nullptr, so no harm done. But suppose it is called with X=Y=some ET with an ownedTagDecl (which is the only time I believe we could see a non null getCommonDecl result here). Should the type that is returned have that same owned tag decl? I think that would violate the sense of the original type "owning" that decl that seems to be respected elsewhere in the AST.

Re type printing, I believe that ownedTagDecls only affects printing of defined TagDecls like struct S { int i; } x; (which is what I was mostly thinking re ownedTagDecls - I don't think we will see those here); for undefined ones like struct S x;, printing should be unaffected whether there is an ownedTagDecl or not.

mizvekov added inline comments.Aug 19 2022, 7:39 PM
clang/lib/AST/ASTContext.cpp
12852

With type deduction this can happen:

auto x = (struct S*)0; // it will also appear in the type of x
using t = decltype(x); // and now also in t

With your second example, it can also happen:

struct S { int i; } x;
int e = true ? &x : (struct S*)0;

In those cases, the type node that owns the decl will be the same object, but that is only because of uniquing.

It may be possible that two different objects end up in this situation, if for example they come from different modules that got merged.

davrec added inline comments.Aug 19 2022, 9:21 PM
clang/lib/AST/ASTContext.cpp
12852

With type deduction this can happen:

auto x = (struct S*)0; // it will also appear in the type of x
using t = decltype(x); // and now also in t

My local clang is out of date - the type of x for me is just an AutoType wrapping a PointerType to a RecordType. In light of the addition of the ElaboratedType there, it seems fine to be consistent here with however that case handles ownedTagDecls, but FWIW I do not think that deduced ElaboratedType should have an ownedTagDecl either. Only the original type should be the owner - not any type deduced from it.

This is a minor issue with few if any observable effects for now, but should be kept in mind if issues arise later: the fact the ownership of ownedTagDecls is respected was the only factor that made https://discourse.llvm.org/t/ast-parent-of-class-in-a-friend-declaration/64275 easily solvable.

With your second example, it can also happen:

struct S { int i; } x;
int e = true ? &x : (struct S*)0;

The DeclRefExpr x indeed reuses the ElaboratedType used in the VarDecl x, with its ownedTagDecl and all. That seems fair, a special case. For all other type deductions I would mildly object to including an ownedTagDecl.

mizvekov marked 5 inline comments as done.Aug 20 2022, 10:48 AM
mizvekov added inline comments.
clang/lib/AST/ASTContext.cpp
12269

It may not be needed in this patch, and it might in fact be related to a bug which I already solved in the main resugaring patch. I will double check later.

12852

I think that assumption might work on ElaboratedTypeLocs instead. I think only one per TU should appear in the AST.
Resugaring might make us rebuild such TypeLocs, but the non-resugared one should be unreachable from the AST.

12953

Thanks! I also added some extras in there.

mizvekov updated this revision to Diff 456201.Sun, Aug 28, 11:01 AM
mizvekov edited the summary of this revision. (Show Details)
ChuanqiXu added inline comments.Thu, Sep 1, 7:30 PM
clang/include/clang/AST/ASTContext.h
1369

Maybe we need a comment for this. The signature looks not straight forward and I can't relate this to the above comment.

mizvekov marked 3 inline comments as done.Thu, Sep 1, 7:35 PM
mizvekov added inline comments.Thu, Sep 1, 7:40 PM
clang/include/clang/AST/ASTContext.h
1369

I think I wanted to make this an internal variant, since we will probably never need this outside of ASTContext anyway, but there is the friendship situation between Type and ASTContext. Maybe making this private would be for the best.

ChuanqiXu added inline comments.Thu, Sep 1, 7:51 PM
clang/include/clang/AST/ASTContext.h
1369

Yeah, the signature confused me as well... as long as the second parameter is Decayed already, why we need to get the decayed type again? I guess Underlying may be a better name.

mizvekov added inline comments.Thu, Sep 1, 8:06 PM
clang/include/clang/AST/ASTContext.h
1369

Ah I see, but this confusion already existed. See the definition of DecayedType in Type.h, where you have the QualType getDecayedType() member. This is what this Decayed parameter represents, what we will put in there to be returned by that method.

For DecayedType / AdjustedType / AttributedType there exists this confusion where they have two child nodes, which arbitrarily picking one of them to be called the UnderlyingType could be controversial. You could make the case that it should be the same one we use for implementing desugar, but there are arguments there for the other case.

ChuanqiXu added inline comments.Thu, Sep 1, 8:10 PM
clang/include/clang/AST/ASTContext.h
1369

I see. I got your point. Thanks.

davrec accepted this revision.Mon, Sep 5, 5:06 PM

LGTM

erichkeane accepted this revision.Tue, Sep 6, 6:43 AM
erichkeane added a subscriber: erichkeane.

I've got concerns about the interface not being clear on null-ability, and THIS patch seems to move us toward nulls being disallowed, so I'd like that formalized when possible (either via asserts or, better, by switching to references when possible). Otherwise, LG.

clang/lib/AST/ASTContext.cpp
12149–12167

same concerns about null here. I find myself wondering if this 'getCommonX' should take/return references when possible.

mizvekov marked 4 inline comments as done.Wed, Sep 7, 5:31 PM
mizvekov added inline comments.
clang/lib/AST/ASTContext.cpp
12149–12167

Null is a valid input on this one. We change this function to now accept unrelated decls, and return null in that case as well.

mizvekov updated this revision to Diff 458613.Wed, Sep 7, 5:31 PM
mizvekov marked an inline comment as done.
This revision was not accepted when it landed; it landed in state Needs Review.Thu, Sep 8, 10:18 AM
This revision was automatically updated to reflect the committed changes.
tra added a subscriber: tra.Thu, Sep 8, 10:47 AM

It looks like some of these changes are causing compiler to crash in clang::ASTContext::getFloatTypeSemantics during CUDA compilation:
https://lab.llvm.org/buildbot/#/builders/55/builds/35047

I'm currently working on narrowing down which commit is the culprit. Here's a snippet of the stack trace:

 #8 0x0000000008fda385 clang::ASTContext::getFloatTypeSemantics(clang::QualType) const (/buildbot/cuda-p4-0/work/clang-cuda-p4/clang/bin/clang-16+0x8fda385)
 #9 0x0000000008927cb5 unsupportedTypeConversion(clang::Sema const&, clang::QualType, clang::QualType) SemaExpr.cpp:0:0
#10 0x000000000894925a clang::Sema::CheckAssignmentConstraints(clang::QualType, clang::ActionResult<clang::Expr*, true>&, clang::CastKind&, bool) (/buildbot/cuda-p4-0/work/clang-cuda-p4/clang/bin/clang-16+0x894925a)
#11 0x0000000008948e26 clang::Sema::CheckAssignmentConstraints(clang::SourceLocation, clang::QualType, clang::QualType) (/buildbot/cuda-p4-0/work/clang-cuda-p4/clang/bin/clang-16+0x8948e26)
#12 0x00000000089570a7 clang::Sema::CheckAssignmentOperands(clang::Expr*, clang::ActionResult<clang::Expr*, true>&, clang::SourceLocation, clang::QualType, clang::BinaryOperatorKind) (/buildbot/cuda-p4-0/work/clang-cuda-p4/clang/bin/clang-16+0x89570a7)
#13 0x000000000893af00 clang::Sema::CreateBuiltinBinOp(clang::SourceLocation, clang::BinaryOperatorKind, clang::Expr*, clang::Expr*) (/buildbot/cuda-p4-0/work/clang-cuda-p4/clang/bin/clang-16+0x893af00)
tra added a comment.Thu, Sep 8, 11:19 AM

The culprit appears to be D111509.

mizvekov reopened this revision.Fri, Sep 9, 9:10 AM
mizvekov updated this revision to Diff 459363.Sun, Sep 11, 7:46 AM
mizvekov updated this revision to Diff 459370.Sun, Sep 11, 7:53 AM
mizvekov updated this revision to Diff 460063.Wed, Sep 14, 6:24 AM
mizvekov updated this revision to Diff 460064.
mizvekov edited the summary of this revision. (Show Details)
mizvekov updated this revision to Diff 460588.Thu, Sep 15, 6:10 PM
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Sep 16, 8:05 AM
This revision was automatically updated to reflect the committed changes.