robertblaketaylor commented 3 years ago

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 commented 3 years ago

What compiler is this?

dwilliamson commented 3 years ago

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

dwilliamson commented 3 years ago

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 commented 3 years ago

What compiler is this?

SDK - 10.0.18362.0

robertblaketaylor commented 3 years ago

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 commented 3 years ago

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

ifdef RMT_PLATFORM_WINDOWS

typedef long NTSTATUS;

include

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 commented 3 years ago

Updates to compile:

ifdef RMT_PLATFORM_WINDOWS

typedef long NTSTATUS;

include

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 commented 3 years ago

Did you get the missing commit? https://github.com/Celtoys/Remotery/commit/218175ba6ebad407589ee6630886ced072e51734

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

robertblaketaylor commented 3 years ago

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 commented 3 years ago

Sorry about that, try https://github.com/Celtoys/Remotery/commit/8ce0d7c7b46b030b440b58a1a8b22a5b46108bc3.

robertblaketaylor commented 3 years ago

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

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

robertblaketaylor commented 3 years ago

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 commented 3 years ago

Yes, please. I also posted a newer fix.

robertblaketaylor commented 3 years ago

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

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 commented 3 years ago

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

dwilliamson commented 3 years ago

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 commented 3 years ago

Thanks for the update!

Celtoys / Remotery

Compile failures with latest changeset #172

ifdef RMT_PLATFORM_WINDOWS

include

include

pragma comment(lib, "Winmm.lib")

pragma comment(lib, "ws2_32.lib")

endif

ifdef RMT_PLATFORM_WINDOWS

include

include

include

pragma comment(lib, "Winmm.lib")

pragma comment(lib, "ws2_32.lib")

endif

ifdef RMT_PLATFORM_WINDOWS

pragma comment(lib, "ws2_32.lib")

endif

ifdef RMT_PLATFORM_WINDOWS

include

include

include

pragma comment(lib, "Winmm.lib")

pragma comment(lib, "ws2_32.lib")

endif

ifdef RMT_PLATFORM_WINDOWS

pragma comment(lib, "ws2_32.lib")

pragma comment(lib, "winmm.lib")

endif

ifdef RMT_PLATFORM_WINDOWS

include

include

pragma comment(lib, "ws2_32.lib")

pragma comment(lib, "winmm.lib")

endif