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

[Zebediah Figura (she/her)]

On 10/7/21 02:32, Giovanni Mascellani wrote:
> 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.

Indeed, I misread it. Could we check the string_buffer itself for NULL 
as well, then, like you describe below? Maybe in a separate patch...

> (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 think that's inescapable; if we run out of memory we are going to do 
*something* wrong. Likely we can't even print the message in the first 
place.

>> 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.
>