Help getting amd64 assembly patch into wine? - wine-devel

4 Oct 2012


      As you might know from watching too many of my patches
scroll by, I've been working on adding support
for OpenMP to wine.  (A handful of games, and a lot
of serious apps, seem to use that api.)  After getting
it nicely organized and cleaned up to the point where it
passed all the tests I could throw at it, I submitted it as
http://source.winehq.org/patches/data/90524,
Alexandre rejected the patch, saying it had multiple problems,
but declining to provide any further hints.
He suggested I find someone to work with to get it in...
so here I am, hat in hand, asking for help.
The patch adds support for OpenMP programs like this:
// Compile with "cl test.c /openmp"
#include <omp.h>
#include <stdio.h>
int main (int argc, char **argv)
{
    #pragma omp parallel
    {
    printf("%d\n", omp_get_thread_num());
    }
}
Later patches add support for other OpenMP constructs.
My patch doesn't actually parallelize the app, but it does let it run
rather than crash on unimplemented _vcomp_fork.
Visual C will happily provide an assembly listing of this
program showing how _vcomp_fork is called, so understanding
that interface well enough to pass simple tests wasn't hard.
The main challenge was figuring out how to get the variable
list of arguments off the stack, and then put them back onto
the stack when calling the provided function pointer.
This bit of varargs hackery can't be done in pure C as far as I
can tell, so I used assembly.
I started programming in machine language long ago,
so getting that working wasn't too hard once I
realized that's what was needed.  Getting it to pass the
Alexandre test is another matter.
Alexandre did give feedback on one earlier iteration of
the patch, telling me I was re-inventing the wheel, so I
tossed my old assembly and copied the helper-calling code
nearly verbatim from call_method in oleaut32/typelib.c.
Maybe he objects to not passing copies of the arguments in
the floating point registers on amd64... but the arguments are
always pointers, so that shouldn't be needed.
I'm definitely not yet comfortable with the ASM_CFI stuff, so perhaps
there's a problem there, but I did at least verify that injecting
a fault into _test_vcomp_fork_worker5() still yielded a good stack
trace on both x86 and amd64 in winedbg.
There aren't many comments, and the ones I did add aren't the
greatest, but I doubt that's the issue.
Perhaps he objects to breaking up _vcomp_fork into three functions,
but I couldn't see any other way to write it without making later
patches like http://source.winehq.org/patches/data/90527
(and a future patch that adds actual multiple threads)
put more logic than seems wise into assembly.
So maybe it's none of the above.  If someone can suggest
what I might be missing, I'd appreciate it.  Even just "It will break if..."
or "Better compare how this was done in..." would be very helpful.
Thanks!