Compile failures with latest changeset

[robertblaketaylor]

Wanted to pick up the latest changeset to get the PRTH message, running into failures on windows:

\remotery.c(4706): error C2143: syntax error: missing ')' before '__cdecl'
\remotery.c(4706): error C2143: syntax error: missing ';' before '__cdecl'
\remotery.c(4706): error C2059: syntax error: ')'
\remotery.c(4707): error C2065: 'NTQUERYINFOMATIONTHREAD': undeclared identifier
\remotery.c(4707): error C2146: syntax error: missing ';' before identifier 'NtQueryInformationThread'
\remotery.c(4707): error C2065: 'NtQueryInformationThread': undeclared identifier
\remotery.c(4707): error C2146: syntax error: missing ';' before identifier 'GetProcAddress'
\remotery.c(4711): error C2146: syntax error: missing ';' before identifier 'status'
\remotery.c(4711): warning C4550: expression evaluates to a function which is missing an argument list
\remotery.c(4711): error C2065: 'status': undeclared identifier
\remotery.c(4711): warning C4013: 'NtQueryInformationThread' undefined; assuming extern returning int
\remotery.c(4712): error C2065: 'status': undeclared identifier
\remotery.c(4740): warning C4311: 'type cast': pointer truncation from 'BYTE *' to 'DWORD'
\remotery.c(4743): warning C4133: 'function': incompatible types - from 'WCHAR [256]' to 'const char *'
\remotery.c(5055): warning C4013: 'timeBeginPeriod' undefined; assuming extern returning int
\remotery.c(5055): error C2065: 'TIMERR_NOERROR': undeclared identifier
\remotery.c(5235): warning C4013: 'timeEndPeriod' undefined; assuming extern returning int
\remotery.c(6219): warning C4013: 'mbstowcs_s' undefined; assuming extern returning int

[dwilliamson]

What compiler is this?

[dwilliamson]

Actually, more importantly, what version of the Windows SDK are you using?

[dwilliamson]

My guess right now is that you're using one of the latest SDKs, which have cleaned up their head includes significantly. Which means Remotery.c is missing some required includes...

timeEndPeriod et. al. require <timeapi.h>.

I think the first error is due to NTSTATUS. This lives in <winternl.h> but my concern is that it's not on the default additional include paths for VC projects. This won't be a problem because we can just typedef LONG NTSTATUS.

I have commit a fix for the DWORD cast problem.

There remains an issue with the cast from WCHAR [256] to const char*. While this will compile and "work" it will not return correct thread names for unnamed threads. It should normally return the DLL name but in the 64-bit case this needs a WideCharToMultiByte pass.

Can you verify the header include changes for me?

[robertblaketaylor]

What compiler is this?

SDK - 10.0.18362.0

[robertblaketaylor]

Pulling in timeapi.h opens up a few new issues, will take a look on my end as well:

\windows10sdk\10.0.18362.0\include\10.0.18362.0\um\mmsyscom.h(94): error C2061: syntax error: identifier 'MMVERSION'
\windows10sdk\10.0.18362.0\include\10.0.18362.0\um\mmsyscom.h(94): error C2059: syntax error: ';'
\windows10sdk\10.0.18362.0\include\10.0.18362.0\um\mmsyscom.h(98): error C2143: syntax error: missing ')' before 'return'
\windows10sdk\10.0.18362.0\include\10.0.18362.0\um\mmsyscom.h(98): error C2143: syntax error: missing '{' before 'return'
\windows10sdk\10.0.18362.0\include\10.0.18362.0\um\mmsyscom.h(98): error C2059: syntax error: 'return'
\windows10sdk\10.0.18362.0\include\10.0.18362.0\um\mmsyscom.h(98): error C2059: syntax error: ')'

[robertblaketaylor]

Still have the WCHAR issue and the multibyte to wide with this patch:

#ifdef RMT_PLATFORM_WINDOWS
typedef long NTSTATUS;
#include <Windows.h>
#include <timeapi.h>
#pragma comment(lib, "Winmm.lib")
#pragma comment(lib, "ws2_32.lib")
#endif

\remotery.c(4751): warning C4133: 'function': incompatible types - from 'WCHAR [256]' to 'const char *'
\remotery.c(6275): warning C4013: 'mbstowcs_s' undefined; assuming extern returning int

[robertblaketaylor]

Updates to compile:

#ifdef RMT_PLATFORM_WINDOWS
typedef long NTSTATUS;
#include <Windows.h>
#include <timeapi.h>
#include <stdlib.h>
#pragma comment(lib, "Winmm.lib")
#pragma comment(lib, "ws2_32.lib")
#endif

if (start_address >= (DWORD_PTR)module_entry.modBaseAddr && start_address <= ((DWORD_PTR)module_entry.modBaseAddr + module_entry.modBaseSize))
{
static char name[256];
sprintf(name, "%ws", module_entry.szModule);
//strcpy_s(name, sizeof(name), module_entry.szModule);
CloseHandle(handle);
return name;
}

However it looks like the processor index is not getting set right (with the sample callback bytes) for 64bit, asserting quite early on:

// Mark the processor this thread was last recorded as running on.
// Note that a thread might be pre-empted multiple times in-between sampling. Given a sampling rate equal to the
// scheduling quantum, this doesn't happen too often. However in such cases, whoever marks the processor last is
// the one that gets recorded.
sample_count = AtomicAdd(&watched_thread->nbSamplesWithoutCallback, 1);
processor_index = watched_thread->processorIndex;
processors[processor_index].thread = watched_thread;

[dwilliamson]

Did you get the missing commit? 218175b

I build on the 10 SDK here but use my own TinyWindows.h. I can swap over and see what's going on.

[robertblaketaylor]

Did you get the missing commit? 218175b

I build on the 10 SDK here but use my own TinyWindows.h. I can swap over and see what's going on.

Ya I've got that commit locally.

[dwilliamson]

Sorry about that, try 8ce0d7c.

[robertblaketaylor]

Thanks for the updated diff, with that diff I'm still hitting this:

It looks like on the first time through that processor_index is still -1 (4294967295). My other local changes include what is posted above (on top of your latest diff (8ce0d7c):

--- old
#ifdef RMT_PLATFORM_WINDOWS
#pragma comment(lib, "ws2_32.lib")
#endif

--- new
#ifdef RMT_PLATFORM_WINDOWS
typedef long NTSTATUS;
#include <Windows.h>
#include <timeapi.h>
#include <stdlib.h>
#pragma comment(lib, "Winmm.lib")
#pragma comment(lib, "ws2_32.lib")
#endif

--- old
if (start_address >= (DWORD_PTR)module_entry.modBaseAddr && start_address <= ((DWORD_PTR)module_entry.modBaseAddr + module_entry.modBaseSize))
{
static char name[256];
strcpy_s(name, sizeof(name), module_entry.szModule);
CloseHandle(handle);
return name;
}
--- new
if (start_address >= (DWORD_PTR)module_entry.modBaseAddr && start_address <= ((DWORD_PTR)module_entry.modBaseAddr + module_entry.modBaseSize))
{
static char name[256];
sprintf(name, "%ws", module_entry.szModule);
CloseHandle(handle);
return name;
}

[dwilliamson]

I've updated with the latest compile fixes, thanks for those. Except the WC/MB one, which I want to solve with WideCharToMultiByte or wcstombs. I'm just a little slow with these changes as I use my own CRT and need to add them when I need them.

[dwilliamson]

The code with processor_index should not just blindly be trusting that the value is no longer -1. If a callback gets scheduled and it hasn't executed because a thread is sleeping by the time it comes back round, it will fail. However, depending on your circumstances, indexing the array with -1 may not be fatal (though it should trigger heap protection asserts).

[dwilliamson]

This is going to take me a couple days to sort out properly. MS has gone all-in on rearranging headers so that the same set of includes doesn't work with different SDK versions. Right now 2015 can't find <timeapi.h>

[robertblaketaylor]

This is going to take me a couple days to sort out properly. MS has gone all-in on rearranging headers so that the same set of includes doesn't work with different SDK versions. Right now 2015 can't find <timeapi.h>

Let me know if you have anything you want me to try out on my end, thanks!

[robertblaketaylor]

Something else I ran into, can file a separate issue. When nesting begin/end samples with RMTSF_None sample flag in the same scope looks to break all sampling, ex:

void Foo() {
  rmt_ScopedCPUSample(fooSample, 0);
  for (int32_t i = 0; i < 10; ++i) {
    rmt_ScopedCPUSample(loopIteration, 0);
  }
}

I don't see any samples show up after exiting the scope of Foo. Let me know if this is expected. If I remove 1 or the other scoped sample, then looks to show up fine.

[dwilliamson]

No, that should work fine, please open another issue. You should get 10 nested samples of loopIteration.

[robertblaketaylor]

I notice you posted a fix for the 64bit offsets, is this a good time for me to pull latest and see if the sample for the processor is working?

[dwilliamson]

Yes, please. I also posted a newer fix.

[robertblaketaylor]

Pull tip in locally, starting to see some of the processing sampling come up, but running into a few bugs out of the box:

• CPU samples are not coming through for the thread or for the self profiling
• The timeline looks to be broken
• Naming on the process samples looks to be the time duration instead of the hash tag

Here is a picture for context (better/more authentic feedback that my points above):

I did have to apply two diffs locally to get it to compile:

Original:
#ifdef RMT_PLATFORM_WINDOWS
#pragma comment(lib, "ws2_32.lib")
#pragma comment(lib, "winmm.lib")
#endif

Updated:
#ifdef RMT_PLATFORM_WINDOWS
#include <Windows.h>
#include <timeapi.h>
#pragma comment(lib, "ws2_32.lib")
#pragma comment(lib, "winmm.lib")
#endif

• stderr: c:\open\ovrsource\socialvr\third-party\remotery\source\remotery\remotery.c(5574): error C2065: 'TIMERR_NOERROR': undeclared identifier
• wchar error from before

[robertblaketaylor]

@dwilliamson anything I can do here to help with repro/issue?

[dwilliamson]

So we no longer have issues with the 64-bit build at runtime, it's just compilation.

The compile issue got worse with a couple recent additions and it's all to do with <windows.h> on different platforms.

I can't get to this until the end of the week as work as taken over.

However the steps necessary to close this are:

• Add a new VS 2019 build target to AppVeyor.
• Add a unicode build variant to AppVeyor.
• Fix everything so that it 100% compiles on all platforms.
• [optional] See if we can get some AppVeyor configs that can target different Windows SDK versions.

It's so odd that this is more difficult than getting this compiling across Windows/OSX/Linux.

[robertblaketaylor]

Thanks for the update!