bombomby / optick

C++ Profiler For Games
https://optick.dev
MIT License
2.95k stars 296 forks source link

Crash brofiling static constructor #29

Closed MatillaMartin closed 5 years ago

MatillaMartin commented 7 years ago

I get a crash using this minimal example:

#include "Brofiler.h"
class Foo
{
public:
    Foo(){ BROFILER_CATEGORY(__FUNCSIG__, Brofiler::Color::Khaki) }
};

Foo foo;

int main( int argc, char * argv[] ) {}

Details on the crash:

Exception thrown: 'System.AccessViolationException' in BrofilerTest.exe Additional information: Attempted to read or write protected memory. This often an indication that other memory is corrupt. If there is a handler for this exception, the program may be safely continued.

I get the crash when brofiling the constructors of static members of global variables. I guess that this is because the BROFILER_CATEGORY call is done before the BrofilerCore library is loaded?

Is there any way to ensure the library is loaded before any global member is initialized?

MatillaMartin commented 7 years ago

More info on the crash:

Exception thrown at 0x7555C54F (KernelBase.dll) in BrofilerTest.exe: 0x04242420 (parameters: 0x31415927, 0x70BE0000, 0x001CF5B0).

Stack: image

The last readable stack function is line 59 of MT::Mutex::Lock

Machine: Windows 7 x64 Build: x86

MatillaMartin commented 7 years ago

My guess is that this is happening: https://isocpp.org/wiki/faq/ctors#static-init-order

The lock has not been initialized yet

bombomby commented 7 years ago

Hey @MatillaMartin , I've submitted a fix for your problem, but keep in mind that Brofiler will not collect events until you first hit PROFILER_FRAME macro - it checks the connection and activates event collection mechanism inside this macro.

I guess in your case you are trying to profile your game startup. If so - see the trick I used with function static local variable to manage the order of initialization. I would recommend to use similar trick to make sure, that your functions are called after PROFILER_FRAME is executed.

Another trick that might be useful for startup analysis is to use PROFILER_FRAME twice - first time in the beginning of your main() and second time inside your game loop. It will allow Brofiler to activate startup analysis before doing initialization if you force it so and will preserve the standard frame-like approach by default.

Give me a shout if you still have a crash with my fix.

MatillaMartin commented 7 years ago

I had to do the same with the EventDescriptionBoard (sorry I can't do a pull request right now):

EventDescriptionBoard.h

 class EventDescriptionBoard
 {
        std::vector<EventDescription*> board;
-       static EventDescriptionBoard instance;
+
 public:
     EventDescription* CreateDescription();

EventDescriptionBoard.cpp

 EventDescriptionBoard& EventDescriptionBoard::Get()
 {
+       static EventDescriptionBoard instance;
        return instance;
 }

-Brofiler::EventDescriptionBoard EventDescriptionBoard::instance;
+//Brofiler::EventDescriptionBoard EventDescriptionBoard::instance;

My static object constructors called BROFILER_CATEGORY (and this created an Event). They were assigned the first indices in the board, but then the static EventDescriptionBoard::instance was constructed, restarting the board indices, thus overwriting the event description.

MatillaMartin commented 7 years ago

One last comment, on https://isocpp.org/wiki/faq/ctors#static-init-order it indicates to use:

Foo & getFoo()
{
    static Foo * foo = new Foo(); // instead of static Foo foo; This does not leak as it is only called once
    return *foo;
}

to avoid issues with destruction. Any specific reason to use the current method?

bombomby commented 5 years ago

I've elimintated all static or global initializers in the latest version. Closing this issue.