KhronosGroup / SPIRV-Registry

SPIR-V specs
109 stars 72 forks source link

Clarify DWARF Version operand of DebugCompilationUnit #280

Closed MrSidims closed 3 days ago

MrSidims commented 1 week ago

On windows default debug info format is CodeView and we would like to have an ability to express CodeView version in SPIR-V. For OpenCL.DebugInfo.100 there are not much alternatives but to set zero as DWARF Version literal operand of DebugCompilationUnit. Starting from NonSemantic.Shader.DebugInfo.100 there are potential alternatives.

Currently our SPIR-V producer (LLVM IR to SPIR-V translator) generates zero constant as DWARF Version operand of DebugCompilationUnit for CodeView and it becomes hard for the SPIR-V consumer to decide on the debug info version to set.

The questions/proposals I have:

  1. Can we modify DebugCompilationUnit within NonSemantic.Shader.DebugInfo.100 to allow DWARF Version to also be OpString? Like this we could encode not only DWARF, but any other potential debug info formats.
  2. If the answer to the above proposal is "no", then what should be the value of DWARF Version for non-DWARF debug info formats? On the one hand the specification says, that DWARF Version must be a 32-bit integer constant (and so in the translator we generate OpConstant with zero value), on the other hand DebugInfoNone is defined as "Other instructions can refer to this one in case the debugging information is unknown, not available, or not applicable.", which seems to be a perfect fit in this case. Shouldn't the specification mention, that DWARF Version operand can also be an id of DebugInfoNone (or better say any OpConstant operands can be DebugInfoNone?) or current wording is sufficient to say that DebugInfoNone can be used in this case.
MrSidims commented 1 week ago

@baldurk hi, may I ask you to express your opinion on that?

baldurk commented 1 week ago

This was ported directly from the OpenCL spec, as I prioritised making as few breaking changes as possible since the original goal was to make some code that generated debug info usable on vulkan. I don't really understand the purpose of this field or what SPIR-V tools could feasibly use it for, but we definitely can't make backwards-compatible breaking changes.

All the spec says is that this field is the DWARF version that "this specification" is compatible with, meaning the debuginfo itself, but it doesn't define that compatibility and there aren't any external dependencies on DWARF as the spec is self-contained so I'm not sure what that could mean. Unless there's a meaningful interpretationn it seems like every emitter could set this field to '5' to refer to DWARF version 5 and leave it at that, I don't see a more useful value to put there.

I don't understand what you mean by "express CodeView version in SPIR-V" or that you are forced to emit 0 for this field. Whether you separately generate codeview info elsewhere is unrelated to the SPIR-V DebugInfo as they are two different specs. If you wanted to add some codeview information to SPIR-V it would need a separate spec and extension.

VyacheslavLevytskyy commented 3 days ago

It looks like https://github.com/KhronosGroup/SPIRV-Registry/blob/main/nonsemantic/NonSemantic.Shader.DebugInfo.100.asciidoc#missing-debugging-information has so global impact on the specification that it deserves even more attention and clarification, probably within the Introduction. Otherwise there will always be doubts if DebugInfoNone is indeed a valid substitution for each ID in every instruction.

baldurk commented 3 days ago

DebugInfoNone is not globally applicable it's used in specific cases where there may be an ID with no valid value, like a pointer that can point to NULL. These cases are called out specifically - e.g. DebugTypeEnum if there's no underlying type, or DebugGlobalVariable if there's no variable.

VyacheslavLevytskyy commented 3 days ago

Thank you @baldurk . Am I correct saying:

"if there is no explicit statement in the description of the Instruction that Id may be substituted by DebugInfoNone, it's not allowed to use DebugInfoNone instead of this Id" ?

baldurk commented 3 days ago

Yes that's how I understand it, unless there's some missed case where an id is referenced that might need to be allowed as None then it would only be where specified.

MrSidims commented 3 days ago

Yes that's how I understand it, unless there's some missed case where an id is referenced that might need to be allowed as None then it would only be where specified.

That's actually quite surprising for me. If we look at DWARF 5 specification, lets say on the following paragraph:

2.14 Declaration Coordinates Any debugging information entry representing the declaration of an object, module, subprogram or type may have DW_AT_decl_file, DW_AT_decl_line and DW_AT_decl_column attributes, each of whose value is an unsigned integer 16 constant.

Now going to, lets say, a subprogram declaration in SPIR-V debug info specification, DebugFunctionDeclaration:

Column is the of a 32-bit integer OpConstant denoting the column number at which the first character of the function declaration appears.

So whilst column information would be an optional information in DWARF - it becomes mandatory in SPIR-V.

And one of the problems that OpenCL.DebugInfo.100 had - is that many debug information there is described by literals, which can't be substituted by DebugInfoNone instruction, and I was thinking, that Nonsemantic.Shader.100 is solving by making every operand to be an id of another instruction always (guess the main reason for ids is that they solve other problems with parsing, but still).

baldurk commented 3 days ago

I'm not familiar with DWARF but I don't think that specification is relevant here as we're talking about the SPIR-V NonSemantic.Shader.DebugInfo.100.

The convention I've seen for column information if it's not available is to set both start and end to 0, though you're right that this should be stated explicitly.

The change from literals was not so that IDs could be substituted with DebugInfoNone, indeed literals would be much simpler for most cases they're used for. It was necessary due to the original requirement from the SPIR-V working group that non-semantic extended instruction sets have IDs for all parameters so that tools could theoretically process them without understanding them.

MrSidims commented 3 days ago

I don't think that specification is relevant here as we're talking about the SPIR-V NonSemantic.Shader.DebugInfo.100.

Agree. And in a flow SPIR-V -> (dwarf producer) -> DWARF it's fine to put stricter rules for SPIR-V.

The convention I've seen for column information if it's not available is to set both start and end to 0, though you're right that this should be stated explicitly.

I'm a bit afraid that we would need to specify quite a lot of defaults then, when debug information is not available. On the other hand it's better, then to blindly rely on DebugInfoNone leaving DWARF producer to decide, what attribute to pick (if applicable).

baldurk commented 3 days ago

Yes I agree blanket allowing DebugInfoNone all over the place would make the situation significantly harder for consumers to handle normal cases. I don't think producing DWARF information from SPIR-V's NonSemantic debug info was ever a goal, that seems like it would have to be 'best effort' if the information is there.

If there are other situations where information could reasonably be optional that seems like a case-by-case basis for spec clarification but I would hope they are rare - aside from fields like in the original issue where it's inherited from the OpenCL debuginfo and doesn't make sense.

Honestly I would personally say that column information shouldn't be optional either, at worst it should just refer to the whole length of the source line if no sub-expression column information is available, but it's a common one for frontends not to generate so there's the convention of leaving 0.

MrSidims commented 3 days ago

but it's a common one for frontends not to generate so there's the convention of leaving 0.

Sure, setting column number to zero is fine (that's actually what we already do in LLVM IR to SPIR-V translator).

I'll scrub, what we have done for https://github.com/KhronosGroup/SPIRV-LLVM-Translator - there are not many places, where DebugInfoNone is being generated to see, where it can be defaulted. If there are objectively impossible to substitute info with some default value, I'll open a PR to the spec and ask you to review :)

And thank you for the answers!