Re: [PATCH v5 2/4] ntdll: Implement RtlWalkFrameChain() for x86 and x86_64.

[Giovanni Mascellani]

Signed-off-by: Giovanni Mascellani

On 03/11/21 11:41, Paul Gofman wrote:
> Signed-off-by: Paul Gofman
>
> v5:
>      - also test for requested_count == frame_count + 1.
>
> dlls/ntdll/exception.c              | 24 ++++++++-
>   dlls/ntdll/ntdll.spec               |  2 +-
>   dlls/ntdll/signal_i386.c            | 17 +++---
>   dlls/ntdll/signal_x86_64.c          |  7 +--
>   dlls/ntdll/tests/exception.c        | 83 +++++++++++++++++++++++++++++
>   dlls/ntoskrnl.exe/ntoskrnl.exe.spec |  2 +-
>   include/ddk/ntddk.h                 |  3 ++
>   include/winnt.h                     |  2 +
>   8 files changed, 124 insertions(+), 16 deletions(-)
>
> diff --git a/dlls/ntdll/exception.c b/dlls/ntdll/exception.c
> index e956d8a722f..b020b829ae7 100644
> --- a/dlls/ntdll/exception.c
> +++ b/dlls/ntdll/exception.c
> @@ -30,6 +30,7 @@
>   #define WIN32_NO_STATUS
>   #include "windef.h"
>   #include "winternl.h"
> +#include "ddk/ntddk.h"
>   #include "ddk/wdm.h"
>   #include "wine/exception.h"
>   #include "wine/list.h"
> @@ -1074,6 +1075,25 @@ USHORT WINAPI RtlCaptureStackBackTrace( ULONG skip, ULONG count, PVOID *buffer,
>   {
>       ULONG ret;
> - ret = capture_stack_back_trace( skip, count, buffer, hash );
> - return min( ret, (USHORT)~0 );
> - ret = capture_stack_back_trace( skip, skip + count, buffer, hash );
> - if (ret < skip) return 0;
> - return min( ret - skip, (USHORT)~0 );
>
> +}
> -
> -
>
> +/**********************************************************************
> -
> -
>          RtlWalkFrameChain      (NTDLL.@)
> - */
>
> +ULONG WINAPI RtlWalkFrameChain( void **callers, ULONG count, ULONG skip )
> +{
> - TRACE( "callers %p, count %u, skip %#x.\n", callers, count, skip );
> -
> - if (skip & ~(0xff << RTL_STACK_WALKING_MODE_FRAMES_TO_SKIP_SHIFT))
> - {
> -
>    WARN( "Invalid flags %#x.\n", skip );
> -
>    return 0;
> - }
> - skip >>= RTL_STACK_WALKING_MODE_FRAMES_TO_SKIP_SHIFT;
> -
> - return capture_stack_back_trace( skip, count, callers, NULL );
> }
>
> diff --git a/dlls/ntdll/ntdll.spec b/dlls/ntdll/ntdll.spec
> index 13e65f65139..c54b2e205aa 100644
> --- a/dlls/ntdll/ntdll.spec
> +++ b/dlls/ntdll/ntdll.spec
> @@ -1073,7 +1073,7 @@
>   @ stdcall RtlWakeAddressSingle(ptr)
>   @ stdcall RtlWakeAllConditionVariable(ptr)
>   @ stdcall RtlWakeConditionVariable(ptr)
> -@ stub RtlWalkFrameChain
> +@ stdcall RtlWalkFrameChain(ptr long long)
>   @ stdcall RtlWalkHeap(long ptr)
>   @ stdcall RtlWow64EnableFsRedirection(long)
>   @ stdcall RtlWow64EnableFsRedirectionEx(long ptr)
> diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c
> index 26150ce877b..ff3e837c784 100644
> --- a/dlls/ntdll/signal_i386.c
> +++ b/dlls/ntdll/signal_i386.c
> @@ -488,27 +488,26 @@ ULONG WINAPI capture_stack_back_trace( ULONG skip, ULONG count, PVOID *buffer, U
>       CONTEXT context;
>       ULONG i;
>       ULONG *frame;
> -
>
> ULONG num_entries = 0;
>
> ++skip;
> -
>
> ++count;
>
> RtlCaptureContext( &context );
>  if (hash) *hash = 0;
>  frame = (ULONG *)context.Ebp;
> - while (skip--)
> - {
> -
>    if (!is_valid_frame( frame )) return 0;
> -
>    frame = (ULONG *)*frame;
> - }
> -
> for (i = 0; i < count; i++)
> {
>     if (!is_valid_frame( frame )) break;
> -
>    buffer[i] = (void *)frame[1];
> -
>    if (hash) *hash += frame[1];
> -
>    if (i >= skip)
> -
>    {
> -
>        buffer[num_entries++] = (void *)frame[1];
> -
>        if (hash) *hash += frame[1];
> -
>    }
>     frame = (ULONG *)*frame;
> }
> - return i;
> - return i ? i - 1 : 0;
> }
>
> diff --git a/dlls/ntdll/signal_x86_64.c b/dlls/ntdll/signal_x86_64.c
> index 5f3c8f70208..4a9c7d4b9ff 100644
> --- a/dlls/ntdll/signal_x86_64.c
> +++ b/dlls/ntdll/signal_x86_64.c
> @@ -1504,16 +1504,17 @@ ULONG WINAPI capture_stack_back_trace( ULONG skip, ULONG count, PVOID *buffer, U
>       TRACE( "(%u, %u, %p, %p)\n", skip, count, buffer, hash );
>   ++skip;
> -
>
> ++count;
>
> RtlCaptureContext( &context );
>  dispatch.TargetIp      = 0;
>  dispatch.ContextRecord = &context;
>  dispatch.HistoryTable  = &table;
>  if (hash) *hash = 0;
> - for (i = 0; i < skip + count; i++)
> - for (i = 0; i < count; i++)
>  {
>      status = virtual_unwind( UNW_FLAG_NHANDLER, &dispatch, &context );
> -
>    if (status != STATUS_SUCCESS) return i;
> -
>    if (status != STATUS_SUCCESS) break;
>
>     if (!dispatch.EstablisherFrame) break;
>
> @@ -1532,7 +1533,7 @@ ULONG WINAPI capture_stack_back_trace( ULONG skip, ULONG count, PVOID *buffer, U
>       }
>       if (hash && num_entries > 0) *hash = hash_pointers( buffer, num_entries );
>       TRACE( "captured %u frames\n", num_entries );
> - return num_entries;
> - return i ? i - 1 : 0;
> }
>
> diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c
> index 6af68317732..6f3b08b5ea6 100644
> --- a/dlls/ntdll/tests/exception.c
> +++ b/dlls/ntdll/tests/exception.c
> @@ -28,6 +28,7 @@
>   #include "winnt.h"
>   #include "winreg.h"
>   #include "winternl.h"
> +#include "ddk/ntddk.h"
>   #include "ddk/wdm.h"
>   #include "excpt.h"
>   #include "wine/test.h"
> @@ -9113,6 +9114,87 @@ static void test_copy_context(void)
>   }
>   #endif
>
> +static void test_walk_stack(void)
> +{
> - ULONG count, expected, frame_count, requested_count, skip_count;
> - void *addrs[256], *addrs2[256];
> - void *start, *end;
> - unsigned int i, j;
> -
> - memset(addrs, 0xcc, sizeof(addrs));
> - memset(addrs2, 0xcc, sizeof(addrs2));
> -
> - frame_count = RtlCaptureStackBackTrace(0, ARRAY_SIZE(addrs), addrs, NULL);
> - count = RtlWalkFrameChain(addrs2, ARRAY_SIZE(addrs2), 0);
> -
> - trace("frame_count %u.\n", frame_count);
> -
> - ok(frame_count > 1, "Got zero frame_count.\n");
> - ok(count == frame_count, "Got unexpected frame_count %u, count %u.\n", frame_count, count);
> -
> - start = test_walk_stack;
> - end = (BYTE *)start + 0x1000;
> - todo_wine_if(sizeof(void *) == 4)
> - ok(addrs[0] >= start && addrs[0] < end, "Address is not inside test function, start %p, end %p, addr %p.\n",
> -
>        start, end, addrs[0]);
> - todo_wine_if(sizeof(void *) == 4)
> - ok(addrs2[0] >= start && addrs2[0] < end, "Address is not inside test function, start %p, end %p, addr %p.\n",
> -
>        start, end, addrs2[0]);
> -
> - for (i = 1; i < frame_count; ++i)
> - {
> -
>    ok(addrs[i] == addrs2[i], "i %u, addresses do not match, %p vs %p.\n", i, addrs[i], addrs2[i]);
> - }
> - todo_wine ok(!!addrs[frame_count - 1], "Expected non-NULL last address.\n");
> -
> - for (requested_count = frame_count - 1; requested_count <= frame_count + 1; ++requested_count)
> - {
> -
>    for (i = 0; i < 32; ++i)
> -
>    {
> -
>        winetest_push_context("requested_count %u, i %u", requested_count, i);
> -
>        skip_count = (1 << i) >> 8;
> -
> -
>        if (i < RTL_STACK_WALKING_MODE_FRAMES_TO_SKIP_SHIFT
> -
>                || i >= RTL_STACK_WALKING_MODE_FRAMES_TO_SKIP_SHIFT + 8)
> -
>            expected = 0;
> -
>        else
> -
>            expected = min(frame_count, requested_count);
> -
> -
>        memset(addrs2, 0xcc, sizeof(addrs2));
> -
>        count = RtlWalkFrameChain(addrs2, requested_count, 1 << i);
> -
>        ok(count == expected, "Got unexpected frame_count %u, expected %u.\n", count, expected);
> -
> -
>        if (skip_count < count)
> -
>            count -= skip_count;
> -
>        else
> -
>            count = 0;
> -
> -
>        for (j = 0; j < count; ++j)
> -
>            ok( addrs2[j] != (void *)(ULONG_PTR)0xcccccccccccccccc, "Address is not set, j %u.\n", j );
> -
>        for (; j < ARRAY_SIZE(addrs2); ++j)
> -
>            ok( addrs2[j] == (void *)(ULONG_PTR)0xcccccccccccccccc, "Address is set, j %u.\n", j );
> -
> -
>        if (!count)
> -
>        {
> -
>            winetest_pop_context();
> -
>            continue;
> -
>        }
> -
> -
>        memset(addrs, 0xcc, sizeof(addrs));
> -
>        expected = skip_count > frame_count ? 0 : min(frame_count - skip_count, requested_count);
> -
>        count = RtlCaptureStackBackTrace(skip_count, requested_count, addrs, NULL);
> -
>        ok(count == expected, "Got unexpected frame_count %u, expected %u. i %u.\n", count, expected, i);
> -
> -
>        count = min(frame_count, requested_count) - skip_count;
> -
>        for (j = 0; j < count; ++j)
> -
>        {
> -
>            ok(addrs[j] == addrs2[j], "Addresses do not match, j %u, %p, %p.\n", j, addrs[j], addrs2[j]);
> -
>        }
> -
>        winetest_pop_context();
> -
>    }
> - }
>
> +}
> - START_TEST(exception)
> {
>     HMODULE hntdll = GetModuleHandleA("ntdll.dll");
>
> @@ -9339,5 +9421,6 @@ START_TEST(exception)
>       test_suspend_thread();
>       test_suspend_process();
>       test_unload_trace();
> - test_walk_stack();
>  VirtualFree(code_mem, 0, MEM_RELEASE);
> }
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> index 0208e2f633f..b94e58ccb26 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -1300,7 +1300,7 @@
>   @ stdcall RtlVerifyVersionInfo(ptr long int64)
>   @ stdcall -arch=arm64,x86_64 RtlVirtualUnwind(long long long ptr ptr ptr ptr ptr)
>   @ stub RtlVolumeDeviceToDosName
> -@ stub RtlWalkFrameChain
> +@ stdcall RtlWalkFrameChain(ptr long long)
>   @ stdcall RtlWriteRegistryValue(long ptr ptr long ptr long)
>   @ stub RtlZeroHeap
>   @ stdcall RtlZeroMemory(ptr long)
> diff --git a/include/ddk/ntddk.h b/include/ddk/ntddk.h
> index 41ad3d721bd..ce558139aa6 100644
> --- a/include/ddk/ntddk.h
> +++ b/include/ddk/ntddk.h
> @@ -245,6 +245,8 @@ typedef EXPAND_STACK_CALLOUT *PEXPAND_STACK_CALLOUT;
>   typedef GUID UUID;
>   #endif
>
> +#define RTL_STACK_WALKING_MODE_FRAMES_TO_SKIP_SHIFT 8
> - NTSTATUS  WINAPI ExUuidCreate(UUID*);
> NTSTATUS  WINAPI IoQueryDeviceDescription(PINTERFACE_TYPE,PULONG,PCONFIGURATION_TYPE,PULONG,
>                                   PCONFIGURATION_TYPE,PULONG,PIO_QUERY_DEVICE_ROUTINE,PVOID);
>
> @@ -267,5 +269,6 @@ NTSTATUS  WINAPI PsSetCreateThreadNotifyRoutine(PCREATE_THREAD_NOTIFY_ROUTINE);
>   NTSTATUS  WINAPI PsSetLoadImageNotifyRoutine(PLOAD_IMAGE_NOTIFY_ROUTINE);
>   void      WINAPI RtlInitializeGenericTableAvl(PRTL_AVL_TABLE,PRTL_AVL_COMPARE_ROUTINE,PRTL_AVL_ALLOCATE_ROUTINE, PRTL_AVL_FREE_ROUTINE,void *);
>   void      WINAPI RtlInsertElementGenericTableAvl(PRTL_AVL_TABLE,void *,ULONG,BOOL*);
> +ULONG     WINAPI RtlWalkFrameChain(void **,ULONG,ULONG);
>
> #endif
> diff --git a/include/winnt.h b/include/winnt.h
> index ef731e29c52..eb1bd165720 100644
> --- a/include/winnt.h
> +++ b/include/winnt.h
> @@ -1840,6 +1840,8 @@ NTSYSAPI PVOID   WINAPI RtlVirtualUnwind(DWORD,ULONG_PTR,ULONG_PTR,RUNTIME_FUNCT
>
> #endif
>
> +NTSYSAPI USHORT  WINAPI RtlCaptureStackBackTrace(ULONG,ULONG,void **,ULONG *);
> - /*
> - Product types
> */