<div dir="ltr">Hi Giovanni,<br><br>> Given your implementation, it seems more likely that CurrentIP returns<br>> void *, instead of int (which would get the pointer truncated, e.g., on<br>> i386). This is also hinted by this snippet that was mentioned in the bug:<br>> <a href="https://github.com/seven-mile/CallCbsCore/blob/6465d9c6801768c56c7ca1faebc65f15e40a07a8/StackManager.cpp#L358">https://github.com/seven-mile/CallCbsCore/blob/6465d9c6801768c56c7ca1faebc65f15e40a07a8/StackManager.cpp#L358</a><br><br>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.<br><br>> Same here. Notice that CurrentIP is not declared here, so the compiler<br>> defaults to the terrible fallback of believing it returns int, though I<br>> don't think this is appropriate here. And you should declare it anyway.<br>> I don't know if there is a header in which it is sensible to put the<br>> declaration.<br><br>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.<br><br>> But just tracing the result is not very useful for testing. Given that<br>> your hypothesis seems to be that CurrentIP returns the return address of<br>> the call, you could try to check that such address is in the caller<br>> function. There is no portable way to do that, but a reasonable attempt<br>> is to check that ret is higher than &test_CurrentIP and lower than, say,<br>> &test_CurrentIP + 0x100. I did a quick test and this seems indeed to be<br>> the case:<br><br>Thank you, that's a great way to check for it. Here's what I have in the test, are these error messages good?<br><br>static void test_CurrentIP(void)<br>{<br> void *cur;<br> void *ret;<br><br> cur = &test_CurrentIP;<br> ret = CurrentIP();<br><br> ok(ret != 0, "Unsupported architecture\n");<br> ok(ret > cur, "&test_CurrentIP=%p CurrentIP()=%p\n", cur, ret);<br> ok(ret < (cur + 0x100), "&test_CurrentIP + 0x100=%p CurrentIP()=%p\n", cur + 0x100, ret);<br>}<br><br>> I think it would be useful add saving all the CPU registers (before or<br>> maybe after calling the function) in the test and dumping them together<br>> with the CurrentIP() return value using just trace().<br>> <br>> So different test runs would provide more data to decide if there is<br>> some correlation between what CurrentIP() returns and any of the regs.<br><br>I'm not sure if this is considered acceptable in Wine, but thanks for the suggestion.<br><br>> A more likely scenario is that the code address is changing from one invocation to another.<br>> <br>> Try compiling with "/LINK /DYNAMICBASE:NO" (in MSVC) or "-no-pie" (in GCC/MinGW).<br>> <br>> See also: <a href="https://en.wikipedia.org/wiki/Address_space_layout_randomization">https://en.wikipedia.org/wiki/Address_space_layout_randomization</a>, and <a href="https://en.wikipedia.org/wiki/Position-independent_executable">https://en.wikipedia.org/wiki/Position-independent_executable</a>.<br><br>I see, compiling it with /LINK /DYNAMICBASE:NO results in the same value. Thanks for the links, good info.<br><br>> > + "ret" )<br>> > +#elif defined(__arm__)<br>> > +__ASM_STDCALL_FUNC(CurrentIP, 0,<br>> > + "mov lr, r0\n\t"<br>> <br>> Operands are swapped. This is not AT&T syntax. Should be "mov r0, lr".<br><br>I'm confused, doesn't mov lr, r0 mean to move the value from lr into r0? Isn't that AT&T syntax? <a href="https://csiflabs.cs.ucdavis.edu/~ssdavis/50/att-syntax.htm">https://csiflabs.cs.ucdavis.edu/~ssdavis/50/att-syntax.htm</a><br><br>Or did you mean to say that arm does not follow AT&T syntax? If so, you're right, the operands are swapped. <a href="https://www.keil.com/support/man/docs/armasm/armasm_dom1361289878994.htm">https://www.keil.com/support/man/docs/armasm/armasm_dom1361289878994.htm</a><br><br>> Let me suggest an alternative: instead of assembly, we can just use GCC's built-in functions here for all architectures.<br>> <br>> #ifdef __has_builtin<br>> #if __has_builtin(__builtin_extract_return_addr)<br>> #define extract_retaddr(x) __builtin_extract_return_addr(x)<br>> #endif<br>> #elif defined(__GNUC__) && __GNUC__ >= 5<br>> #define extract_retaddr(x) __builtin_extract_return_addr(x)<br>> #else<br>> #define extract_retaddr(x) (x)<br>> #endif<br>> <br>> void *WINAPI CurrentIP(void)<br>> {<br>> return extract_retaddr( __builtin_return_address( 0 ));<br>> }<br>> <br>> Also this means the debug channel is (again) no longer needed.<br><br>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: <a href="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</a><br><br>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.<br><br>> If the Media Creation Tool won't still run even after this patch (with builtin wdscore.dll),<br>> then I suppose the patch has to wait until the RC code freeze ends.<br><br>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.".<br><br>> You could probably write a conformance test by manually emitting<br>> assembly, e.g. on i386 something like<br>> <br>> call CurrentIP<br>> 1: cmp %eax, $1b<br>> sete %al<br>> ret<br>> <br>> That's a bit of work, though, and it's not clear to me that it's worthwhile.<br><br>Thank you. Agreed, it doesn't seem worthwhile. I think Giovanni's test case is sufficient.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jan 14, 2022 at 11:46 AM Zebediah Figura <<a href="mailto:zfigura@codeweavers.com">zfigura@codeweavers.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 1/13/22 23:31, Mohamad Al-Jaf wrote:<br>
>> Adding the debug channel declaration should be deferred until the first<br>
>> patch that uses it.<br>
> <br>
> I was going to remove it but since the #ifdef guards ends with an unknown<br>
> architecture, it would require the debug channel to display the fixme<br>
> message, no?<br>
<br>
Yes, in that case it would belong in this patch.<br>
<br>
>> Is there any indication this function should be doing something like<br>
>> this at all?<br>
> <br>
> I'm testing it out in Windows to get a sense of what it does. Here's my<br>
> code in C++, let me know if it's incorrect:<br>
> <br>
> typedef int (*CurrentIP)();<br>
> HMODULE hDLL = LoadLibraryA("wdscore.dll");<br>
> CurrentIP ip = (CurrentIP)GetProcAddress(hDLL, "CurrentIP");<br>
> std::cout << "CurrentIP() = " << ip() << "\n";<br>
> FreeLibrary(hDLL);<br>
> <br>
> It returns random numbers, e.g. 10424371, 1249331, 6033459. So doesn't this<br>
> mean it's returning the instruction pointer?<br>
> <br>
> If so, I don't know how to write a conformance test for it. Would it be<br>
> necessary in this case?<br>
<br>
You could probably write a conformance test by manually emitting <br>
assembly, e.g. on i386 something like<br>
<br>
call CurrentIP<br>
1: cmp %eax, $1b<br>
sete %al<br>
ret<br>
<br>
That's a bit of work, though, and it's not clear to me that it's worthwhile.<br>
</blockquote></div>