[PATCH] wdscore: Implement CurrentIP.

[Jinoh Kang]

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