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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D146426#4207118 <https://reviews.llvm.org/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 errors with no locations and diagnostic errors.
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 <https://github.com/llvm/llvm-project/blob/f67b481098cc30567d0f50a2b21f8f57b92052bd/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L4425>.

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 <https://reviews.llvm.org/D146426#4207146>, @erichkeane wrote:

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146426