So it seems like what we're ultimately going to do with NEXT_STAGE_INFO is to construct an output map for the vertex shader. One (perhaps obvious) alternative then would be to pass that output map to vkd3d-shader directly, instead of NEXT_STAGE_INFO. The main advantage would be that such an output map could be stored in a fairly compact way if needed, and could easily be compared with memcmp().
There may also be variants we could do based on that. E.g., keep passing NEXT_STAGE_INFO to vkd3d_shader_compile(), but introduce a function that returns an output map consistent with what vkd3d_shader_compile() would use for a given signature pair. Or we could return the output map that would be used from vkd3d_shader_scan().
Yeah, that was broadly what I was trying to propose with (c).
Not sure if I should take this as an endorsement or preference of that approach, but I at least can't say I like anything else better, so I may just try to start writing an implementation thereof.
Side note: maybe this should be a vkd3d_shader_parameter instead? I think it'd probably be good to lean in that direction, rather than adding more chained structures, for new compilation options like this. Mostly because it's easier to design and, I think, easier to use. It'll require rerolling vkd3d_shader_parameter which unfortunately can only have uint32_t in it, but we needed that anyway.
As a side note—when I wrote vkd3d_shader_next_stage_info I had in mind that it could be extended with other aspects of the next shader. The only one of these that I can currently actually think of is the shader type, which I think GLSL cares about sometimes.
We could, and it might not even be that hard, but it would of course introduce some extra allocations and copying that would be superfluous for more typical usage. In particular, the expectation is that we have essentially two kinds of uses of the API/structure:
- Users that keep neither the signature nor the shader around. E.g. because the user of the API is just printing some information about the shader, or e.g. because the user of the API is using its own structures to store whatever information it actually cares about.
- Users that keep both the signature and the shader around. E.g. because the API user isn't particularly concerned about just keeping the shader around, or because it needs to keep the shader around anyway because it will be needed again later.
I.e., the expectation is that it should be fairly uncommon to have a need to hold on to a struct vkd3d_shader_signature, but not the corresponding shader.
It's a fair point, although I am still sort of worried that an API user isn't going to see the design in that terms, especially not a priori (I mean, I didn't), and we'd risk needlessly frustrating with what looks like a bug.
Or, perhaps more saliently, I could see someone assuming (despite the documentation) or forgetting about that implicit dependency, and causing subtle UAF problems as a result.
Or I could see an argument for someone wanting to, say, compile a shader into SPIRV, keep the compiled shader and the signature around, and throw away the original.
Which is not me trying to belabour the argument in favor of "fixing" the "bug"—I'm not even fully convinced that it needs fixing either—but it does make me nervous.
Note that "just fix it" doesn't make the issue in the commit in question that much easier to fix either; we'd still need to keep track of when it's safe to release the signature. We could wrap the signature in some structure with a refcount in wined3d, but in that case we could also just keep a reference to the shader the signature belongs to, and that might not end up being much worse in practice.
Also true. I can't claim to have fully thought this part out, but I think I was figuring that we'd just keep the list of variants around forever, even if one or more of the corresponding vertex shaders is destroyed. [Especially because... how likely is it that the VS's even will be?]
There is a performance question here—do we care about avoiding shader recompilation when multiple different PS's with identical signatures are used? Wine seems to care, at least enough to avoid recompiling the VS into GLSL.