On Thu Aug 31 12:50:17 2023 +0000, Henri Verbeet wrote:
My intention is to have this essentially as a debug and documentation
tool. As the `assert()` implies, ideally in no case an IR error should ever be detected, because it is assumed that the frontend already generates correct code, or fails if some error in its own input is detected. So we'd probably want to skip it when not developing/debugging then, right? That may also make it a bit more acceptable to do something drastic like calling abort() as the result of a validation failure, or doing potentially more expensive kinds of validation.
I think we already discussed this in the past, but I disagree: I find
that using `assert()` is useful to document the assumptions of some piece of code. If those assumptions end up being violated, there are two sensible behaviors: 1) do nothing, hoping for the best; or 2) make the world explode, so that a developer is more likely to check why something that should have never happened really happened. You sound like you're from camp 1, Not exactly, I'd like more options than those two. (In particular, I think "reject the specific input" is the right choice here. We can make it an ICE if you like.) Fundamentally though, my arguments are this:
- assert() is not a robust way to abort on unexpected input, even if
we would like to abort the process instead of just aborting compilation of that specific shader. Calling abort() would already be better in that regard, although I think it's rarely appropriate for libraries to call abort() as well.
- We're a library, we might get invalid or malicious input, and we
can't possibly oversee all the consequences of aborting the calling process. (And I'll point out that that's just as true for d3d11 or d3dcompiler as it is for vkd3d-shader.) And it shouldn't be terribly hard to think up scenario's where "abort the process on unexpected input" isn't the most appropriate course of action. E.g., imagine we're running some kind of shader translation web service. People submit their shaders to our web/application server, the server translates those shaders to the desired output format, and sends the results back. It might not be a terribly robust design to run shader translation in the context of the web/application server, but I don't think it's necessarily unreasonable, and who am I to judge. In this scenario, it would seem more appropriate to me to fail compilation when someone submits a shader that manages to trip our checks than to make the server explode.
I agree that `assert()` is not an appropriate way to deal with unexpected or untrusted input. My point is that that `assert()` should never trigger, even on unexpected or untrusted input, because I consider the frontend's job to deal with such input. In other words, my expectation from the frontend is that it's free to reject its input (and in that case no IR is generated nor validated), but if it accepts the input then it takes the responsibility to generate correct and valid IR; so the IR read by the validator is already considered trusted, and the `assert()` call is there just to, well, assert that. If we don't `assert()` we're likely in UB domain anyway, because the backend will assume that the IR is correct and count on it. If the IR is not valid vkd3d could crash anyway, and by that point I believe it's better to crash in a repeatable way with a clear error message rather then ending up tripping whatever UB will bring us any time later.
However, I don't mean to insist too much on this detail. I can remove the assertion and just rely on logging; I can also gate the check with `TRACE_ON()`, which hopefully addresses your performances concern. I should mention that at some point I contemplated making this a `FIXME` rather than a `TRACE`, so that it is more conspicuous on logs posted by users, but that makes sense only if we accept the validator to be always run.