[PATCH vkd3d] vkd3d-shader: Allow a NULL buffer in vkd3d_string_buffer.

[Giovanni Mascellani]

Hi,

Il 06/10/21 16:54, Zebediah Figura (she/her) ha scritto:
>> Mmh, I am not sure I understand why that would lessen the boilerplate. 
>> You still need to name and destroy the string buffer, each of which 
>> takes a line. And vkd3d_string_buffer_data(string) is definitely 
>> longer than string->buffer. I don't really think this helps here.
> 
> Well, it lessens the boilerplate if you no longer need to check for 
> success from hlsl_type_to_string().

In this form, you still have to check for the result of 
hlsl_type_to_string. My patch doesn't change the fact that 
hlsl_type_to_string can still return NULL if it cannot allocate the 
vkd3d_string_buffer as a whole. Notice that my patch deals with the 
buffer field of vkd3d_string_buffer being NULL, but the whole structure 
still needs to be correctly allocated.

(en passant, I also notice that hlsl_type_to_string has insufficient 
error checking: it doesn't check the return value of 
vkd3d_string_buffer_prints, so if allocations fails there it will return 
an empty string; this doesn't lead to UB, but it generates a bad error 
message, which is still a bug, although much smaller)

> I mean, it's very convenient to be able to not have to check for success 
> from an allocation, in some way or another, if you're going to be doing 
> that frequently. vkd3d_string_buffer_data() achieves that, so I think 
> it's useful to see that as an end goal of vkd3d_string_buffer_data() in 
> itself, and to take advantage of it to get rid of ubiquitous error 
> checking.

Well, I guess this can be added as an additional goal, but we're not 
there yet. Taking this example of the error boilerplate:

>                 struct vkd3d_string_buffer *string;
> 
>                 string = hlsl_type_to_string(ctx, $3);
>                 if (string)
>                     hlsl_error(ctx, @3, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
>                             "Matrix base type %s is not scalar.", vkd3d_string_buffer_data(string));
>                 hlsl_release_string_buffer(ctx, string);
>                 YYABORT;

You cannot remove the "if (string)", because hlsl_type_to_string can 
still return NULL, and vkd3d_string_buffer_data wouldn't like it (no 
problems if string->buffer is NULL, but segfault if string itself is NULL).

It's true that vkd3d_string_buffer_data might be replaced with something 
like:

> static inline const char *vkd3d_string_buffer_data(const struct vkd3d_string_buffer *buffer)
> {
>     return buffer && buffer->buffer ? buffer->buffer : "";
> }

In this case it would work as you say, though it would risk to generate 
a bad error message: "Matrix base type  is not scalar.". As I said, less 
grave than UB, but still a bug. If you want to really achieve what you 
say, it seems to me that more hack^W engineering is required (and I am 
not against it).

Giovanni.