[PATCH v2 0/1] MR1395: dbghelp: Add SymAddrIncludeInlineTrace stub.

[Mohamad Al-Jaf (＠maljaf)]

Football Manager 2023 Editor calls this function.

The header already includes the prototype.

--
  v2: dbghelp: Add SymAddrIncludeInlineTrace stub.

https://gitlab.winehq.org/wine/wine/-/merge_requests/1395

[PATCH v2 1/1] dbghelp: Add SymAddrIncludeInlineTrace stub.

[Mohamad Al-Jaf]

From: Mohamad Al-Jaf

Football Manager 2023 Editor calls this function.
---
v2: - Move function just above SymSrvGetFileIndexInfo.
---
 dlls/dbghelp/dbghelp.spec |  2 +-
 dlls/dbghelp/symbol.c     | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/dlls/dbghelp/dbghelp.spec b/dlls/dbghelp/dbghelp.spec
index 754fb456611..336a6f7aefd 100644
--- a/dlls/dbghelp/dbghelp.spec
+++ b/dlls/dbghelp/dbghelp.spec
@@ -38,7 +38,7 @@
 @ stub SymAddSourceStreamW
 @ stdcall SymAddSymbol(ptr int64 str int64 long long)
 @ stdcall SymAddSymbolW(ptr int64 wstr int64 long long)
-@ stub SymAddrIncludeInlineTrace
+@ stdcall SymAddrIncludeInlineTrace(long int64)
 @ stdcall SymCleanup(long)
 @ stub SymCompareInlineTrace
 @ stub SymDeleteSymbol
diff --git a/dlls/dbghelp/symbol.c b/dlls/dbghelp/symbol.c
index c82b5b3d4ef..474d2a0c45c 100644
--- a/dlls/dbghelp/symbol.c
+++ b/dlls/dbghelp/symbol.c
@@ -2787,6 +2787,17 @@ BOOL WINAPI SymGetLineFromInlineContextW(HANDLE hProcess, DWORD64 addr, ULONG in
     return internal_line_copy_toW64(&intl, line);
 }

+/******************************************************************
+ *      SymAddrIncludeInlineTrace (DBGHELP.@)
+ *
+ */
+DWORD WINAPI SymAddrIncludeInlineTrace(HANDLE hProcess, DWORD64 addr)
+{
+    FIXME("(%p, %I64x): stub\n", hProcess, addr);
+    SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
+    return 0;
+}
+
 /******************************************************************
  *      SymSrvGetFileIndexInfo (DBGHELP.@)
  *

[PATCH v2 1/1] dbghelp: Add SymAddrIncludeInlineTrace stub.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=126336

Your paranoid android.

=== debian11 (32 bit report) ===

d3drm:
d3drm.c:6777: Test failed: Cannot get IDirect3DRMDevice3 interface, hr 0x8007000e.
d3drm.c:6783: Test failed: Cannot get IDirect3DRMViewport2 interface, hr 0x8876030d
Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00408a30).

=== debian11 (build log) ===

0550:err:winediag:d3d_device_create The application wants to create a Direct3D device, but the current DirectDrawRenderer does not support this.

[Mohamad Al-Jaf (＠maljaf)]

On Fri Nov 18 04:11:28 2022 +0000, eric pouech wrote:
> Hi
> - could you rather put the implementation in symbol.c near the other
>
> inline context related functions?
> (at the end, just above SymSrvGetFileIndexInfo)?
> 2) is this the only DbgHelp's symbol missing for the app?
> thanks

Hi,

1. Sure
2. It looks that way, yeah. The user who reported this issue said it only happens when they click on the "People" button in the app. They said this stub fixes the crash and haven't reported any other issue since then. But it's always possible that they just haven't stumbled upon an unimplemented call from features they haven't used.

https://github.com/ValveSoftware/Proton/issues/6320

Thanks for the review!

[eric pouech (＠epo)]

On Fri Nov 18 04:11:28 2022 +0000, Mohamad Al-Jaf wrote:
> Hi,
> - Sure
> - It looks that way, yeah. The user who reported this issue said it
>
> only happens when they click on the "People" button in the app. They
> said this stub fixes the crash and haven't reported any other issue
> since then. But it's always possible that they just haven't stumbled
> upon an unimplemented call from features they haven't used.
> https://github.com/ValveSoftware/Proton/issues/6320
> Thanks for the review!

@maljaf:
A) I didn't look closely at the patch in the first run... calling SetLastError() is wrong (and will never be checked by callers).
B) I have actually been working on implementing this function (among a partial implementation of another one).
What I suggest is that I include your patch in the serie I'm about to send. (removing the offending SetLastError() call)
It may be that the actual implementation of SymAddrIncludeInlineTrace will trigger different code paths in the mentionned game so separating stub from implementation will help on bisecting (if ever needed)

[Mohamad Al-Jaf (＠maljaf)]

On Fri Nov 18 08:02:34 2022 +0000, eric pouech wrote:
> @maljaf:
> - I didn't look closely at the patch in the first run... calling
>
> SetLastError() is wrong (and will never be checked by callers).
> 2. I have actually been working on implementing this function (among a
> partial implementation of another one).
> What I suggest is that I include your patch in the serie I'm about to
> send. (removing the offending SetLastError() call)
> It may be that the actual implementation of SymAddrIncludeInlineTrace
> will trigger different code paths in the mentionned game so separating
> stub from implementation will help on bisecting (if ever needed).
> Submitted as !1438

@epo:

Could you explain why it's wrong? I used past commits as a reference and the stubs that were added had SetLastError(ERROR_CALL_NOT_IMPLEMENTED). Yeah I agree, I don't think apps actually check for it.

Sure, that's fine.

Yeah, that makes sense given what the function is used for.

Thanks for the link, closing this merge request.

[PATCH v2 0/1] MR1395: dbghelp: Add SymAddrIncludeInlineTrace stub. - closed

[Mohamad Al-Jaf (＠maljaf)]

This merge request was closed by Mohamad Al-Jaf.

[eric pouech (＠epo)]

On Fri Nov 18 08:12:32 2022 +0000, Mohamad Al-Jaf wrote:
> @epo: 
> Could you explain why it's wrong? I used past commits as a reference and
> the stubs that were added had SetLastError(ERROR_CALL_NOT_IMPLEMENTED).
> Yeah I agree, I don't think apps actually check for it.
> Sure, that's fine. 
> Yeah, that makes sense given what the function is used for.
> Thanks for the link, closing this merge request.

setting last error is only meaningful when you can tell when the function failed

(returning a BOOL: FALSE is an error, returning NTSTATUS: anything but STATUS_SUCCESS is an error, ...
in this very function, there's no documented way to return an error...)

0 means there's no inline context at this address... but it doesn't tell if the address is actually mapped, if there's a loaded module at this address, if there's a function at this address...

[Mohamad Al-Jaf (＠maljaf)]

On Fri Nov 18 08:58:19 2022 +0000, eric pouech wrote:
> setting last error is only meaningful when you can tell when the
> function failed
> (returning a BOOL: FALSE is an error, returning NTSTATUS: anything but
> STATUS_SUCCESS is an error, ...
> in this very function, there's no documented way to return an error...)
> 0 means there's no inline context at this address... but it doesn't tell
> if the address is actually mapped, if there's a loaded module at this
> address, if there's a function at this address...

Oh right, that's pretty obvious now that I think about it. Thanks so much for explaining it though.

And thanks again for the review. :)