VM_DllSyscall is hacky

[mrwonko]

I'm not a big fan of MP's legacy VM_DllSyscall, which even has its hackiness partially documented*; its purpose is passing its arguments as an array to CL_CgameSystemCalls/SV_GameSystemCalls/CL_UISystemCalls.

On some platforms it assumes vararg arguments are laid out sequentially in stack, which is somewhat questionable but arguably acceptable since that behavior can't change on those platforms due to binary compatibility. But on other platforms it calls va_arg 15 times to copy the arguments into an array of intptr_t. This is undefined for two reasons:

If va_arg is called when there are no more arguments in ap, or if the type of the next argument in ap (after promotions) is not compatible with T, the behavior is undefined, unless:

• one type is a signed integer type, the other type is the corresponding unsigned integer type, and the value is representable in both types; or
• one type is pointer to void and the other is a pointer to a character type (char, signed char, or unsigned char).

(From http://en.cppreference.com/w/cpp/utility/variadic/va_arg)

It both makes incompatible conversion (e.g. int to intprt_r; and I'm not even talking about 64 bit**) and reads more arguments than are available.

I know this is merely in the legacy interface, which any new platforms are unlikely to use, but it still makes me uneasy. I suggest we adjust the system call client functions to instead of taking an array be vararg themselves, with them doing the unpacking using va_arg with the appropriate types. This may be marginally slower than the current stack pointer hack, but I doubt it's a bottleneck and it just feels wrong as it is.

Permission to proceed?

* codemp/qcommon/vm.cpp:67
** not least because all legacy mods are 32 bit. But it's still invalid.

[mrwonko]

VM_Call() is similarly hacky; it always copies 15 values, even if there are less. What's even worse, it calls vmMain with all 16 parameters as if it were a vararg function, when really it is a non-vararg function with 12 parameters. Eww.

It works because arguments are pushed right-to-left and removed by the caller in cdecl, but still.

[mrwonko]

Small correction: intptr_t is an integral type, so the issue is not calling it with an int (at least on 32 bit builds), but using pointers. Aside from the whole 16 instead of as many as required thing.

[ensiform]

It's handled correctly on the platforms all tech 3 engines support so I don't see the need to start being breaky in the future with old mod api.

[mrwonko]

What I'm suggesting isn't a change to the ABI, I'm just saying the current implementation of it relies on undefined behavior, but this can be fixed by making the syscall functions twice as long.

[mrwonko]

My suggested changes would look something like this, just for every syscall (it's just the first couple so far).

std::tuple is only used to simplify the definition; instead of

auto args = va.args< void*, int, int >();
Com_Memset( std::get< 0 >( args ), std::get< 1 >( args ), std::get< 2 >( args ) );

we could also use

void* mem = va_arg( ap, void* );
int val = va_arg( ap, int );
int size = va_arg( ap, int );
Com_Memset( mem, val, size );

but the compiler should create the same code either way and I like the former more. For reference it currently looks like this:

Com_Memset( VMA(1), args[2], args[3] );

Which obviously looks a lot nicer but relies on undefined behavior. But through creating a VMArg(n) macro to do the std::get< n >( args ) part that syntax can be approximated.

[ensiform]

You could also push the discussion upstream to ioquake3 too? Obviously the former would be invalid there since C only.

[mrwonko]

I don't care about ioquake3 :stuck_out_tongue:

[ensiform]

Well you should :P

[ahmedtd]

Just going to throw this out there -- I have a branch in which the whole vmMain thing is removed in favor of plugins calling directly into the engine. That way there's no varargs at all (especially good since varargs are slower than ordinary function calls on amd64). If you're going to start excising legacy code, you might as well go all the way.

[ahmedtd]

My example code is PR #722. It's not really a serious PR, since it totally destroys legacy compatibility.

[mrwonko]

But only for SP, which is fine in my book.

[Razish]

+1