[PATCH vkd3d 3/4] vkd3d-shader: Add support for retrieving the shader version through vkd3d_shader_scan().

[Henri Verbeet]

On Fri, 7 Jan 2022 at 11:44, Giovanni Mascellani
<gmascellani at codeweavers.com> wrote:
> On 06/01/22 11:15, Henri Verbeet wrote:
> > On Tue, 28 Dec 2021 at 23:20, Zebediah Figura <zfigura at codeweavers.com> wrote:
> >> Besides being the first patch implementing reflection, this patch introduces an
> >> API problem with regard to D3DReflect() that will affect many different fields.
> >> Namely, if the SM4 version token contains a nonsensical version D3DReflect()
> >> will happily return it verbatim. As I see it our options are:
> >>
> >> (1) return the token verbatim anyway instead of using struct
> >>      vkd3d_shader_version,
> >>
> >> (2) don't use vkd3d_shader_scan() for wine's D3DReflect() [and either diverge
> >>      from native in vkd3d-utils' D3DReflect(), or don't use vkd3d_shader_scan()
> >>      there either]
> >>
> >> (3) return the token verbatim in a separate field (and in a separate chained
> >>      structure?)
> >>
> >> (4) punt, and use one of the above solutions if we ever do come across a shader
> >>      that contains insane data.
> >>
> > I'd be inclined to go with option 3 here.
>
> I don't think I have a complete understanding of the matter, but based
> on what I gather I'd say that it makes sense to report both interpreted
> and uninterpreted data, so that the caller can choose what they believe
> it's best for them. I guess it's option 3 here. No strong opinion of
> whether to using the same or another chained structure (weak opinion is
> to use the same).
>
In the abstract, sure. In practice, I'm not sure how much value there
would be in returning e.g. a vkd3d_shader_version or creator string
for HLSL source, or e.g. SPIR-V source if we ever ended up supporting
that.

> >> * Instruction counts (and I think everything else in the STAT chunk) are always
> >>    taken from the STAT chunk; if the STAT chunk is missing then D3DReflect() will
> >>    return zero, even though it could just count them. Should we preserve that
> >>    behaviour in libvkd3d-shader? in libvkd3d-utils?
> >>
> > If we're going with something like
> > vkd3d_shader_tpf_scan_info/vkd3d_shader_d3d_scan_info, probably yes.
>
> In the same philosophy, I'd make both the reported and recomputed data
> available, so that the caller can choose. If it is expensive to
> recompute, I'd gate that with a flag.
>
I don't think we'd need a flag; if the relevant _scan_info structure
is missing from the chain, we could avoid retrieving the relevant
information, while if the application doesn't need that information,
it should probably avoid adding the relevant structure to the
structure chain.