This makes most vkd3d-shader insufficiencies a lot more obvious when used as an embedded library.
The obvious disadvantage here is that vkd3d-compiler will basically print every fixme message twice.
This is a bit of an RFC, since I'm not married to this patch. Other potential approaches I see (not all mutually exclusive):
* Always print a FIXME if we return VKD3D_ERROR_NOT_IMPLEMENTED from compile_hlsl(), but don't print messages for individual compilation errors. Thus we clue in debuggers that a bug is related to HLSL, but let them use VKD3D_SHADER_DEBUG to find out what the actual bug is. Since the first step is most of the hard part I think this is a reasonable solution.
* Or do the same thing from d3dcompiler.
* Relegate hlsl_fixme to a separate vkd3d_shader_log_level, and print messages from that log level in d3dcompiler by default. This isn't quite as simple as that though, since currently all the messages are stored in a single blob.
* Extra APIs to control whether FIXMES are printed.
I'd also like to propose extending whichever of the above solutions to the hlsl_error() / VKD3D_ERROR_INVALID_SHADER cases. That way we catch things which we aren't going to catch via hlsl_fixme(), such as syntax errors from unrecognized keywords. There's a counterargument that compiling an invalid shader is valid usage, and that we (wine or vkd3d) shouldn't print any ERR/FIXME messages when a program does something valid and implemented, but I'd counter that in practice intentionally compiling an invalid shader should be exceedingly rare, and especially for something like wine or vkd3d where debugging is difficult, it's simply not worth insisting on "a correctly functioning program should print no messages" if it's at the cost of making debugging that much harder.
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/hlsl.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 8591fe31..93a44f9d 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -59,12 +59,16 @@ void hlsl_fixme(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc, c { struct vkd3d_string_buffer *string; va_list args; + size_t len;
va_start(args, fmt); string = hlsl_get_string_buffer(ctx); vkd3d_string_buffer_printf(string, "Aborting due to not yet implemented feature: "); + len = string->content_size; vkd3d_string_buffer_vprintf(string, fmt, args); vkd3d_shader_error(ctx->message_context, loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "%s", string->buffer); + if (len >= 0) + FIXME("%s\n", string->buffer + len); hlsl_release_string_buffer(ctx, string); va_end(args);
I don't care much for this particular patch; mostly because it's an approach to this issue that I had previously considered and rejected.
The obvious disadvantage here is that vkd3d-compiler will basically print every fixme message twice.
Yes, unless vkd3d-compiler sets its own debug callbacks or fiddles with VKD3D_SHADER_DEBUG at least.
Fundamentally, I think vkd3d-shader compiler output should go through vkd3d_shader_message_context instead of TRACE/WARN/FIXME/ERR. That's not to say it doesn't make sense for d3dcompiler and/or vkd3d to be more verbose, but I don't think we should be making that decision on the vkd3d-shader level. It's perhaps also worth pointing out that there are parallels to handling GLSL compiler output in wined3d.
The question would then be whether the existing API is sufficient for d3dcompiler, vkd3d, and wined3d. If it's not, we could certainly introduce additional API to fix that. For example, we could introduce callbacks that receive compiler output with the location, log level, error code, and message already separated out. It's not necessarily clear to me that some variant of what wined3d does in shader_spirv_compile_shader() wouldn't be sufficient for d3dcompiler/vkd3d though.
Fundamentally, I think vkd3d-shader compiler output should go through vkd3d_shader_message_context instead of TRACE/WARN/FIXME/ERR.
Yes, although I think there's also clearly some things that belong in the debug channels. We still have a few FIXME that should be hlsl_fixme(), but...
The question would then be whether the existing API is sufficient for d3dcompiler, vkd3d, and wined3d. If it's not, we could certainly introduce additional API to fix that. For example, we could introduce callbacks that receive compiler output with the location, log level, error code, and message already separated out. It's not necessarily clear to me that some variant of what wined3d does in shader_spirv_compile_shader() wouldn't be sufficient for d3dcompiler/vkd3d though.
Broadly, except that currently the program might suppress the log level, and we probably don't want to print warnings by default.
I'm hesitant to make the location and error code fully structured, because I think both could easily change. (Locations can be of varying levels of specificity and sophistication, and error codes can easily constrain us to a poorly organized classification.) But separating messages by log level at least could make sense.
Actually, I guess now is a good time to bring up a related discussion—the error code schema doesn't make much sense to me currently. I can see the point of applying codes to warnings, so that they can be selectively disabled. I'm not sure it's the best way to handle it in general, though. I think the gcc approach is more expressive and allows reclassifying some warnings, e.g. you can divide up what was previously one warning into multiple by applying an extra argument (consider -Wshift-overflow). I suppose we could claim no API stability around warnings, though; it's not like gcc guarantees that either...
I don't see the point of applying codes to errors, though. Disabling or even reporting them selectively doesn't make sense.
The other main RFC related to warnings is: currently we have not been matching the native HLSL compiler at all with respect to warnings. Should that change? One good reason it potentially should is to allow "#pragma warning" to work as intended.
Fundamentally, I think vkd3d-shader compiler output should go through vkd3d_shader_message_context instead of TRACE/WARN/FIXME/ERR.
Yes, although I think there's also clearly some things that belong in the debug channels. We still have a few FIXME that should be hlsl_fixme(), but...
Sure; and I think hlsl is pretty decent at this these days. That's not necessarily the case for some of the other code, like dxbc-tpf or spir-v.
The question would then be whether the existing API is sufficient for d3dcompiler, vkd3d, and wined3d. If it's not, we could certainly introduce additional API to fix that. For example, we could introduce callbacks that receive compiler output with the location, log level, error code, and message already separated out. It's not necessarily clear to me that some variant of what wined3d does in shader_spirv_compile_shader() wouldn't be sufficient for d3dcompiler/vkd3d though.
Broadly, except that currently the program might suppress the log level, and we probably don't want to print warnings by default.
If we're talking about D3DCompile2(), I don't think that does log levels? We always pass VKD3D_SHADER_LOG_INFO, at least. The application may decide to pass NULL for "messages_blob", of course, but we still retrieve the messages from vkd3d_shader_compile() in that case; we'd just have to output them.
I'm personally not too worried about outputting warnings by default from D3DCompile2(), but at least in the cases where applications pass NULL for "messages_blob" we could also adjust the log level based on the enabled Wine debug channel. If we're particularly worried about this we could try to filter the messages or recompile with a different log level. I'm not necessarily convinced that's worth it, but the option does exist.
If we do end up introducing API for this, I'd want to be fairly careful about it; ideally we wouldn't need to introduce a v2 and v3 of that API within a couple of releases.
Actually, I guess now is a good time to bring up a related discussion—the error code schema doesn't make much sense to me currently. I can see the point of applying codes to warnings, so that they can be selectively disabled. I'm not sure it's the best way to handle it in general, though. I think the gcc approach is more expressive and allows reclassifying some warnings, e.g. you can divide up what was previously one warning into multiple by applying an extra argument (consider -Wshift-overflow). I suppose we could claim no API stability around warnings, though; it's not like gcc guarantees that either...
I don't see the point of applying codes to errors, though. Disabling or even reporting them selectively doesn't make sense.
The other main RFC related to warnings is: currently we have not been matching the native HLSL compiler at all with respect to warnings. Should that change? One good reason it potentially should is to allow "#pragma warning" to work as intended.
A few considerations went into this:
- If I were to get e.g. an "E5003: Argument to preincrement operator is const." error from vkd3d-compiler, it would be relatively straightforward to map E5003 back to VKD3D_SHADER_ERROR_HLSL_MODIFIES_CONST, and find all the places in vkd3d-shader that could possibly generate that error. It would be slightly harder to get there from just the message. Similarly, it would be relatively straightforward to search for error codes in e.g. Bugzilla.
- I don't know whether we'll ever get translations for vkd3d-shader output, but it's certainly a possibility. This would only amplify the previous point.
- I'm not aware of any applications that depend on receiving specific error codes from the HLSL compiler, and I certainly hope they don't exist, but stranger things have happened. If we do ever run into one of those, we'd have the ability to remap error codes in d3dcompiler.
If we're talking about D3DCompile2(), I don't think that does log levels? We always pass VKD3D_SHADER_LOG_INFO, at least. The application may decide to pass NULL for "messages_blob", of course, but we still retrieve the messages from vkd3d_shader_compile() in that case; we'd just have to output them.
You're right, it doesn't. I somehow had gotten it into my mind that there was a way to disable warnings, but no...
This merge request was closed by Zebediah Figura.
-
Henri Verbeet (＠hverbeet) -
Zebediah Figura
-
Zebediah Figura (＠zfigura)