[PATCH] D87349: [clang] adapt c++17 behavior for dependent decltype-specifiers

Thu Sep 24 13:03:49 PDT 2020

hokein added inline comments.

================
Comment at: clang/include/clang/AST/Type.h:4499-4508
 /// Internal representation of canonical, dependent
 /// decltype(expr) types.
 ///
 /// This class is used internally by the ASTContext to manage
 /// canonical, dependent types, only. Clients will only see instances
 /// of this class via DecltypeType nodes.
+class DependentDecltypeType : public DecltypeType {
----------------
rsmith wrote:
> Can we remove this class now?
yeah I think it is possible, but I would rather do it in a follow-up patch -- the current patch seems large enough. Added a FIXME.

================
Comment at: clang/test/CodeGenCXX/mangle.cpp:805

-  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE
+  // CHECK-LABEL: define weak_odr void @_ZN6test341fIiEEvm
   template void f<int>(decltype(sizeof(1)));
----------------
rsmith wrote:
> hokein wrote:
> > rsmith wrote:
> > > rsmith wrote:
> > > > sammccall wrote:
> > > > > sammccall wrote:
> > > > > > GCC mangles this as `_ZN6test341fIiEEvDTstDTplcvT__EcvS1__EEE`, so we're breaking compat here :-\
> > > > > > 
> > > > > > And in fact we're just incorrect. https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling:
> > > > > > 
> > > > > > > If the operand expression of decltype is not instantiation-dependent then the resulting type is encoded directly.
> > > > > > 
> > > > > > Here, instantiation-dependent *is* explicitly defined as "type-dependent or value-dependent or ...". And therefore these cases must not be "encoded directly", including the motivating case (decltype(N) where N is a typed constant whose initializer is value-dependent).
> > > > > > 
> > > > > > We could consider not two but *three* types of decltypetypes:
> > > > > >  - decltype(type-dependent), which is sugar for a canonical DependentDeclTypeType
> > > > > >  - decltype(instantiation-but-not-type-dependent), which is still sugar for a concrete canonical type (per C++17) but mangles using the expr (per cxxabi)
> > > > > >  - decltype(non-dependent), which is sugar for a concrete canonical type and mangles directly
> > > > > > 
> > > > > > This only works if it's OK for mangling to depend on non-canonical type details - I don't know whether this is the case. @rsmith - any hints here?
> > > > > Hmm, my naive reading of the mangle code matches what I described.
> > > > > 
> > > > > The big comment in ItaniumMangle talks about related issues: https://github.com/llvm/llvm-project/blob/24238f09edb98b0f460aa41139874ae5d4e5cd8d/clang/lib/AST/ItaniumMangle.cpp#L2541-L2572
> > > > > 
> > > > > I don't really understand what's going on here, sorry.
> > > > Looks like we need the single-step-desugaring loop below the big comment you quoted to stop when it hits a `decltype` type. That's presumably where we step from the instantiation-dependent-but-not-type-dependent `decltype` node to its desugared type.
> > > Also... the FIXME from that comment will apply here too. Given:
> > > ```
> > > template<typename T> void f(decltype(sizeof(T)), decltype(sizeof(T))) {}
> > > template void f<int>(unsigned long, unsigned long);
> > > ```
> > > we currently (correctly, as far as I can see) use a substitution for the second parameter type, but with the change here, I don't think we will do so any more. We could fix that by deduplicating `DecltypeType`s when they're instantiation dependent but not dependent; that's what we do for `ConstantArrayType`'s size expression, for example. If we do that, we'll need to store the actual expression on the `DecltypeTypeLoc`, since we can't rely on the `DecltypeType` having the right expression any more (only an expression that's equivalent to it).
> > Thanks for the hints and explanations.
> > 
> > > we currently (correctly, as far as I can see) use a substitution for the second parameter type, but with the change here, I don't think we will do so any more.
> > 
> > thanks for the example, yeah, the behavior was changed with the prior change of this patch. 
> > 
> > > We could fix that by deduplicating DecltypeTypes when they're instantiation dependent but not dependent; that's what we do for ConstantArrayType's size expression, for example. If we do that, we'll need to store the actual expression on the DecltypeTypeLoc, since we can't rely on the DecltypeType having the right expression any more (only an expression that's equivalent to it).
> > 
> > It took me sometime to understand the whole piece here, but I'm afraid that I still don't fully understand the second part -- any reason why we can't rely on the underlying expression of DecltypeType when doing the deduplication? If we store the actual expression on `DecltypeTypeLoc`, how do we retrieve it in `ASTContext::getDecltypeType()`, I didn't find any connection between `DecltypeTypeLoc` and `ASTContext::getDecltypeType`.
> > 
> > I updated the patch using the `DecltypeTypeLoc` to do the duplication, it passes the example you gave above, but it might be wrong. 
> > 
> The problem with the patch as it now stands is that for a case such as:
> 
> ```
> int x;
> template<typename T> void f(decltype(sizeof(T.f(x))));
> template<typename T> void g(decltype(sizeof(T.f(x))));
> ```
> 
> ... there is only one reference to `x` reachable in the AST, on line #2. The expression containing the `DeclRefExpr` on line #3 was discarded entirely. So (for example) any tooling that's looking to find references to `x` will not find the one on line #3.
> 
> The best way to fix this would be to store the `Expr*` for the actual expression in the `DecltypeTypeLoc` information. We'll also need to make sure that `TreeTransform` uses that `Expr*` rather than the one from the type when transforming a `DecltypeType` with source info.
Thanks, now I get it. 

I have updated this patch to implement what you suggested (storing the actual expression in `DecltypeTypeLoc`), it touches more files. Please take another look.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87349/new/

https://reviews.llvm.org/D87349