[PATCH] D90345: [DebugInfo] Fix ICE in DwarfCompileUnit::constructSubprogramScopeDIE

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

In D90345#2888746 <https://reviews.llvm.org/D90345#2888746>, @dblaikie wrote:

> In D90345#2888538 <https://reviews.llvm.org/D90345#2888538>, @scott.linder wrote:
>
>> In D90345#2888514 <https://reviews.llvm.org/D90345#2888514>, @dblaikie wrote:
>>
>>> In D90345#2887719 <https://reviews.llvm.org/D90345#2887719>, @scott.linder wrote:
>>>
>>>> Update tests.
>>>>
>>>> I went the route of "upgrading" bitcode which lacks the `type:` field on `DISubprogram`s. It seems like there is currently no difference from LLVM's perspective between `!DISubroutineType(types: !{})` and `!DISubroutineType(types: !{null})`, or at least the difference (or whether the first form should even be valid) is not clear to me.
>>>>
>>>> As an example, DebugTypeInfoRemoval uses `!DISubroutineType(types: !{})` to represent the C type `(void)()`, but the langref would lead one to believe this should be `!DISubroutineType(types: !{null})`.
>>>>
>>>> Does anyone have any thoughts?
>>>
>>> I'd lean slightly towards `!{null}` being the representation for a known signature of `void()` - for consistency.
>>
>> Would it be worth preceding this patch with another that enforces this? It seems like a verifier check, doc update, and bitcode reader upgrade path from `!{}` to `!{null}` for `DISubroutineType` would clear things up. If so, I can do that and then update this patch.
>
> Maybe? Though might be more effort than its worth - it's probably pretty harmless for !{null} and !{} to be treated the same - especially if they are already supported? But if we're trying to pick a representation to use in new cases where it comes up I'd lean towards !{null}.
>
> Open to other opinions - @aprantl and co have more context/care more about the verifier .

I also think we should go with LangRef here. Clang currently also emits `void f() {}` as `!DISubroutineType(types: !{null})`. If have the time, you could write a separate patch to add a Verifier pass that rejects an empty list and then fix up all testcases, but as @dblaikie it's pretty harmless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90345