Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile failures with latest changeset #172

Open
robertblaketaylor opened this issue Feb 26, 2021 · 23 comments
Open

Compile failures with latest changeset #172

robertblaketaylor opened this issue Feb 26, 2021 · 23 comments

Comments

@robertblaketaylor
Copy link

robertblaketaylor commented Feb 26, 2021

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
Copy link
Collaborator

What compiler is this?

@dwilliamson
Copy link
Collaborator

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

@dwilliamson
Copy link
Collaborator

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
Copy link
Author

What compiler is this?

SDK - 10.0.18362.0

@robertblaketaylor
Copy link
Author

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
Copy link
Author

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
Copy link
Author

robertblaketaylor commented Feb 26, 2021

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

Sorry about that, try 8ce0d7c.

@robertblaketaylor
Copy link
Author

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

image

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

robertblaketaylor commented Mar 3, 2021

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
Copy link
Collaborator

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

@robertblaketaylor
Copy link
Author

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
Copy link
Collaborator

Yes, please. I also posted a newer fix.

@robertblaketaylor
Copy link
Author

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):
image

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
Copy link
Author

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

@dwilliamson
Copy link
Collaborator

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
Copy link
Author

Thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants