Sebastian Lackner sebastian@fds-team.de writes:
+void WINAPIV _vcomp_fork(BOOL ifval, int nargs, void *wrapper, ...) +{
- __ms_va_list valist;
- TRACE("(%d, %d, %p, ...)\n", ifval, nargs, wrapper);
- __ms_va_start(valist, wrapper);
- _vcomp_fork_call_wrapper(wrapper, nargs, valist);
- __ms_va_end(valist);
+}
Using a valist for this is questionable, even more so in the next patch where you copy it around, and doesn't buy anything over doing it in assembly from the start.
On 15.07.2015 08:08, Alexandre Julliard wrote:
Sebastian Lackner sebastian@fds-team.de writes:
+void WINAPIV _vcomp_fork(BOOL ifval, int nargs, void *wrapper, ...) +{
- __ms_va_list valist;
- TRACE("(%d, %d, %p, ...)\n", ifval, nargs, wrapper);
- __ms_va_start(valist, wrapper);
- _vcomp_fork_call_wrapper(wrapper, nargs, valist);
- __ms_va_end(valist);
+}
Using a valist for this is questionable, even more so in the next patch where you copy it around, and doesn't buy anything over doing it in assembly from the start.
Hello Alexandre,
thanks for the review.
I am not sure why you think it is questionable. We have compiler support for variadic functions, so why should we not use it? Please note that on x86_64 this does more than just grabbing a pointer to the variadic arguments, the declaration as WINAPIV also ensures that the compiler stores the register values ecx/edx/r8/r9 back onto the stack:
11bf: 89 4d 10 mov %ecx,0x10(%rbp) 11c2: 89 55 18 mov %edx,0x18(%rbp) 11c5: 4c 89 45 20 mov %r8,0x20(%rbp) 11c9: 4c 89 4d 28 mov %r9,0x28(%rbp)
Moreover, implementing the whole logic of _vcomp_fork in Assembler seems unreasonable. Do you mean a second Assembler wrapper, one to grab the "valist" pointer, and one to copy the arguments again on the target stack to execute the function? Would be nice if you could explain a bit more detailed what kind of solution you have in mind. We definitely need to copy the argument list, the target function does not take a pointer, but instead a copy of all arguments.
Best regards, Sebastian
Sebastian Lackner sebastian@fds-team.de writes:
I am not sure why you think it is questionable. We have compiler support for variadic functions, so why should we not use it? Please note that on x86_64 this does more than just grabbing a pointer to the variadic arguments, the declaration as WINAPIV also ensures that the compiler stores the register values ecx/edx/r8/r9 back onto the stack:
If you need them on the stack, you should be putting them there from assembly, not relying on the compiler implementation of va_list or assuming that va_list is a pointer to the first argument.
va_list is supposed to be an opaque type, so if you want to use it you'd have to access the arguments from C code too, which AFAICS is not possible in this case.
Moreover, implementing the whole logic of _vcomp_fork in Assembler seems unreasonable. Do you mean a second Assembler wrapper, one to grab the "valist" pointer, and one to copy the arguments again on the target stack to execute the function?
Probably something like that, or something along the lines of the existing relay code.
On 15.07.2015 16:35, Alexandre Julliard wrote:
Sebastian Lackner sebastian@fds-team.de writes:
I am not sure why you think it is questionable. We have compiler support for variadic functions, so why should we not use it? Please note that on x86_64 this does more than just grabbing a pointer to the variadic arguments, the declaration as WINAPIV also ensures that the compiler stores the register values ecx/edx/r8/r9 back onto the stack:
If you need them on the stack, you should be putting them there from assembly, not relying on the compiler implementation of va_list or assuming that va_list is a pointer to the first argument.
va_list is supposed to be an opaque type, so if you want to use it you'd have to access the arguments from C code too, which AFAICS is not possible in this case.
Wine contains several public functions which take __ms_va_list pointers as arguments. Its well-defined what such a __ms_va_list means, and it will never change because it would break the ABI. I also think that we can rely on the fact that compilers work properly, the values on the stack of course have to be initialized before the next function call, otherwise it would be a compiler bug.
Wine contains mixed C/Assembly code at a lot of places, and its nothing unusual that the Assembly code makes assumptions about how data structures are stored / passed. Why is it a problem here? Even when I add additional Assembler functions and wrapper, they will still contain ABI specific assumptions.
Moreover, implementing the whole logic of _vcomp_fork in Assembler seems unreasonable. Do you mean a second Assembler wrapper, one to grab the "valist" pointer, and one to copy the arguments again on the target stack to execute the function?
Probably something like that, or something along the lines of the existing relay code.
I will take a closer look, but I am not convinced that this will make the code look better or improve anything. I would still prefer if the patch would be accepted in the original shape because thats the best way to do it, imho.
Best regards, Sebastian
On 15.07.2015 17:14, Sebastian Lackner wrote:
On 15.07.2015 16:35, Alexandre Julliard wrote:
Sebastian Lackner sebastian@fds-team.de writes:
I am not sure why you think it is questionable. We have compiler support for variadic functions, so why should we not use it? Please note that on x86_64 this does more than just grabbing a pointer to the variadic arguments, the declaration as WINAPIV also ensures that the compiler stores the register values ecx/edx/r8/r9 back onto the stack:
If you need them on the stack, you should be putting them there from assembly, not relying on the compiler implementation of va_list or assuming that va_list is a pointer to the first argument.
va_list is supposed to be an opaque type, so if you want to use it you'd have to access the arguments from C code too, which AFAICS is not possible in this case.
Wine contains several public functions which take __ms_va_list pointers as arguments. Its well-defined what such a __ms_va_list means, and it will never change because it would break the ABI. I also think that we can rely on the fact that compilers work properly, the values on the stack of course have to be initialized before the next function call, otherwise it would be a compiler bug.
Wine contains mixed C/Assembly code at a lot of places, and its nothing unusual that the Assembly code makes assumptions about how data structures are stored / passed. Why is it a problem here? Even when I add additional Assembler functions and wrapper, they will still contain ABI specific assumptions.
Moreover, implementing the whole logic of _vcomp_fork in Assembler seems unreasonable. Do you mean a second Assembler wrapper, one to grab the "valist" pointer, and one to copy the arguments again on the target stack to execute the function?
Probably something like that, or something along the lines of the existing relay code.
I will take a closer look, but I am not convinced that this will make the code look better or improve anything. I would still prefer if the patch would be accepted in the original shape because thats the best way to do it, imho.
Best regards, Sebastian
I would like to resent the patch because I noticed that the fallback code is not declared as static. However, to avoid spamming the wine-patches mailing list, I'll wait for your final decision first.
Sebastian Lackner sebastian@fds-team.de writes:
I will take a closer look, but I am not convinced that this will make the code look better or improve anything. I would still prefer if the patch would be accepted in the original shape because thats the best way to do it, imho.
I still don't like abusing va_list this way, but it's your code, so if you think the alternative is worse I can live with it.
-
Alexandre Julliard -
Sebastian Lackner