Added SDL_HINT_INVALID_PARAM_CHECKS and SDL_HINT_INVALID_PARAM_ACTION

[slouken]

This is the more complete parameter validation pass that was out of scope for SDL3 release. I've currently set the default behavior of debug SDL builds to assert if parameter validation fails, and release builds to skip parameter validation entirely. We can adjust this as needed.

sdl2-compat will probably want to set hints for games that rely on parameter validation, or just always enable fast parameter validation.

This closes #13213

[slouken]

I've currently implemented it for the renderer APIs, but if we like this direction, I can pretty easily extend it to other subsystems.

@icculus, @sezero, @madebr, @smcv, @AntTheAlchemist, @mechakotik, @bjorn, thoughts?

BTW, @madebr, @icculus, I noticed that asserts aren't enabled in debug builds. The logic we use here isn't correct for cmake on Windows:

SDL/include/SDL3/SDL_assert.h

Lines 97 to 104 in de1b52f

	#ifdef SDL_DEFAULT_ASSERT_LEVEL
	#define SDL_ASSERT_LEVEL SDL_DEFAULT_ASSERT_LEVEL
	#elif defined(_DEBUG) || defined(DEBUG) || \
	(defined(__GNUC__) && !defined(__OPTIMIZE__))
	#define SDL_ASSERT_LEVEL 2
	#else
	#define SDL_ASSERT_LEVEL 1
	#endif

It turns out that Microsoft Visual Studio doesn't actually define _DEBUG when /MDd is passed on the command line, that just appears to be a convention for default projects. The approach I took in SDL_internal.h is the only one that works on Windows for both CMake and Visual Studio. Since keying off of NDEBUG is the convention for disabling asserts in assert.h, we probably want to use that for SDL_assert() as well.

[AntTheAlchemist]

Very nice.

I see NDEBUG enables checking at hint level. And do I see that SDL_DISABLE_PARAMETER_CHECKS disables it at compile-time? That's exactly what I want. There's an option for everyone.

Should we have a comprehensive list of preprocessor options for SDL_LEAN_AND_MEAN / SDL_GPU_DISABLED & co?

[bjorn]

thoughts?

While I think it's great to see such a comprehensive change to address this issue, speaking for myself I think it's unfair to supersede PR #13213 by @mechakotik in this way. They have already put in significant effort and have been waiting for feedback for months.

Their PR solved the issue quite elegantly by reusing the SDL_ObjectValid functionality, thereby needing to touch much less code. I do see that this new approach is significantly different in that it affects not only "valid object" checks but all kinds of preconditions on parameters, but I think the "valid object" part of the change could have been taken from the original PR, meaning this change could have been a follow-up.

I've currently set the default behavior of debug SDL builds to assert if parameter validation fails, and release builds to skip parameter validation entirely. We can adjust this as needed.

I think assertion-enabled debug and fast-but-crashing release builds matches the generally expected defaults.

[smcv]

This evaluates invalid three times, is that ever going to be a problem? If yes, then it should assign the result of evaluating invalid to a local variable, and then use that local variable everywhere.

[smcv]

If there's a concern about performance, it might be worthwhile to introduce an equivalent of GLib's G_LIKELY (which wraps gcc __builtin_expect) and use that to tell the compiler to assume that parameter checks "usually" succeed.

Another technique I've seen in other libraries is to do the actual parameter check inline, but offload all the handling of what happens when it wasn't true into some other internal function, like this pseudocode using gcc statement expressions:

#define check(condition) \
({ \
  bool _result = LIKELY (!!(condition)); \
  if (!_result) \
    handle_check_failure(#condition); \
  _result;
})

(I can't immediately see how to do this in portable C though...)

[slouken]

@icculus, sanity check here?

[slouken]

@madebr, did I do the CMake changes correctly? They appear to work here in Visual Studio.

[mechakotik]

Wouldn't it be cleaner to bypass validity checks inside SDL_ObjectValid like I did originally? It wouldn't require to change code around every validity check, and should be inlined anyway if using LTO. For guaranteed inlining you can also make SDL_ObjectValid a globally defined macro.

Also, is NULL check bypass really useful? NULL check is one integer comparison that costs nothing compared to amount of work most SDL functions do, so performance benefit of disabling them would probably be unnoticeable. You also check whether SDL_INVALID_PARAM_CHECKS_FAST is set, which is the same integer comparison that one NULL check is, so is there a benefit from this at all?

[icculus]

@icculus, sanity check here?

This seems super heavy-weight. Is this something we really want to do, vs just having if statements that return an error?

[AntTheAlchemist]

I still vote for all the checks just to be wrapped behind a compile-time option. I don't even want any parameters checked for NULL. My apps already know parameters are valid, and doesn't need the same object checked thousands of times each frame - hundreds of thousands per second - this is a significant performance hit.

[madebr]

About NDEBUG vs DEBUG/_DEBUG, only NDEBUG is documented by the C standard. Its only job is to disable assert from assert.h.

Disabling all assertions when NDEBUG is defined (#define SDL_ASSERT_LEVEL 0) is tricky because it'd also also handicap the SDL_gpu debug mode. So perhaps that should be rechecked.

Configuring the assert levels depending on NDEBUG and DEBUG/_DEBUG could like as following:

#if defined(NDEBUG)
#define SDL_ASSERT_LEVEL 0     // enabled: none                          | Disabled: SDL_assert_paranoid SDL_assert SDL_assert_release
#elif defined(DEBUG) || defined(_DEBUG)
#define SDL_ASSERT_LEVEL 2     // enabled: SDL_assert_release SDL_assert | Disabled: SDL_assert_paranoid
#else
#define SDL_ASSERT_LEVEL 1     // enabled: SDL_assert_release            | Disabled: SDL_assert_paranoid SDL_assert
#endif

If we really want to keep SDL_assert_release enabled for NDEBUG, then the current behavior is correct already.

[slouken]

If we really want to keep SDL_assert_release enabled for NDEBUG, then the current behavior is correct already.

Yes, SDL_assert_release() should be enabled when NDEBUG is defined.

[slouken]

Okay, thanks for the feedback, everyone. I'm going to mark this draft for more thought.