[PATCH v2 1/2] winevulkan: Manually fixup struct alignment for VkPipelineCreationFeedback.

[Georg Lehmann]

On 15.02.22 19:49, Zebediah Figura wrote:
> On 2/9/22 11:20, Georg Lehmann wrote:
>>
>>
>> On 09.02.22 18:07, Zebediah Figura wrote:
>>> On 2/9/22 10:39, Georg Lehmann wrote:
>>>> We can't reasonably auto generate this because it's output in an
>>>> otherwise input pNext chain.
>>>
>>> Aren't we going to need to do that anyway, though, for many other
>>> functions, for wow64 support?
>>>
>>> It seems like it'd be more worthwhile to add infrastructure for
>>> automatically generating this, or at least some of it.
>>>
>>
>> As far as I can tell this is the only pNext chain like this. We don't
>> handle pure output chains, that's something that needs to be automated
>> for wow64. But this issue is an unique edge case.
> 
> I'll admit this doesn't make me feel comfortable, not when Vulkan is a 
> fast developing API that we are making an effort to keep up with. I.e. 
> even if there aren't any others now it seems not unlikely we'll come 
> across more of these in the future. And if we can find a solution that 
> allows handling partial and wholly output chains in one breath, that 
> seems nice...
> 
> It seems like most such structs are marked "returnedonly"; can we use 
> that to determine what needs output conversion? It's missing from 
> VkPipelineCreationFeedbackCreateInfo, but maybe that's an error.
> 

It's not quite as simple because some structs are input in one chain but 
output in another. Like all the feature structs, which you use to query 
the available features but also to enable them on device creation.
Generally everything in a const void* pNext chain is input and 
everything in a void* pNext chain is output, but 
VkPipelineCreationFeedbackCreateInfo is the exception to that rule.

>>
>>>> +static void fixup_pipeline_feedback(VkPipelineCreationFeedback
>>>> *feedback, uint32_t count)
>>>> +{
>>>> +#if defined(USE_STRUCT_CONVERSION)
>>>> +    struct host_pipeline_feedback
>>>> +    {
>>>> +        VkPipelineCreationFeedbackFlags flags;
>>>> +        uint64_t duration;
>>>> +    } *host_feedback;
>>>> +    int64_t i;
>>>> +
>>>> +    host_feedback = (void *) feedback;
>>>> +
>>>> +    for (i = count - 1; i >= 0; i--)
>>>> +    {
>>>> +        memmove(&feedback[i].duration, &host_feedback[i].duration,
>>>> sizeof(uint64_t));
>>>
>>> Do we need memmove() here?
>>>
>>
>> These partially overlap so I think memmove is only way to do it safely.
> 
> Right, of course.
> 
>>
>>>> +        feedback[i].flags = host_feedback[i].flags;
>>>> +    }
>>>> +#else
>>>> +    (void)feedback;
>>>> +    (void)count;
>>>
>>> This is unnecessary; we don't use -Wunused-parameter.
>>>
>>
>> Personally I like being explicit here, but if you want I can send a new
>> version with these removed.
> 
> We don't do this elsewhere in Wine, for whatever that's worth. (Nor do I 
> think it's worthwhile, given how many unused parameters there are to 
> Windows functions.) But regardless winevulkan isn't code I work on, so I 
> don't really intend to prescribe coding style for it.