[PATCH v2 vkd3d 1/4] vkd3d-shader: Handle preprocessor parsing errors.

[Zebediah Figura (she/her)]

On 12/14/20 9:26 AM, Henri Verbeet wrote:
> On Sat, 12 Dec 2020 at 01:52, Zebediah Figura (she/her)
> <zfigura at codeweavers.com> wrote:
>>
>> On 12/11/20 12:11 PM, Henri Verbeet wrote:
>>> On Fri, 11 Dec 2020 at 00:27, Zebediah Figura <zfigura at codeweavers.com> wrote:
>>>> +struct preproc_location
>>>> +{
>>>> +    const char *filename;
>>>> +    unsigned int first_line, first_column;
>>>> +};
>>>> +
>>>>  struct preproc_ctx
>>>>  {
>>>>      void *scanner;
>>>>
>>>> +    struct vkd3d_shader_message_context *message_context;
>>>>      struct vkd3d_string_buffer buffer;
>>>> +    unsigned int line, column;
>>>> +    const char *source_name;
>>>> +
>>>> +    bool error;
>>>>  };
>>>>
>>> There seems to be some duplication of location information between
>>> struct vkd3d_shader_message_context, struct preproc_ctx, and struct
>>> preproc_location. Is that intentional? If it is, would it make sense
>>> to introduce e.g. a struct vkd3d_shader_source_location instead of
>>> struct preproc_location, and then use that structure in both struct
>>> vkd3d_shader_message_context and struct preproc_ctx?
>>
>> Wrt the vkd3d_shader_message_context part—the current structure
>> definitely isn't a great fit for the preprocessor or HLSL parser (i.e.
>> the parts that use yacc) because the location tracking means that we'll
>> be setting a new location on every error call. (And of course most of
>> the other parsers we have don't really have a meaningful concept of
>> line/column locations...) But I wasn't sure enough to actually try to
>> redesign those parts, and it seemed like something that could be
>> deferred, so I just left it alone...
>>
> I probably haven't looked at this series in enough detail, so I may be
> missing something obvious, but is the issue that we're getting an
> explicit location in yyerror() that may not match the location stored
> in struct preproc_ctx? I think it would be fine to pass an explicit
> location to vkd3d_shader_error() and similar functions as well, and
> moving location tracking out of struct vkd3d_shader_message_context.

yyerror() is one example where we need to preserve a location that's not
the "current" one, but it actually ends up being most errors.

In the case of the HLSL compiler, except for a scant few errors
generated by the lexer, pretty much everything has a location attached
that doesn't match the lexer's current location. This is partly because
of lookahead (e.g. we want to complain about variable attributes on a
function, and have the caret point toward the invalid attribute, but we
don't know which one we've parsed yet), partly because bison makes no
guarantees about where the lexer actually is when a given rule is
parsed, and partly because we report some errors *after* having parsed
the whole file [which we do by saving the locations of code that caused
us to generate instructions and other IR objects].

In the case of the preprocessor, it's a bit odd, because most errors can
be blamed on a malformed or misplaced directive [and so even including
the column in the first place doesn't necessarily make a lot of sense].
It'd be arguably possible to always pass the current line and file name,
except that it'd actually need to be the previous line and file name,
since we've already parsed a newline at that point. But this is also
sort of depending on bison internals that I don't think are guaranteed.

> That probably doesn't need to delay this series, but if that change is
> what we want, it seems easier to make it sooner rather than later.

Well, I did add preproc_error() as a helper (and there are already
similar helpers for the HLSL compiler) so that it wouldn't be too much
of a problem. But it is of course nice to not punt the problem, and it's
not too hard...

> 
> It does look like we'd want a struct vkd3d_shader_source_location
> instead of struct preproc_location in that case.
> 
>> Wrt preproc_location vs preproc_ctx—part of the rub here is that {line,
>> column} is a bit different from {first_line, first_column}; i.e. if we
>> were to add {last_line, last_column}, that wouldn't be a part of
>> preproc_ctx.
>>
> Hmm, right.
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x0D9D358A07A17840.asc
Type: application/pgp-keys
Size: 1769 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201214/d51b2b07/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201214/d51b2b07/attachment.sig>