[Sema] Fix crash on __fp16 parameters in template instantiations

[ilya-biryukov]

Summary

Fixes #61441.

Currently, Clang stores nullptr in the parameter lists inside
FunctionProtoTypeLoc if __fp16 is used without pointer qualifiers.

Any code path that calls Declarator::setInvalidType() before
GetFullTypeForDeclarator will lead to the same problem downstream.

The relevant code is:

cpp
if (D.isInvalidType())
  return Context.getTrivialTypeSourceInfo(T);

return GetTypeSourceInfoForDeclarator(state, T, TInfo);

GetTypeSourceInfoForDeclarator sets the parameter Decl, but we can't
call it when isInvalidType() == true as this causes other assertion
failures that seem harder to fix.

[shafik]

Please edit the summary to indicate this fixes: https://github.com/llvm/llvm-project/issues/61441

I am happy someone had more time to dig into this problem after my initial investigation.

[ilya-biryukov]

In D146426#4206519, @shafik wrote:

Please edit the summary to indicate this fixes: https://github.com/llvm/llvm-project/issues/61441
I am happy someone had more time to dig into this problem after my initial investigation.

Thanks, done!
I wasn't aware of the upstream bug and your prior investigation. It would have definitely saved some time.

[shafik]

clang/test/SemaCXX/crash-params.cpp

3

If this test really does not fit into any other test set the please rename the test file GH61441.cpp so we know it is a regression test for that github bug report.

Otherwise if we add a test from a bug report into an existing test file then we wrap it something like namespace GH61441 so we know that test is a regression test for github bug 61441.

[shafik]

clang/lib/Sema/SemaDeclCXX.cpp

1714

This comment seems to no longer be true since we don't emit a diagnostic for the !PD case.

[shafik]

In D146426#4206525, @ilya-biryukov wrote:

In D146426#4206519, @shafik wrote:

Please edit the summary to indicate this fixes: https://github.com/llvm/llvm-project/issues/61441
I am happy someone had more time to dig into this problem after my initial investigation.

Thanks, done!
I wasn't aware of the upstream bug and your prior investigation. It would have definitely saved some time.

That is unfortunate, I definitely would have preferred for you to have benefitted from my work.

[shafik]

Adding clang-language-wg for more visibility

[ilya-biryukov]

• rename test file to GH61441.cpp, add a comment this checks for no crash
• Update outdated comment

clang/lib/Sema/SemaDeclCXX.cpp

1714

Good catch! Updated the comment.

clang/test/SemaCXX/crash-params.cpp

3

I am not very good at navigating the test files and couldn't find a good existing file for the test.
Ended up creating GH61441.cpp.

[kadircet]

as discussed offline, this feels a little fishy and we should probably try and not put nulls into the parameter lists at all (and mark the functiontype as invalid instead), but since i don't know how to do that change myself it doesn't feel fair to ask it from you :)
LG and addresses a common crash we see on clangd, so let's ship it.

[aaron.ballman]

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

[erichkeane]

In D146426#4207118, @aaron.ballman wrote:

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

Frankly, I would like to see us just insert the __fp16 type instead. We've already diagnosed, so we won't go to codegen, which is where the parameter issue is going to cause a problem. I can't imagine we have ANY code that depends on __fp16 and would break if it is a parameter.

[ilya-biryukov]

In D146426#4207118, @aaron.ballman wrote:

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

