Fundamentally, because I don't think libraries should call abort(). Libraries only rarely know the full context they're running in; aborting other people's code is at best rude, and could e.g. have security implications or cause data loss in less benign cases.
It sounds like what you're objecting to is largely things like what FAudio does, i.e. assert() based on user input (cf. [1] for the assertion, [2] for the problems it caused). That absolutely is something we should avoid.
Well, we've definitely had Wine bugs filed for assertions in the HLSL compiler triggered by trying to compile a particular shader.
I haven't fully checked whether that's the case in this commit, mind.
However, the way I've used it, and the way I think it should be used, is not that; it's rather to document *and* double-check an internal assumption that, if violated, would basically lead to potentially any kind of error.
The alternatives are (1) don't do that, which makes things a lot harder to debug if the error *does* happen, or (2) check and try to gracefully handle everything that would be an assert(), which I think quickly becomes untenable *and* unreadable. [I've seen code that actually does this and it wastes so much time and space.]
The whole point of abort() is that, rude as it is, it's generally the least rude option. It's far safer and more polite than things like (a) pretending to succeed but giving incorrect output; (b) crashing a greater distance away from the root cause, e.g. inside the library user's code.
I don't think that's a decision a library can reliably make.
Moreover:
- Aborting the application may be appropriate in some cases. If the reason the assert gets triggered is e.g. memory corruption, perhaps it makes sense to abort the application. Unfortunately we don't know that in advance; for something like e.g. https://bugs.winehq.org/show_bug.cgi?id=55190, I think aborting is worse that the issue being detected.
The tests don't make it clear, but if we just removed that assertion, we'd be outputting incorrect code. How is that better than aborting?
It mostly depends on the context we're running in. If we're being fed malicious input, returning incorrect output may very well be the better option. But those aren't the only two options we have, of course. For the cases where incorrect output would be the worst that could happen, I think the right answer is pretty much vkd3d_shader_error().
Even if that example ended up being fine in practice, well, we wouldn't have known it in advance. assert() catches invariants that could result in *anything* if violated.
In theory, sure. In practice, I think we have a number of cases where assert() is essentially used as a substitute for input validation. It also tends to make review a bit harder; instead of verifying the condition is handled appropriately, the reviewer needs to verify it can never happen.
- assert() isn't an error handling/reporting mechanism. vkd3d-shader should generally use vkd3d_shader_verror() for that. If we want vkd3d_shader_verror() to abort() for easier debugging, fine; add a local patch that does that, or introduce a VKD3D_SHADER_CONFIG flag. I don't think it should be the default behaviour.
assert() as I'm using it is for catching *internal* assumptions.
With all that said, should we use vkd3d_shader_error(VKD3D_SHADER_ERROR_ICE) instead of assert()? Probably? I think it's mildly objectively better, and about as cheap in most contexts. It doesn't have the NDEBUG tradeoff built in but we could add that easily enough.
I just don't think assert() is awful either.
I think I've been fairly lenient about accepting these in the HLSL compiler in particular, if grudgingly. Still, I'd rather see fewer assert()'s than more.