[PATCH] wdscore: Implement CurrentIP.

[Mohamad Al-Jaf]

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/6465d9c6801768c56c7ca1faebc65f15e40a07a8/StackManager.cpp#L358

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 at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220116/8760975f/attachment.htm>