I actually prototyped doing this and it caused extra diagnostics with no locations during template instantiations.
Moreover, this happens not only with __fp16, but for all cases when Declarator::setInvalidType is called (and that's quite a lot of cases).
We also have checks for nullptr inside parameter lists in many places of the codebase, e.g. see a usage in SemaTemplateInstantiateDecl.cpp.

I fully agree that the direction taken is not very good, but it has not been chosen by this patch.
A more involved refactoring that would ensure parameter lists never have nulls is definitely very welcome, this change intends to quickly fix a crash we're experiencing.

In D146426#4207146, @erichkeane wrote:

In D146426#4207118, @aaron.ballman wrote:

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

Frankly, I would like to see us just insert the __fp16 type instead. We've already diagnosed, so we won't go to codegen, which is where the parameter issue is going to cause a problem. I can't imagine we have ANY code that depends on __fp16 and would break if it is a parameter.

+1 to that, __fp16 is a better choice here, e.g. that's something I would expect to see in AST dumps.
We just need a fix that avoids nulls inside parameter lists.
Note that the types are already handled properly, but for some reason that I didn't dig out we choose to defer setting the parameter decls until a call to GetTypeSourceInfoForDeclarator.

[shafik]

As I noted in the bug report not doing D.setInvalidType(); does fix this bug and seems harmless since the error diagnostic should prevent us from getting to codegen but it is not clear to me if this has other negative impacts. Reading your replies it is not obvious you looked at this path or not.

I would not mind a quick fix but I think we should have a plan for a more correct fix before we do that.

[ilya-biryukov]

In D146426#4207423, @shafik wrote:

As I noted in the bug report not doing D.setInvalidType(); does fix this bug and seems harmless since the error diagnostic should prevent us from getting to codegen but it is not clear to me if this has other negative impacts. Reading your replies it is not obvious you looked at this path or not.

There are too many other code paths that set setInvalidType(), I'll try to add more tests that capture those too.
It would be easy to avoid calling setInvalidType() for __fp16 in particular, but checking for nulls actually saves us from other potential crashes .

I would not mind a quick fix but I think we should have a plan for a more correct fix before we do that.

Based on conversations in this thread, I think the right way is to make sure parameter lists never have nulls.
One way to fix this is to make sure we always fill parameters with non-null values, even if GetTypeSourceInfoForDeclarator does not get called.
An alternative way would be to call GetTypeSourceInfoForDeclarator even for invalid types (IIUC, it can perfectly handle function types, but other types also fail there).

I'll try to explore some these fixes and try to come up with a follow-up change.

[ilya-biryukov]

• Add a test for block pointers

[ilya-biryukov]

I landed the fix to unbreak our crashes and explored a bit of the alternative solution.

Digging a bit deeper, trying to always set non-null parameters causes ~30 test failures, but the ones I looked at so far look more localized and should be fixable.
Some started producing more diagnostics (I assume those are just noise, but I haven't checked closely), it might be trickier to sort those out.

I'll try to spend some more time on it and come up with a better fix some time this week.

[aaron.ballman]

In D146426#4208960, @ilya-biryukov wrote:

In D146426#4207423, @shafik wrote:

As I noted in the bug report not doing D.setInvalidType(); does fix this bug and seems harmless since the error diagnostic should prevent us from getting to codegen but it is not clear to me if this has other negative impacts. Reading your replies it is not obvious you looked at this path or not.

There are too many other code paths that set setInvalidType(), I'll try to add more tests that capture those too.
It would be easy to avoid calling setInvalidType() for __fp16 in particular, but checking for nulls actually saves us from other potential crashes .

I would not mind a quick fix but I think we should have a plan for a more correct fix before we do that.

Based on conversations in this thread, I think the right way is to make sure parameter lists never have nulls.
One way to fix this is to make sure we always fill parameters with non-null values, even if GetTypeSourceInfoForDeclarator does not get called.
An alternative way would be to call GetTypeSourceInfoForDeclarator even for invalid types (IIUC, it can perfectly handle function types, but other types also fail there).

I'll try to explore some these fixes and try to come up with a follow-up change.

Thank you for offering to do that in a follow-up, but please, next time wait for there to be agreement on the patch before landing it. Multiple reviewers expressed concerns that this is papering over a larger bug and didn't have the chance to respond to your comments. Reverting recently broken patches is fine because that gets us back into a better state, but pushing temporary hacks is a different situation entirely (those have a tendency to become permanent and cause problems in the future because the design is more confused). (No need to revert your changes that have already landed, that'll just cause churn, but those changes should be reverted in your follow-up patch.)

Note that the types are already handled properly, but for some reason that I didn't dig out we choose to defer setting the parameter decls until a call to GetTypeSourceInfoForDeclarator.

It's been this way since the function was introduced: https://github.com/llvm/llvm-project/commit/fc8b02281020fa97d34fa98326080052ca0bb565, so this is a very long-standing way of handling things. I did a bit of debugging and can see that the tree transform is given a function prototype that has no ParmVarDecl objects to transform, so the transformed lambda call operator gets null ParmVarDecl objects as well.

GetTypeSourceInfoForDeclarator sets the parameter Decl, but we can't call it when isInvalidType() == true as this causes other assertion failures that seem harder to fix.

The implementation there is inconsistent and I suspect that might be part of the issue. Notice how this code block https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5447 doesn't set the declarator type as invalid despite issuing an error diagnostic, while this block https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5468 does. IIUC, setting the declarator type to be invalid is done to avoid additional diagnostics later (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5608), but GetTypeSourceInfoForDeclarator() doesn't generate any additional diagnostics.

[ilya-biryukov]

In D146426#4209620, @aaron.ballman wrote:

Thank you for offering to do that in a follow-up, but please, next time wait for there to be agreement on the patch before landing it. Multiple reviewers expressed concerns that this is papering over a larger bug and didn't have the chance to respond to your comments. Reverting recently broken patches is fine because that gets us back into a better state, but pushing temporary hacks is a different situation entirely (those have a tendency to become permanent and cause problems in the future because the design is more confused). (No need to revert your changes that have already landed, that'll just cause churn, but those changes should be reverted in your follow-up patch.)

Sorry about that. This looked like a small change and I tried to rush it in to address the crash we had in production.
I wouldn't mind a revert from someone else on the basis of not getting agreement here.
FWIW, new cases started showing up not handled by this patch, so the discussion was fully warranted and I think my original approach does not even help with that.

