[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

Tue Mar 21 07:27:06 PDT 2023

aaron.ballman added a comment.

In D146426#4208960 <https://reviews.llvm.org/D146426#4208960>, @ilya-biryukov wrote:

> In D146426#4207423 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146426