[PATCH] wdscore: Implement CurrentIP.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51850 Signed-off-by: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> --- Based on this: https://bugs.winehq.org/show_bug.cgi?id=51850#c6 --- dlls/wdscore/Makefile.in | 3 +++ dlls/wdscore/main.c | 45 +++++++++++++++++++++++++++++++++++++++ dlls/wdscore/wdscore.spec | 2 +- 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 dlls/wdscore/main.c diff --git a/dlls/wdscore/Makefile.in b/dlls/wdscore/Makefile.in index 20ba1d3b1c9..2020e72c7bb 100644 --- a/dlls/wdscore/Makefile.in +++ b/dlls/wdscore/Makefile.in @@ -1,3 +1,6 @@ MODULE = wdscore.dll EXTRADLLFLAGS = -Wb,--prefer-native + +C_SRCS = \ + main.c diff --git a/dlls/wdscore/main.c b/dlls/wdscore/main.c new file mode 100644 index 00000000000..78309ac561d --- /dev/null +++ b/dlls/wdscore/main.c @@ -0,0 +1,45 @@ +/* + * Copyright 2021 Mohamad Al-Jaf + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include <stdarg.h> +#include <stdint.h> + +#include "windef.h" +#include "winbase.h" + +#include "wine/asm.h" +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(wdscore); + +uint32_t get_ip(); +__ASM_GLOBAL_FUNC( get_ip, + "mov 0(%esp), %eax\n\t" + "ret\n\t" ) + + +/*********************************************************************** + * CurrentIP (wdscore.@) + */ +int WINAPI CurrentIP() +{ + uint32_t ip; + + ip = get_ip(); + return ip; +} \ No newline at end of file diff --git a/dlls/wdscore/wdscore.spec b/dlls/wdscore/wdscore.spec index 15958b86aba..8b6febe6b3b 100644 --- a/dlls/wdscore/wdscore.spec +++ b/dlls/wdscore/wdscore.spec @@ -71,7 +71,7 @@ @ stub ConstructPartialMsgIfW @ stub ConstructPartialMsgVA @ stub ConstructPartialMsgVW -@ stub CurrentIP +@ stdcall CurrentIP() @ stub EndMajorTask @ stub EndMinorTask @ stub GetMajorTask -- 2.34.1
Just realized that I forgot to update the copyright year to 2022. Not sure if I should submit a revision for that or not, but in the meantime I'll wait for feedback for the code itself. On Wed, Jan 12, 2022 at 6:04 AM Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> wrote:
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51850 Signed-off-by: Mohamad Al-Jaf <mohamadaljaf(a)gmail.com> --- Based on this: https://bugs.winehq.org/show_bug.cgi?id=51850#c6 --- dlls/wdscore/Makefile.in | 3 +++ dlls/wdscore/main.c | 45 +++++++++++++++++++++++++++++++++++++++ dlls/wdscore/wdscore.spec | 2 +- 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 dlls/wdscore/main.c
diff --git a/dlls/wdscore/Makefile.in b/dlls/wdscore/Makefile.in index 20ba1d3b1c9..2020e72c7bb 100644 --- a/dlls/wdscore/Makefile.in +++ b/dlls/wdscore/Makefile.in @@ -1,3 +1,6 @@ MODULE = wdscore.dll
EXTRADLLFLAGS = -Wb,--prefer-native + +C_SRCS = \ + main.c diff --git a/dlls/wdscore/main.c b/dlls/wdscore/main.c new file mode 100644 index 00000000000..78309ac561d --- /dev/null +++ b/dlls/wdscore/main.c @@ -0,0 +1,45 @@ +/* + * Copyright 2021 Mohamad Al-Jaf + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#include <stdarg.h> +#include <stdint.h> + +#include "windef.h" +#include "winbase.h" + +#include "wine/asm.h" +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(wdscore); + +uint32_t get_ip(); +__ASM_GLOBAL_FUNC( get_ip, + "mov 0(%esp), %eax\n\t" + "ret\n\t" ) + + +/*********************************************************************** + * CurrentIP (wdscore.@) + */ +int WINAPI CurrentIP() +{ + uint32_t ip; + + ip = get_ip(); + return ip; +} \ No newline at end of file diff --git a/dlls/wdscore/wdscore.spec b/dlls/wdscore/wdscore.spec index 15958b86aba..8b6febe6b3b 100644 --- a/dlls/wdscore/wdscore.spec +++ b/dlls/wdscore/wdscore.spec @@ -71,7 +71,7 @@ @ stub ConstructPartialMsgIfW @ stub ConstructPartialMsgVA @ stub ConstructPartialMsgVW -@ stub CurrentIP +@ stdcall CurrentIP() @ stub EndMajorTask @ stub EndMinorTask @ stub GetMajorTask -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=105179 Your paranoid android. === debian11 (build log) === Task: Patch failed to apply === debian11 (build log) === Task: Patch failed to apply
Hello Mohammed, thanks for the patch! I have a few comments inlined below. On 1/12/22 05:03, Mohamad Al-Jaf wrote:
+#include <stdarg.h> +#include <stdint.h> + +#include "windef.h" +#include "winbase.h" + +#include "wine/asm.h" +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(wdscore);
This debug channel is unused, and should generate a warning.
+ +uint32_t get_ip(); +__ASM_GLOBAL_FUNC( get_ip, + "mov 0(%esp), %eax\n\t" + "ret\n\t" )
No reason to make this a separate function; you can use __ASM_STDCALL_FUNC() to declare it.
+ + +/*********************************************************************** + * CurrentIP (wdscore.@) + */ +int WINAPI CurrentIP() +{ + uint32_t ip; + + ip = get_ip(); + return ip; +} \ No newline at end of file
Please try to avoid whitespace errors.
On 1/12/22 10:00, Zebediah Figura (she/her) wrote:
Hello Mohammed, thanks for the patch! I have a few comments inlined below.
On 1/12/22 05:03, Mohamad Al-Jaf wrote:
+#include <stdarg.h> +#include <stdint.h> + +#include "windef.h" +#include "winbase.h" + +#include "wine/asm.h" +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(wdscore);
This debug channel is unused, and should generate a warning.
+ +uint32_t get_ip(); +__ASM_GLOBAL_FUNC( get_ip, + "mov 0(%esp), %eax\n\t" + "ret\n\t" )
No reason to make this a separate function; you can use __ASM_STDCALL_FUNC() to declare it.
Giovanni points out that this also won't compile on anything other than i386. You'll need to use #ifdef guards to handle separate architectures.
+ + +/*********************************************************************** + * CurrentIP (wdscore.@) + */ +int WINAPI CurrentIP() +{ + uint32_t ip; + + ip = get_ip(); + return ip; +} \ No newline at end of file
Please try to avoid whitespace errors.
Hi Zebediah, Thanks for the feedback. Not sure how to quote in Gmail but I'll add > for each line I reply to.
This debug channel is unused, and should generate a warning.
I can remove it but I plan on adding stub functions to the dll. CurrentIP is the first function the Microsoft Media Creation tool calls upon, ConstructPartialMsgVW after. Not sure how many others it will try to call. Based on your method, I think ConstructPartialMsgVW takes 3 arguments, but I checked it a few weeks ago so I don't remember if this is the exact number. In any case, the debug channel would be used for those arguments. While we're on this subject, is there a method to determine the function return type, argument types and possibly names? I see DWORD and sometimes void is used for undocumented functions, would this be appropriate?
No reason to make this a separate function; you can use __ASM_STDCALL_FUNC() to declare it.
Thanks, is this correct so far? __ASM_STDCALL_FUNC(CurrentIP, 0, "mov 0(%esp), %eax\n\t" "ret\n\t" )
Please try to avoid whitespace errors.
I see a lot of files have a newline at the end, I'm assuming a newline avoids whitespace errors?
Giovanni points out that this also won't compile on anything other than i386. You'll need to use #ifdef guards to handle separate architectures.
I see, I'm not really sure what to do for the other architectures or which ones are appropriate in this context. I'm not familiar with arm assembly. I looked at the assembly code in kernelbase/thread.c for reference and this is what I have so far: #ifdef __i386__ __ASM_STDCALL_FUNC(CurrentIP, 0, "movl 0(%esp), %eax\n\t" "ret" ) #elif defined(__x86_64__) __ASM_STDCALL_FUNC(CurrentIP, 0, "movq 0(%rsp), %rax\n\t" "ret" ) #elif defined(__arm__) __ASM_STDCALL_FUNC(CurrentIP, 0, "mov r13, r0\n\t" "ret" ) #elif defined(__aarch64__) __ASM_STDCALL_FUNC(CurrentIP, 0, "mov sp, x0\n\t" "ret" ) #else int WINAPI CurrentIP() { FIXME( "not implemented\n" ); return 0; } #endif Is this a good start? What parts are incorrect? On Wed, Jan 12, 2022 at 11:14 AM Zebediah Figura (she/her) < zfigura(a)codeweavers.com> wrote:
On 1/12/22 10:00, Zebediah Figura (she/her) wrote:
Hello Mohammed, thanks for the patch! I have a few comments inlined below.
On 1/12/22 05:03, Mohamad Al-Jaf wrote:
+#include <stdarg.h> +#include <stdint.h> + +#include "windef.h" +#include "winbase.h" + +#include "wine/asm.h" +#include "wine/debug.h" + +WINE_DEFAULT_DEBUG_CHANNEL(wdscore);
This debug channel is unused, and should generate a warning.
+ +uint32_t get_ip(); +__ASM_GLOBAL_FUNC( get_ip, + "mov 0(%esp), %eax\n\t" + "ret\n\t" )
No reason to make this a separate function; you can use __ASM_STDCALL_FUNC() to declare it.
Giovanni points out that this also won't compile on anything other than i386. You'll need to use #ifdef guards to handle separate architectures.
+ +
+/***********************************************************************
+ * CurrentIP (wdscore.@) + */ +int WINAPI CurrentIP() +{ + uint32_t ip; + + ip = get_ip(); + return ip; +} \ No newline at end of file
Please try to avoid whitespace errors.
On 1/12/22 20:25, Mohamad Al-Jaf wrote:
Hi Zebediah,
Thanks for the feedback.
Not sure how to quote in Gmail but I'll add > for each line I reply to.
This debug channel is unused, and should generate a warning.
I can remove it but I plan on adding stub functions to the dll. CurrentIP is the first function the Microsoft Media Creation tool calls upon, ConstructPartialMsgVW after. Not sure how many others it will try to call. Based on your method, I think ConstructPartialMsgVW takes 3 arguments, but I checked it a few weeks ago so I don't remember if this is the exact number.
Adding the debug channel declaration should be deferred until the first patch that uses it. I am sort of led to wonder, though, is there really a point in trying to use this program with builtin wdscore? It sounds like the DLL is rather private to the program in question. As long as you're copying the Media Creation tool from a windows 10 installation (since I don't see any other way to acquire it?) I'd imagine that one should copy native wdscore.dll as well.
In any case, the debug channel would be used for those arguments. While we're on this subject, is there a method to determine the function return type, argument types and possibly names? I see DWORD and sometimes void is used for undocumented functions, would this be appropriate?
No, you pretty much have to guess just from figuring out what the arguments look like. If you can't tell I'd recommend using "void *" or "DWORD_PTR" so that the entire value is captured.
No reason to make this a separate function; you can use __ASM_STDCALL_FUNC() to declare it.
Thanks, is this correct so far?
__ASM_STDCALL_FUNC(CurrentIP, 0, "mov 0(%esp), %eax\n\t" "ret\n\t" )
Yes, that looks correct. Note that you don't need the \n\t after the last instruction.
Please try to avoid whitespace errors.
I see a lot of files have a newline at the end, I'm assuming a newline avoids whitespace errors?
A missing newline at the end of a file is considered a whitespace error; I believe Git should warn you about those.
Giovanni points out that this also won't compile on anything other than i386. You'll need to use #ifdef guards to handle separate architectures.
I see, I'm not really sure what to do for the other architectures or which ones are appropriate in this context. I'm not familiar with arm assembly. I looked at the assembly code in kernelbase/thread.c for reference and this is what I have so far:
#ifdef __i386__ __ASM_STDCALL_FUNC(CurrentIP, 0, "movl 0(%esp), %eax\n\t" "ret" ) #elif defined(__x86_64__) __ASM_STDCALL_FUNC(CurrentIP, 0, "movq 0(%rsp), %rax\n\t" "ret" ) #elif defined(__arm__) __ASM_STDCALL_FUNC(CurrentIP, 0, "mov r13, r0\n\t" "ret" ) #elif defined(__aarch64__) __ASM_STDCALL_FUNC(CurrentIP, 0, "mov sp, x0\n\t" "ret" ) #else int WINAPI CurrentIP() { FIXME( "not implemented\n" ); return 0; } #endif
Is this a good start? What parts are incorrect?
I'm not an expert in ARM assembly, but I believe you want: mov lr, r0 bx lr for ARM, and mov lr, x0 ret for ARM64. The rest looks correct to me.
On 1/14/22 02:23, Zebediah Figura (she/her) wrote:
On 1/12/22 20:25, Mohamad Al-Jaf wrote:
Hi Zebediah,
Thanks for the feedback.
Not sure how to quote in Gmail but I'll add > for each line I reply to.
This debug channel is unused, and should generate a warning.
I can remove it but I plan on adding stub functions to the dll. CurrentIP is the first function the Microsoft Media Creation tool calls upon, ConstructPartialMsgVW after. Not sure how many others it will try to call. Based on your method, I think ConstructPartialMsgVW takes 3 arguments, but I checked it a few weeks ago so I don't remember if this is the exact number.
Adding the debug channel declaration should be deferred until the first patch that uses it.
I am sort of led to wonder, though, is there really a point in trying to use this program with builtin wdscore? It sounds like the DLL is rather private to the program in question. As long as you're copying the Media Creation tool from a windows 10 installation (since I don't see any other way to acquire it?) I'd imagine that one should copy native wdscore.dll as well.
In any case, the debug channel would be used for those arguments. While we're on this subject, is there a method to determine the function return type, argument types and possibly names? I see DWORD and sometimes void is used for undocumented functions, would this be appropriate?
No, you pretty much have to guess just from figuring out what the arguments look like. If you can't tell I'd recommend using "void *" or "DWORD_PTR" so that the entire value is captured.
No reason to make this a separate function; you can use __ASM_STDCALL_FUNC() to declare it.
Thanks, is this correct so far?
__ASM_STDCALL_FUNC(CurrentIP, 0, "mov 0(%esp), %eax\n\t" "ret\n\t" )
Yes, that looks correct. Note that you don't need the \n\t after the last instruction.
Please try to avoid whitespace errors.
I see a lot of files have a newline at the end, I'm assuming a newline avoids whitespace errors?
A missing newline at the end of a file is considered a whitespace error; I believe Git should warn you about those.
Giovanni points out that this also won't compile on anything other than i386. You'll need to use #ifdef guards to handle separate architectures.
I see, I'm not really sure what to do for the other architectures or which ones are appropriate in this context. I'm not familiar with arm assembly. I looked at the assembly code in kernelbase/thread.c for reference and this is what I have so far:
#ifdef __i386__ __ASM_STDCALL_FUNC(CurrentIP, 0, "movl 0(%esp), %eax\n\t" "ret" ) #elif defined(__x86_64__) __ASM_STDCALL_FUNC(CurrentIP, 0, "movq 0(%rsp), %rax\n\t" "ret" ) #elif defined(__arm__) __ASM_STDCALL_FUNC(CurrentIP, 0, "mov r13, r0\n\t" "ret" ) #elif defined(__aarch64__) __ASM_STDCALL_FUNC(CurrentIP, 0, "mov sp, x0\n\t" "ret" ) #else int WINAPI CurrentIP() { FIXME( "not implemented\n" ); return 0; } #endif
Is this a good start? What parts are incorrect?
I'm not an expert in ARM assembly, but I believe you want:
mov lr, r0 bx lr
for ARM, and
mov lr, x0 ret
for ARM64.
The rest looks correct to me.
Is there any indication this function should be doing something like this at all?
Adding the debug channel declaration should be deferred until the first patch that uses it.
I was going to remove it but since the #ifdef guards ends with an unknown architecture, it would require the debug channel to display the fixme message, no?
I am sort of led to wonder, though, is there really a point in trying to use this program with builtin wdscore? It sounds like the DLL is rather private to the program in question. As long as you're copying the Media Creation tool from a windows 10 installation (since I don't see any other way to acquire it?) I'd imagine that one should copy native wdscore.dll as well.
You can download the tool directly from Microsoft.com and also the archive link in the wine bug report. But you're right, there's no real point to using the builtin wdscore. Personally, I'd use the native dll for something like this. I figured since it was already in the wine tree and the bug report was accepted, it would be a good learning exercise.
No, you pretty much have to guess just from figuring out what the arguments look like. If you can't tell I'd recommend using "void *" or "DWORD_PTR" so that the entire value is captured.
I see, thank you.
Yes, that looks correct. Note that you don't need the \n\t after the last instruction.
Thanks, I figured as much and removed the \n\t afterwards.
A missing newline at the end of a file is considered a whitespace error; I believe Git should warn you about those.
That's odd, I don't think it warned me. Perhaps I need to configure it for that?
I'm not an expert in ARM assembly, but I believe you want:
mov lr, r0 bx lr
for ARM, and
mov lr, x0 ret
for ARM64.
The rest looks correct to me.
Thank you so much, I really appreciate all the help you've given me. You're a great teacher!
Is there any indication this function should be doing something like this at all?
I'm testing it out in Windows to get a sense of what it does. Here's my code in C++, let me know if it's incorrect: typedef int (*CurrentIP)(); HMODULE hDLL = LoadLibraryA("wdscore.dll"); CurrentIP ip = (CurrentIP)GetProcAddress(hDLL, "CurrentIP"); std::cout << "CurrentIP() = " << ip() << "\n"; FreeLibrary(hDLL); It returns random numbers, e.g. 10424371, 1249331, 6033459. So doesn't this mean it's returning the instruction pointer? If so, I don't know how to write a conformance test for it. Would it be necessary in this case? On Thu, Jan 13, 2022 at 11:20 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 1/14/22 02:23, Zebediah Figura (she/her) wrote:
On 1/12/22 20:25, Mohamad Al-Jaf wrote:
Hi Zebediah,
Thanks for the feedback.
Not sure how to quote in Gmail but I'll add > for each line I reply to.
This debug channel is unused, and should generate a warning.
I can remove it but I plan on adding stub functions to the dll. CurrentIP is the first function the Microsoft Media Creation tool calls upon, ConstructPartialMsgVW after. Not sure how many others it will try to call. Based on your method, I think ConstructPartialMsgVW takes 3 arguments, but I checked it a few weeks ago so I don't remember if this is the exact number.
Adding the debug channel declaration should be deferred until the first patch that uses it.
I am sort of led to wonder, though, is there really a point in trying to use this program with builtin wdscore? It sounds like the DLL is rather private to the program in question. As long as you're copying the Media Creation tool from a windows 10 installation (since I don't see any other way to acquire it?) I'd imagine that one should copy native wdscore.dll as well.
In any case, the debug channel would be used for those arguments. While we're on this subject, is there a method to determine the function return type, argument types and possibly names? I see DWORD and sometimes void is used for undocumented functions, would this be appropriate?
No, you pretty much have to guess just from figuring out what the arguments look like. If you can't tell I'd recommend using "void *" or "DWORD_PTR" so that the entire value is captured.
No reason to make this a separate function; you can use __ASM_STDCALL_FUNC() to declare it.
Thanks, is this correct so far?
__ASM_STDCALL_FUNC(CurrentIP, 0, "mov 0(%esp), %eax\n\t" "ret\n\t" )
Yes, that looks correct. Note that you don't need the \n\t after the last instruction.
Please try to avoid whitespace errors.
I see a lot of files have a newline at the end, I'm assuming a newline avoids whitespace errors?
A missing newline at the end of a file is considered a whitespace error; I believe Git should warn you about those.
Giovanni points out that this also won't compile on anything other than i386. You'll need to use #ifdef guards to handle separate architectures.
I see, I'm not really sure what to do for the other architectures or which ones are appropriate in this context. I'm not familiar with arm assembly. I looked at the assembly code in kernelbase/thread.c for reference and this is what I have so far:
#ifdef __i386__ __ASM_STDCALL_FUNC(CurrentIP, 0, "movl 0(%esp), %eax\n\t" "ret" ) #elif defined(__x86_64__) __ASM_STDCALL_FUNC(CurrentIP, 0, "movq 0(%rsp), %rax\n\t" "ret" ) #elif defined(__arm__) __ASM_STDCALL_FUNC(CurrentIP, 0, "mov r13, r0\n\t" "ret" ) #elif defined(__aarch64__) __ASM_STDCALL_FUNC(CurrentIP, 0, "mov sp, x0\n\t" "ret" ) #else int WINAPI CurrentIP() { FIXME( "not implemented\n" ); return 0; } #endif
Is this a good start? What parts are incorrect?
I'm not an expert in ARM assembly, but I believe you want:
mov lr, r0 bx lr
for ARM, and
mov lr, x0 ret
for ARM64.
The rest looks correct to me.
Is there any indication this function should be doing something like this at all?
* On 2022-01-14 07:31, Mohamad Al-Jaf wrote:
Is there any indication this function should be doing something like > this at all?
I'm testing it out in Windows to get a sense of what it does. Here's my code in C++, let me know if it's incorrect:
typedef int (*CurrentIP)(); HMODULE hDLL = LoadLibraryA("wdscore.dll"); CurrentIP ip = (CurrentIP)GetProcAddress(hDLL, "CurrentIP"); std::cout << "CurrentIP() = " << ip() << "\n"; FreeLibrary(hDLL);
It returns random numbers, e.g. 10424371, 1249331, 6033459. So doesn't this mean it's returning the instruction pointer?
If so, I don't know how to write a conformance test for it. Would it be necessary in this case?
I think it would be useful add saving all the CPU registers (before or maybe after calling the function) in the test and dumping them together with the CurrentIP() return value using just trace(). So different test runs would provide more data to decide if there is some correlation between what CurrentIP() returns and any of the regs. My 2c€, S.
On 1/14/22 14:31, Mohamad Al-Jaf wrote:
I'm testing it out in Windows to get a sense of what it does. Here's my code in C++, let me know if it's incorrect:
typedef int (*CurrentIP)(); HMODULE hDLL = LoadLibraryA("wdscore.dll"); CurrentIP ip = (CurrentIP)GetProcAddress(hDLL, "CurrentIP"); std::cout << "CurrentIP() = " << ip() << "\n"; FreeLibrary(hDLL);
It returns random numbers, e.g. 10424371, 1249331, 6033459. So doesn't this mean it's returning the instruction pointer?
A more likely scenario is that the code address is changing from one invocation to another. Try compiling with "/LINK /DYNAMICBASE:NO" (in MSVC) or "-no-pie" (in GCC/MinGW). See also: https://en.wikipedia.org/wiki/Address_space_layout_randomization, and https://en.wikipedia.org/wiki/Position-independent_executable. -- Sincerely, Jinoh Kang
On 1/14/22 14:31, Mohamad Al-Jaf wrote:
Adding the debug channel declaration should be deferred until the first patch that uses it.
I was going to remove it but since the #ifdef guards ends with an unknown architecture, it would require the debug channel to display the fixme message, no?
I am sort of led to wonder, though, is there really a point in trying to use this program with builtin wdscore? It sounds like the DLL is rather private to the program in question. As long as you're copying the Media Creation tool from a windows 10 installation (since I don't see any other way to acquire it?) I'd imagine that one should copy native wdscore.dll as well.
You can download the tool directly from Microsoft.com and also the archive link in the wine bug report. But you're right, there's no real point to using the builtin wdscore. Personally, I'd use the native dll for something like this. I figured since it was already in the wine tree and the bug report was accepted, it would be a good learning exercise.
If the Media Creation Tool won't still run even after this patch (with builtin wdscore.dll), then I suppose the patch has to wait until the RC code freeze ends. -- Sincerely, Jinoh Kang
On 1/13/22 23:31, Mohamad Al-Jaf wrote:
Adding the debug channel declaration should be deferred until the first patch that uses it.
I was going to remove it but since the #ifdef guards ends with an unknown architecture, it would require the debug channel to display the fixme message, no?
Yes, in that case it would belong in this patch.
Is there any indication this function should be doing something like this at all?
I'm testing it out in Windows to get a sense of what it does. Here's my code in C++, let me know if it's incorrect:
typedef int (*CurrentIP)(); HMODULE hDLL = LoadLibraryA("wdscore.dll"); CurrentIP ip = (CurrentIP)GetProcAddress(hDLL, "CurrentIP"); std::cout << "CurrentIP() = " << ip() << "\n"; FreeLibrary(hDLL);
It returns random numbers, e.g. 10424371, 1249331, 6033459. So doesn't this mean it's returning the instruction pointer?
If so, I don't know how to write a conformance test for it. Would it be necessary in this case?
You could probably write a conformance test by manually emitting assembly, e.g. on i386 something like call CurrentIP 1: cmp %eax, $1b sete %al ret That's a bit of work, though, and it's not clear to me that it's worthwhile.
Hi Giovanni,
Given your implementation, it seems more likely that CurrentIP returns void *, instead of int (which would get the pointer truncated, e.g., on i386). This is also hinted by this snippet that was mentioned in the bug:
https://github.com/seven-mile/CallCbsCore/blob/6465d9c6801768c56c7ca1faebc65... Yes, you're right. I saw that snippet but thought that they were casting it as void * simply because they didn't know the data type. In retrospect, int isn't appropriate, especially since it's signed and 32-bits.
Same here. Notice that CurrentIP is not declared here, so the compiler defaults to the terrible fallback of believing it returns int, though I don't think this is appropriate here. And you should declare it anyway. I don't know if there is a header in which it is sensible to put the declaration.
The compiler gave a warning about that, but I don't know where to declare it or if there's even an existing header in which it is acceptable to put it in. And creating a new header file seems like bloat, though I have no problem adding it if the Wine team or Alexandre okays it.
But just tracing the result is not very useful for testing. Given that your hypothesis seems to be that CurrentIP returns the return address of the call, you could try to check that such address is in the caller function. There is no portable way to do that, but a reasonable attempt is to check that ret is higher than &test_CurrentIP and lower than, say, &test_CurrentIP + 0x100. I did a quick test and this seems indeed to be the case:
Thank you, that's a great way to check for it. Here's what I have in the test, are these error messages good? static void test_CurrentIP(void) { void *cur; void *ret; cur = &test_CurrentIP; ret = CurrentIP(); ok(ret != 0, "Unsupported architecture\n"); ok(ret > cur, "&test_CurrentIP=%p CurrentIP()=%p\n", cur, ret); ok(ret < (cur + 0x100), "&test_CurrentIP + 0x100=%p CurrentIP()=%p\n", cur + 0x100, ret); }
I think it would be useful add saving all the CPU registers (before or maybe after calling the function) in the test and dumping them together with the CurrentIP() return value using just trace().
So different test runs would provide more data to decide if there is some correlation between what CurrentIP() returns and any of the regs.
I'm not sure if this is considered acceptable in Wine, but thanks for the suggestion.
A more likely scenario is that the code address is changing from one invocation to another.
Try compiling with "/LINK /DYNAMICBASE:NO" (in MSVC) or "-no-pie" (in GCC/MinGW).
See also: https://en.wikipedia.org/wiki/Address_space_layout_randomization, and https://en.wikipedia.org/wiki/Position-independent_executable.
I see, compiling it with /LINK /DYNAMICBASE:NO results in the same value. Thanks for the links, good info.
+ "ret" ) +#elif defined(__arm__) +__ASM_STDCALL_FUNC(CurrentIP, 0, + "mov lr, r0\n\t"
Operands are swapped. This is not AT&T syntax. Should be "mov r0, lr".
I'm confused, doesn't mov lr, r0 mean to move the value from lr into r0? Isn't that AT&T syntax? https://csiflabs.cs.ucdavis.edu/~ssdavis/50/att-syntax.htm Or did you mean to say that arm does not follow AT&T syntax? If so, you're right, the operands are swapped. https://www.keil.com/support/man/docs/armasm/armasm_dom1361289878994.htm
Let me suggest an alternative: instead of assembly, we can just use GCC's built-in functions here for all architectures.
#ifdef __has_builtin #if __has_builtin(__builtin_extract_return_addr) #define extract_retaddr(x) __builtin_extract_return_addr(x) #endif #elif defined(__GNUC__) && __GNUC__ >= 5 #define extract_retaddr(x) __builtin_extract_return_addr(x) #else #define extract_retaddr(x) (x) #endif
void *WINAPI CurrentIP(void) { return extract_retaddr( __builtin_return_address( 0 )); }
Also this means the debug channel is (again) no longer needed.
Thank you, this looks great. But according to the GNU documentation __builtin_return_address is processor specific and might return 0 or worse, a random value: https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Return-Address.html I think the debug channel is still needed for unsupported architectures. In this case, a return value of 0 is easy to check for, but a random value would result in a false return address. I can't think of a trivial way to write a check for the random value. I suppose I could do something like what Giovanni did with the test but it seems redundant and I'm not sure if it's even considered acceptable code.
If the Media Creation Tool won't still run even after this patch (with builtin wdscore.dll), then I suppose the patch has to wait until the RC code freeze ends.
I have no problem deferring the patch until after the code freeze, but aren't Wine bugs supposed to be specific to one problem? To me, this makes sense intuitively and is in line with how Wine patches are typically small. I just checked the wiki and it also says "Each bug report should cover one problem.".
You could probably write a conformance test by manually emitting assembly, e.g. on i386 something like
call CurrentIP 1: cmp %eax, $1b sete %al ret
That's a bit of work, though, and it's not clear to me that it's worthwhile.
Thank you. Agreed, it doesn't seem worthwhile. I think Giovanni's test case is sufficient. On Fri, Jan 14, 2022 at 11:46 AM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
On 1/13/22 23:31, Mohamad Al-Jaf wrote:
Adding the debug channel declaration should be deferred until the first patch that uses it.
I was going to remove it but since the #ifdef guards ends with an unknown architecture, it would require the debug channel to display the fixme message, no?
Yes, in that case it would belong in this patch.
Is there any indication this function should be doing something like this at all?
I'm testing it out in Windows to get a sense of what it does. Here's my code in C++, let me know if it's incorrect:
typedef int (*CurrentIP)(); HMODULE hDLL = LoadLibraryA("wdscore.dll"); CurrentIP ip = (CurrentIP)GetProcAddress(hDLL, "CurrentIP"); std::cout << "CurrentIP() = " << ip() << "\n"; FreeLibrary(hDLL);
It returns random numbers, e.g. 10424371, 1249331, 6033459. So doesn't this mean it's returning the instruction pointer?
If so, I don't know how to write a conformance test for it. Would it be necessary in this case?
You could probably write a conformance test by manually emitting assembly, e.g. on i386 something like
call CurrentIP 1: cmp %eax, $1b sete %al ret
That's a bit of work, though, and it's not clear to me that it's worthwhile.
Hi, Il 17/01/22 03:55, Mohamad Al-Jaf ha scritto:
Thank you, that's a great way to check for it. Here's what I have in the test, are these error messages good?
static void test_CurrentIP(void) { void *cur; void *ret;
cur = &test_CurrentIP; ret = CurrentIP();
ok(ret != 0, "Unsupported architecture\n");
This is redundant, given the following two.
ok(ret > cur, "&test_CurrentIP=%p CurrentIP()=%p\n", cur, ret); ok(ret < (cur + 0x100), "&test_CurrentIP + 0x100=%p CurrentIP()=%p\n", cur + 0x100, ret); }
Usually messages are a bit more descriptive. I would go for something like: ok(cur <= ret && ret < (cur + 0x100), "Address %p not in function starting at %p.\n", ret, cur); Though notice that in C it is illegal to add a number to a void*, so this line will raise some warnings. Convert pointers to char* or to uintptr_t to avoid the warnings.
Let me suggest an alternative: instead of assembly, we can just use GCC's built-in functions here for all architectures.
#ifdef __has_builtin #if __has_builtin(__builtin_extract_return_addr) #define extract_retaddr(x) __builtin_extract_return_addr(x) #endif #elif defined(__GNUC__) && __GNUC__ >= 5 #define extract_retaddr(x) __builtin_extract_return_addr(x) #else #define extract_retaddr(x) (x) #endif
void *WINAPI CurrentIP(void) { return extract_retaddr( __builtin_return_address( 0 )); }
Also this means the debug channel is (again) no longer needed.
Thank you, this looks great. But according to the GNU documentation __builtin_return_address is processor specific and might return 0 or worse, a random value: https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Return-Address.html <https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Return-Address.html>
My interpretation of that page is that calling __builtin_return_address(0) is always fine, it is for higher frames that it is not necessarily reliable. Still, it is a GCC-specific thing, so alternatives should be provided for other compilers. Giovanni.
Hi,
This is redundant, given the following two.
Yeah, that should have been obvious, not sure how I didn't see it.
Though notice that in C it is illegal to add a number to a void*, so this line will raise some warnings. Convert pointers to char* or to uintptr_t to avoid the warnings.
Sorry about that. This is what I have now: ok(cur <= ret && (char*)ret < ((char*)ret + 0x100), "Address %p not in function starting at %p.\n", ret, cur); Is it okay to cast it like this?
My interpretation of that page is that calling __builtin_return_address(0) is always fine, it is for higher frames that it is not necessarily reliable. Still, it is a GCC-specific thing, so alternatives should be provided for other compilers.
I see, so would it be best to just use the assembly code? Regarding the unit test, is it okay to declare the function directly in the test itself? Doing this, the compiler doesn't issue a warning, but it seems suboptimal to me. A header seems like the correct way to go. But I don't know if it's acceptable to write a header file for just 1 function. If you or the Wine team thinks a header file is required, should I submit 2 patches instead of one? First for the function implementation and the second for the test, including the header file.
Hi, Il 18/01/22 01:48, Mohamad Al-Jaf ha scritto:
ok(cur <= ret && (char*)ret < ((char*)ret + 0x100), "Address %p not in function starting at %p.\n", ret, cur);
Is it okay to cast it like this?
Does the compiler emit any warning? Remember that if it makes your life easier, you can directly define cur and ret of type char*.
My interpretation of that page is that calling __builtin_return_address(0) is always fine, it is for higher frames that it is not necessarily reliable. Still, it is a GCC-specific thing, so alternatives should be provided for other compilers.
I see, so would it be best to just use the assembly code?
It seems plausible.
Regarding the unit test, is it okay to declare the function directly in the test itself? Doing this, the compiler doesn't issue a warning, but it seems suboptimal to me. A header seems like the correct way to go. But I don't know if it's acceptable to write a header file for just 1 function.
If you or the Wine team thinks a header file is required, should I submit 2 patches instead of one? First for the function implementation and the second for the test, including the header file.
I am not able to answer about this. Giovanni.
On 18/01/2022 18:00, Giovanni Mascellani wrote:
Hi,
Il 18/01/22 01:48, Mohamad Al-Jaf ha scritto:
ok(cur <= ret && (char*)ret < ((char*)ret + 0x100), "Address %p not in function starting at %p.\n", ret, cur);
Is it okay to cast it like this?
Does the compiler emit any warning?
Remember that if it makes your life easier, you can directly define cur and ret of type char*.
> My interpretation of that page is that calling > __builtin_return_address(0) is always fine, it is for higher frames that > it is not necessarily reliable. Still, it is a GCC-specific thing, so > alternatives should be provided for other compilers.
I see, so would it be best to just use the assembly code?
It seems plausible.
Regarding the unit test, is it okay to declare the function directly in the test itself? Doing this, the compiler doesn't issue a warning, but it seems suboptimal to me. A header seems like the correct way to go. But I don't know if it's acceptable to write a header file for just 1 function.
If you or the Wine team thinks a header file is required, should I submit 2 patches instead of one? First for the function implementation and the second for the test, including the header file.
I am not able to answer about this.
Giovanni.
Well, I think you can just probably just declare it in the source, either in the test function or outside of it. There's some examples of that in existing tests, see e.g: gdiplus/tests/brush.c -> extern BOOL color_match(...); imagehlp/tests/testdll.c -> extern DWORD WINAPI StrCmpCA(...); windowscodecs/tests/converter.c -> extern HRESULT STDMETHODCALLTYPE IWICBitmapFrameEncode_WriteSource_Proxy(...); It wouldn't even be more "correct" to declare it in a private header to the test since it's technically an imported function from the Win32 API, not part of the test's functions, so I would say it's pointless, but that's just my opinion, not authoritative. Best is to just send it and see what response it gets about it, if any.
Hi,
Does the compiler emit any warning?
No, no warnings.
Remember that if it makes your life easier, you can directly define cur and ret of type char*.
Thanks, it does make it easier and more readable. Doing this causes no warnings: char *cur; char *ret; cur = (char*)&test_CurrentIP; ret = (char*)CurrentIP();
Well, I think you can just probably just declare it in the source, either in the test function or outside of it. There's some examples of that in existing tests, see e.g:
I see, thanks for the examples. I took a look at them and put this in tests/main.c: extern void *WINAPI CurrentIP(void); Doing this, the 64-bit test compiles without a problem, but the 32-bit runs into this problem: /usr/lib/gcc/i686-w64-mingw32/11.2.0/../../../../i686-w64-mingw32/bin/ld: dlls/wdscore/tests/main.cross.o: in function `test_CurrentIP': src/wine-staging-32-build/../wine-staging/dlls/wdscore/tests/main.c:33: undefined reference to `CurrentIP(a)0' /usr/lib/gcc/i686-w64-mingw32/11.2.0/../../../../i686-w64-mingw32/bin/ld: /src/wine-staging-32-build/../wine-staging/dlls/wdscore/tests/main.c:33: undefined reference to `CurrentIP(a)0' collect2: error: ld returned 1 exit status winegcc: /usr/bin/i686-w64-mingw32-gcc failed make[1]: *** [Makefile:228478: dlls/wdscore/tests/wdscore_test.exe] Error 2 Removing WINAPI from the declaration allows it to compile: extern void *CurrentIP(void); I don't understand why this is happening. What am I missing? The same error happens if I add a header file and import it. The dll wlanapi also has a void* function (void *WINAPI WlanAllocateMemory) and I don't see anything that might be missing in wdscore. Is there a missing header? I tried adding windows.h and other headers from similar dlls but it still says undefined reference to CurrentIP. Why would it work on 64-bit but not 32-bit? I had a similar problem in the past with UiaRaiseAutomationPropertyChangedEvent but that was due to an incorrect spec file parameter. As far as I know, the spec parameter should be empty for 0 arguments and this is indeed how it is with other dlls: ./dlls/powrprof/powrprof.spec:2:@ stdcall CanUserWritePwrScheme () ./dlls/user32/user32.spec:365:@ stdcall GetProgmanWindow () ./dlls/ws2_32/ws2_32.spec:71:@ stdcall WSACreateEvent () Sorry for all the questions, I hope I'm not annoying anyone.
Hi, Il 19/01/22 01:59, Mohamad Al-Jaf ha scritto:
Thanks, it does make it easier and more readable. Doing this causes no warnings:
char *cur; char *ret;
cur = (char*)&test_CurrentIP; ret = (char*)CurrentIP();
This looks fine to me.
Removing WINAPI from the declaration allows it to compile:
extern void *CurrentIP(void);
I don't understand why this is happening. What am I missing?
Neither do I, although the same happens here. Since I doubt many people are still following the thread, I'd suggest resending the patch with all the modifications and removing that WINAPI, with a comment that says that you don't understand why you have to remove it. And hope that somebody more experienced will have something to say. Giovanni.
Hi, Okay, I've submitted a revision. I tried my best to fix the issue on my own but to no avail. To everyone who's participated in this discussion, thank you all for the help. On Wed, Jan 19, 2022 at 8:24 AM Giovanni Mascellani < gmascellani(a)codeweavers.com> wrote:
Hi,
Il 19/01/22 01:59, Mohamad Al-Jaf ha scritto:
Thanks, it does make it easier and more readable. Doing this causes no warnings:
char *cur; char *ret;
cur = (char*)&test_CurrentIP; ret = (char*)CurrentIP();
This looks fine to me.
Removing WINAPI from the declaration allows it to compile:
extern void *CurrentIP(void);
I don't understand why this is happening. What am I missing?
Neither do I, although the same happens here. Since I doubt many people are still following the thread, I'd suggest resending the patch with all the modifications and removing that WINAPI, with a comment that says that you don't understand why you have to remove it. And hope that somebody more experienced will have something to say.
Giovanni.
On 1/19/22 09:59, Mohamad Al-Jaf wrote:
I see, thanks for the examples. I took a look at them and put this in tests/main.c:
extern void *WINAPI CurrentIP(void);
Doing this, the 64-bit test compiles without a problem, but the 32-bit runs into this problem:
/usr/lib/gcc/i686-w64-mingw32/11.2.0/../../../../i686-w64-mingw32/bin/ld: dlls/wdscore/tests/main.cross.o: in function `test_CurrentIP': src/wine-staging-32-build/../wine-staging/dlls/wdscore/tests/main.c:33: undefined reference to `CurrentIP(a)0' /usr/lib/gcc/i686-w64-mingw32/11.2.0/../../../../i686-w64-mingw32/bin/ld: /src/wine-staging-32-build/../wine-staging/dlls/wdscore/tests/main.c:33: undefined reference to `CurrentIP(a)0' collect2: error: ld returned 1 exit status winegcc: /usr/bin/i686-w64-mingw32-gcc failed make[1]: *** [Makefile:228478: dlls/wdscore/tests/wdscore_test.exe] Error 2
"CurrentIP(a)0" is a mangled symbol. The "@0" suffix means "this function follows the __stdcall calling convention, and it accepts 0 bytes of arguments." However, export symbols are usually exported as unmangled names. Try: DECLSPEC_IMPORT extern void *WINAPI CurrentIP(void);
Removing WINAPI from the declaration allows it to compile:
extern void *CurrentIP(void);
Note that this workaround can only be applied if the function takes no arguments. Otherwise, stack corruption would result since the callee (__stdcall) would pop out the arguments from the stack while the caller doesn't expect that (since it was declared with default calling convention, which is __cdecl).
I don't understand why this is happening. What am I missing? The same error happens if I add a header file and import it. The dll wlanapi also has a void* function (void *WINAPI WlanAllocateMemory) and I don't see anything that might be missing in wdscore. Is there a missing header? I tried adding windows.h and other headers from similar dlls but it still says undefined reference to CurrentIP. Why would it work on 64-bit but not 32-bit?
In any architectures other than IA-32, __stdcall and __cdecl are the same; thus, the name mangling does not take place.
I had a similar problem in the past with UiaRaiseAutomationPropertyChangedEvent but that was due to an incorrect spec file parameter. As far as I know, the spec parameter should be empty for 0 arguments and this is indeed how it is with other dlls:
./dlls/powrprof/powrprof.spec:2:@ stdcall CanUserWritePwrScheme () ./dlls/user32/user32.spec:365:@ stdcall GetProgmanWindow () ./dlls/ws2_32/ws2_32.spec:71:@ stdcall WSACreateEvent ()
The "@NNN" part specifies the argument size, so there's some chance it is probably related. -- Sincerely, Jinoh Kang
Hi, Il 21/01/22 05:32, Jinoh Kang ha scritto:
"CurrentIP(a)0" is a mangled symbol. The "@0" suffix means "this function follows the __stdcall calling convention, and it accepts 0 bytes of arguments."
However, export symbols are usually exported as unmangled names. Try:
DECLSPEC_IMPORT extern void *WINAPI CurrentIP(void);
Trying this on top of Mohamad's v3 patch doesn't fix the problem:
/usr/bin/i686-w64-mingw32-ld: dlls/wdscore/tests/main.cross.o: in function `test_CurrentIP': /home/giovanni/progetti/windows/wine/build32/../wine/dlls/wdscore/tests/main.c:33: undefined reference to `_imp__CurrentIP(a)0' /usr/bin/i686-w64-mingw32-ld: /home/giovanni/progetti/windows/wine/build32/../wine/dlls/wdscore/tests/main.c:33: undefined reference to `_imp__CurrentIP(a)0' collect2: error: ld returned 1 exit status
Giovanni.
Am 21.01.2022 um 11:47 schrieb Giovanni Mascellani <gmascellani(a)codeweavers.com>:
Hi,
Il 21/01/22 05:32, Jinoh Kang ha scritto:
"CurrentIP(a)0" is a mangled symbol. The "@0" suffix means "this function follows the __stdcall calling convention, and it accepts 0 bytes of arguments." However, export symbols are usually exported as unmangled names. Try: DECLSPEC_IMPORT extern void *WINAPI CurrentIP(void);
Trying this on top of Mohamad's v3 patch doesn't fix the problem:
You can always resolve it manually it with GetProcAddress. Afaiu that's preferred anyway, especially for undocumented functions. A quick grep over the Wine code finds only one case of a WINAPI extern void thing in a test: stefan(a)retina wine % grep -r "extern " . | grep tests | grep WINAPI ./dlls/imagehlp/tests/testdll.c:extern DWORD WINAPI StrCmpCA(const char *, const char *); And looking at the file, StrCmpCA isn't used at all in this file, so this line is dead code - unless there is some PE file inspection magic I missed with my (terrible) grep-foo. If you import things via the import library (wdscore.lib, if it exists), it connects the decorated import to the undecorated export: https://stackoverflow.com/questions/38710243/dll-using-stdcall-without-name-... Afaiu linking at build time to a .dll without a .lib is a gcc/mingw-ism anyway. Visual Studio wouldn't allow you to do that. You either need a .lib file or use LoadLibrary + GetProcAddress. Wine doesn't build a libwdscore.a and I can't find a wdscore.lib in my Visual Studio installation.
Hi,
"CurrentIP(a)0" is a mangled symbol. The "@0" suffix means "this function follows the __stdcall calling convention, and it accepts 0 bytes of arguments."
However, export symbols are usually exported as unmangled names. Try:
DECLSPEC_IMPORT extern void *WINAPI CurrentIP(void);
As with Giovanni, this gives the error undefined reference to `_imp__CurrentIP(a)0' But thanks for the detailed explanation, I really appreciate it.
You can always resolve it manually it with GetProcAddress
Thanks! I had no idea you could do that in Wine, I thought it was part of the Windows SDK only. I've submitted a revision. Just wondering if the revision is showing up under this email for anyone else or if it's just me? Maybe it's a Gmail issue. The subject line is different so that's weird.
On 1/17/22 11:55, Mohamad Al-Jaf wrote:
A more likely scenario is that the code address is changing from one invocation to another.
Try compiling with "/LINK /DYNAMICBASE:NO" (in MSVC) or "-no-pie" (in GCC/MinGW).
See also: https://en.wikipedia.org/wiki/Address_space_layout_randomization, and https://en.wikipedia.org/wiki/Position-independent_executable.
I see, compiling it with /LINK /DYNAMICBASE:NO results in the same value. Thanks for the links, good info.
+ "ret" ) +#elif defined(__arm__) +__ASM_STDCALL_FUNC(CurrentIP, 0, + "mov lr, r0\n\t"
Operands are swapped. This is not AT&T syntax. Should be "mov r0, lr".
I'm confused, doesn't mov lr, r0 mean to move the value from lr into r0? Isn't that AT&T syntax? https://csiflabs.cs.ucdavis.edu/~ssdavis/50/att-syntax.htm <https://csiflabs.cs.ucdavis.edu/~ssdavis/50/att-syntax.htm>
Or did you mean to say that arm does not follow AT&T syntax? If so, you're right, the operands are swapped. https://www.keil.com/support/man/docs/armasm/armasm_dom1361289878994.htm <https://www.keil.com/support/man/docs/armasm/armasm_dom1361289878994.htm>
Yes, I did. I apologise for the confusion.
Let me suggest an alternative: instead of assembly, we can just use GCC's built-in functions here for all architectures.
#ifdef __has_builtin #if __has_builtin(__builtin_extract_return_addr) #define extract_retaddr(x) __builtin_extract_return_addr(x) #endif #elif defined(__GNUC__) && __GNUC__ >= 5 #define extract_retaddr(x) __builtin_extract_return_addr(x) #else #define extract_retaddr(x) (x) #endif
void *WINAPI CurrentIP(void) { return extract_retaddr( __builtin_return_address( 0 )); }
Also this means the debug channel is (again) no longer needed.
Thank you, this looks great. But according to the GNU documentation __builtin_return_address is processor specific and might return 0 or worse, a random value: https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Return-Address.html
My understanding is that __builtin_return_address is predictable if it is passed 0 as argument. I believe this one has already been addressed by another commenter, so I won't say anything here.
I think the debug channel is still needed for unsupported architectures. In this case, a return value of 0 is easy to check for, but a random value would result in a false return address. I can't think of a trivial way to write a check for the random value. I suppose I could do something like what Giovanni did with the test but it seems redundant and I'm not sure if it's even considered acceptable code.
If the Media Creation Tool won't still run even after this patch (with builtin wdscore.dll), then I suppose the patch has to wait until the RC code freeze ends.
I have no problem deferring the patch until after the code freeze, but aren't Wine bugs supposed to be specific to one problem? To me, this makes sense intuitively and is in line with how Wine patches are typically small. I just checked the wiki and it also says "Each bug report should cover one problem.".
Yes, it is. My "patch has to wait" remark was merely a suggestion. -- Sincerely, Jinoh Kang
participants (10)
-
Gabriel Ivăncescu -
Giovanni Mascellani -
Jinoh Kang -
Marvin -
Mohamad Al-Jaf -
Nikolay Sivov -
Saulius Krasuckas -
Stefan Dösinger -
Zebediah Figura -
Zebediah Figura (she/her)