I tried to look at the problem more closely and I think there is a way to avoid getting null pointers entirely, but we'll have to make the recovery a bit more robust in case of errors.
See D146971 for the first attempt at this approach, let's continue the discussion there.

clang/lib/Sema/SemaChecking.cpp

 /// function are appropriate for the definition of a function. This
 /// takes care of any checks that cannot be performed on the
 /// declaration itself, e.g., that the types of each of the function
 /// parameters are complete.
 bool Sema::CheckParmsForFunctionDef(ArrayRef<ParmVarDecl *> Parameters,
                                     bool CheckParameterNames) {
   bool HasInvalidParm = false;
   for (ParmVarDecl *Param : Parameters) {
+    if (!Param) {
+      HasInvalidParm = true;
+      continue;
+    }
     // C99 6.7.5.3p4: the parameters in a parameter type list in a
     // function declarator that is part of a function definition of
     // that function shall not have incomplete type.
     //
     // This is also C++ [dcl.fct]p6.
     if (!Param->isInvalidDecl() &&
         RequireCompleteType(Param->getLocation(), Param->getType(),
                             diag::err_typecheck_decl_incomplete_type)) {

clang/lib/Sema/SemaDeclCXX.cpp

if (!Check(B.getBaseTypeLoc(), B.getType(), nullptr))
       return false;
   for (const FieldDecl *FD : RD->fields())
     if (!Check(FD->getLocation(), FD->getType(), FD))
       return false;
   return true;
 }
 
 /// Check whether a function's parameter types are all literal types. If so,
 /// return true. If not, produce a suitable diagnostic and return false.
 static bool CheckConstexprParameterTypes(Sema &SemaRef,
                                          const FunctionDecl *FD,
                                          Sema::CheckConstexprKind Kind) {
   unsigned ArgIndex = 0;
   const auto *FT = FD->getType()->castAs<FunctionProtoType>();
   for (FunctionProtoType::param_type_iterator i = FT->param_type_begin(),
                                               e = FT->param_type_end();
        i != e; ++i, ++ArgIndex) {
     const ParmVarDecl *PD = FD->getParamDecl(ArgIndex);
+    if (!PD)
+      return false;
     SourceLocation ParamLoc = PD->getLocation();
     if (CheckLiteralType(SemaRef, Kind, ParamLoc, *i,
                          diag::err_constexpr_non_literal_param, ArgIndex + 1,
                          PD->getSourceRange(), isa<CXXConstructorDecl>(FD),
                          FD->isConsteval()))
       return false;
   }
   return true;

clang/lib/Sema/SemaLambda.cpp

void Sema::CompleteLambdaCallOperator(
   Method->setTypeSourceInfo(MethodTyInfo);
   Method->setType(buildTypeForLambdaCallOperator(*this, LSI->Lambda,
                                                  TemplateParams, MethodTyInfo));
   Method->setConstexprKind(ConstexprKind);
   Method->setStorageClass(SC);
   if (!Params.empty()) {
     CheckParmsForFunctionDef(Params, /*CheckParameterNames=*/false);
     Method->setParams(Params);
-    for (auto P : Method->parameters())
+    for (auto P : Method->parameters()) {
+      if (!P)
+        continue;
       P->setOwningFunction(Method);
     }
+  }
 
   buildLambdaScopeReturnType(*this, LSI, Method, HasExplicitResultType);
 }
 
 void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,
                                                 Scope *CurrentScope) {
 
   LambdaScopeInfo *LSI = getCurLambda();

clang/lib/Sema/SemaTemplateInstantiate.cpp

ExprResult TransformLambdaExpr(LambdaExpr *E) {
       LocalInstantiationScope Scope(SemaRef, /*CombineWithOuterScope=*/true);
       Sema::ConstraintEvalRAII<TemplateInstantiator> RAII(*this);
       ExprResult Result = inherited::TransformLambdaExpr(E);
       if (Result.isInvalid())
         return Result;
 
       CXXMethodDecl *MD = Result.getAs<LambdaExpr>()->getCallOperator();
       for (ParmVarDecl *PVD : MD->parameters()) {
-        if (!PVD->hasDefaultArg())
+        if (!PVD || !PVD->hasDefaultArg())
           continue;
         Expr *UninstExpr = PVD->getUninstantiatedDefaultArg();
         // FIXME: Obtain the source location for the '=' token.
         SourceLocation EqualLoc = UninstExpr->getBeginLoc();
         if (SemaRef.SubstDefaultArgument(EqualLoc, PVD, TemplateArgs)) {
           // If substitution fails, the default argument is set to a
           // RecoveryExpr that wraps the uninstantiated default argument so
           // that downstream diagnostics are omitted.

clang/test/SemaCXX/crash-params.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+template <bool>
+int foo() {
+  auto x = [&](__fp16) { return 0; }; // expected-error {{parameters cannot have __fp16 type}}
+  return 0;
+}
+
+int bar() { return foo<true>(); }