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.

Sign up for GitHub

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

Jump to bottom

GCC4.9+ de-virtualization issues. #51

Open
KyleSanderson opened this issue Jul 17, 2018 · 9 comments
Open

GCC4.9+ de-virtualization issues. #51

KyleSanderson opened this issue Jul 17, 2018 · 9 comments

Comments

@KyleSanderson
Copy link
Member

https://travis-ci.org/alliedmodders/metamod-source/jobs/405106127

 ./core/sourcehook/test/test_sourcehook/test_sourcehook -v
TestList passed
No error: ModuleInMemory
No error: Part 1
No error: Part 2
FAIL: Should be:
  State_F1_HookAdded; Param1=1
  State_F1_Called
  State_F1_PreHandler_Called; Param1=0xffdf5072
FAIL: Is:
  State_F1_HookAdded; Param1=1
  State_F1_Called
  State_F1_Called
TestBasic FAILED: Part 3
FAIL: Should be:
  State_EatYams_Called; Param1=0
  State_EatYams_Called; Param1=1
  State_EatYams_Handler_Called; Param1=0
  State_EatYams_Called; Param1=0
  State_EatYams_Handler_Called; Param1=1
  State_EatYams_Called; Param1=1
FAIL: Is:
  State_EatYams_Called; Param1=0
  State_EatYams_Called; Param1=1
  State_EatYams_Called; Param1=0
  State_EatYams_Called; Param1=1
TestVafmtAndOverload FAILED: Part 1
No error: Part 1
No error: Part 1.1
No error: Part 2
FAIL: Should be:
  State_Func1H_Called; Param1=0x9edd090
  State_Func1_Called; Param1=0x9edd090
  State_Func2H_Called; Param1=0x9edd090
  State_Func2_Called; Param1=0x9edd098
  State_Func3H_Called; Param1=0x9edd090
  State_Func3_Called; Param1=0x9edd090
  State_Func1H_Called; Param1=0x9edd090
  State_Func1_Called; Param1=0x9edd090
  State_Func2H_Called; Param1=0x9edd090
  State_Func2_Called; Param1=0x9edd098
FAIL: Is:
  State_Func1_Called; Param1=0x9edd090
  State_Func2_Called; Param1=0x9edd098
  State_Func3_Called; Param1=0x9edd090
  State_Func1_Called; Param1=0x9edd090
  State_Func2_Called; Param1=0x9edd098
TestThisPtrOffs FAILED: Part 3
FAIL: Should be:
  State_Func1H_Called
  State_Func1_Called
  State_Func2_Called
  State_Func2H_Called
  State_Func3H_Called
  State_Func3_Called
FAIL: Is:
  State_Func1_Called
  State_Func2_Called
  State_Func3_Called
TestPlugSys FAILED: Part 1.1
No error: Part 1
No error: Part 2.1
No error: Part 3
No error: Part 4
No error: Part 5
No error: Part 6
TestBail passed
FAIL: Should be:
  State_H_C1_F; Param1=0x811391c
  State_H_C1_G; Param1=0x811391c
  State_H_C2_F; Param1=0x8113918
  State_H_C2_G; Param1=0x8113918
  State_H_C3_F; Param1=0x8113914
  State_H_C3_G; Param1=0x8113914
  State_H_C4_F; Param1=0x8113910
  State_H_C4_G; Param1=0x8113910
  State_H_C5_F; Param1=0x811390c
  State_H_C5_G; Param1=0x811390c
  State_H_C6_F; Param1=0x8113908
  State_H_C6_G; Param1=0x8113908
  State_H_C7_F; Param1=0x8113904
  State_H_C7_G; Param1=0x8113904
  State_H_C8_F; Param1=0x8113900
  State_H_C8_G; Param1=0x8113900
  State_C8_G; Param1=0x8113900
  State_C8_F; Param1=0x8113900
  State_C7_G; Param1=0x8113904
  State_C7_F; Param1=0x8113904
  State_C6_G; Param1=0x8113908
  State_C6_F; Param1=0x8113908
  State_C5_G; Param1=0x811390c
  State_C5_F; Param1=0x811390c
  State_C4_G; Param1=0x8113910
  State_C4_F; Param1=0x8113910
  State_C3_G; Param1=0x8113914
  State_C3_F; Param1=0x8113914
  State_C2_G; Param1=0x8113918
  State_C2_F; Param1=0x8113918
  State_C1_G; Param1=0x811391c
  State_C1_F; Param1=0x811391c
FAIL: Is:
  State_C1_F; Param1=0x811391c
TestReentr FAILED: 1
No error: Part 1
No error: Part 1.1
FAIL: Should be:
  State_Func1H_Called; Param1=0x9eded50
  State_Func1_Called; Param1=0x9eded50
  State_Func2H_Called; Param1=0x9eded50; Param2=200
  State_Func2_Called; Param1=0x9eded50; Param2=-2023406815
  State_Func3H_Called; Param1=0x9eded50
  State_Func3_Called; Param1=0x9eded50
  State_Return; Param1=3
  State_Func4H_Called; Param1=0x9eded50; Param2=400
  State_Func4_Called; Param1=0x9eded50; Param2=305419896
  State_Return; Param1=4
FAIL: Is:
  State_Func1_Called; Param1=0x9eded50
  State_Func2_Called; Param1=0x9eded50; Param2=200
  State_Func3_Called; Param1=0x9eded50
  State_Return; Param1=3
  State_Func4_Called; Param1=0x9eded50; Param2=400
  State_Return; Param1=4
TestManual FAILED: Part 2
FAIL: Should be:
  State_H1_Func1; Param1=77
  State_H2_Func1; Param1=5
  State_Func1; Param1=0
  State_HP_Func1; Param1=0; Param2=0x9eddae0
FAIL: Is:
  State_Func1; Param1=77
TestRecall FAILED: Part 1
TestMulti FAILED: g_callcount[0] != 0
No error: Part 1
No error: Part 2
FAIL: Should be:
  State_Result_InHook; Param1=0
  State_Result; Param1=20
  State_Result_InHook; Param1=10
  State_Result; Param1=20
  State_Result_InHook; Param1=11
  State_Result; Param1=20
  State_Result_InHook; Param1=12
  State_Result; Param1=20
FAIL: Is:
  State_Result; Param1=0
  State_Result; Param1=10
  State_Result; Param1=11
  State_Result; Param1=12
TestRef FAILED: Part 3
No error: Part 1
FAIL: Should be:
  State_Func1_Pre1; Param1=0x9eddb5c
  State_Func1; Param1=0x9edd4dc
  State_Func1_Ret; Param1=0x9eddb5c
FAIL: Is:
  State_Func1; Param1=0x9edd4dc
  State_Func1_Ret; Param1=0x9edd4dc
TestRefRet FAILED: Part 2
FAIL: Should be:
  State_Func1_Pre; Param1=0xffdf5088
  State_D1_Func1; Param1=0xffdf5088
  State_Func1_Pre; Param1=0xffdf508c
  State_D1_Func1; Param1=0xffdf508c
  State_D2_Func1; Param1=0xffdf5090
FAIL: Is:
  State_D1_Func1; Param1=0xffdf5088
  State_D1_Func1; Param1=0xffdf508c
  State_D2_Func1; Param1=0xffdf5090
TestVPHooks FAILED: Part 1
TestCPageAlloc passed
No error: GlobCtors
No error: Test0 Part1
FAIL: Should be:
  State_Deleg_0; Param1=1; Param2=0x9edd560; Param3=0; Param4=
  State_Func0; Param1=0x9edd560; Param2=
  State_Deleg_0; Param1=3; Param2=0x9edd560; Param3=1; Param4=
  State_Func0; Param1=0x9edd560; Param2=
FAIL: Is:
  State_Func0; Param1=0x9edd560; Param2=
  State_Func0; Param1=0x9edd560; Param2=
/home/travis/.travis/job_stages: line 78:  6108 Segmentation fault      (core dumped) ./core/sourcehook/test/test_sourcehook/test_sourcehook -v
The command "./core/sourcehook/test/test_sourcehook/test_sourcehook -v" exited with 139.```
@KyleSanderson KyleSanderson mentioned this issue Jul 17, 2018
Merged
@KyleSanderson
Copy link
Member Author

GCC-4.9 also shows this. 4.8 completes no problem.

$ ./core/sourcehook/test/test_sourcehook/test_sourcehook -v
TestList passed
No error: ModuleInMemory
No error: Part 1
No error: Part 2
FAIL: Should be:
  State_F1_HookAdded; Param1=1
  State_F1_Called
  State_F1_PreHandler_Called; Param1=0xfff98472
FAIL: Is:
  State_F1_HookAdded; Param1=1
  State_F1_Called
  State_F1_Called
TestBasic FAILED: Part 3
FAIL: Should be:
  State_EatYams_Called; Param1=0
  State_EatYams_Called; Param1=1
  State_EatYams_Handler_Called; Param1=0
  State_EatYams_Called; Param1=0
  State_EatYams_Handler_Called; Param1=1
  State_EatYams_Called; Param1=1
FAIL: Is:
  State_EatYams_Called; Param1=0
  State_EatYams_Called; Param1=1
  State_EatYams_Called; Param1=0
  State_EatYams_Called; Param1=1
TestVafmtAndOverload FAILED: Part 1
No error: Part 1
No error: Part 1.1
No error: Part 2
FAIL: Should be:
  State_Func1H_Called; Param1=0x8b30090
  State_Func1_Called; Param1=0x8b30090
  State_Func2H_Called; Param1=0x8b30090
  State_Func2_Called; Param1=0x8b30098
  State_Func3H_Called; Param1=0x8b30090
  State_Func3_Called; Param1=0x8b30090
  State_Func1H_Called; Param1=0x8b30090
  State_Func1_Called; Param1=0x8b30090
  State_Func2H_Called; Param1=0x8b30090
  State_Func2_Called; Param1=0x8b30098
FAIL: Is:
  State_Func1_Called; Param1=0x8b30090
  State_Func2_Called; Param1=0x8b30098
  State_Func3_Called; Param1=0x8b30090
  State_Func1_Called; Param1=0x8b30090
  State_Func2_Called; Param1=0x8b30098
TestThisPtrOffs FAILED: Part 3
FAIL: Should be:
  State_Func1H_Called
  State_Func1_Called
  State_Func2_Called
  State_Func2H_Called
  State_Func3H_Called
  State_Func3_Called
FAIL: Is:
  State_Func1_Called
  State_Func2_Called
  State_Func3_Called
TestPlugSys FAILED: Part 1.1
No error: Part 1
No error: Part 2.1
No error: Part 3
No error: Part 4
No error: Part 5
No error: Part 6
TestBail passed
FAIL: Should be:
  State_H_C1_F; Param1=0x810f93c
  State_H_C1_G; Param1=0x810f93c
  State_H_C2_F; Param1=0x810f938
  State_H_C2_G; Param1=0x810f938
  State_H_C3_F; Param1=0x810f934
  State_H_C3_G; Param1=0x810f934
  State_H_C4_F; Param1=0x810f930
  State_H_C4_G; Param1=0x810f930
  State_H_C5_F; Param1=0x810f92c
  State_H_C5_G; Param1=0x810f92c
  State_H_C6_F; Param1=0x810f928
  State_H_C6_G; Param1=0x810f928
  State_H_C7_F; Param1=0x810f924
  State_H_C7_G; Param1=0x810f924
  State_H_C8_F; Param1=0x810f920
  State_H_C8_G; Param1=0x810f920
  State_C8_G; Param1=0x810f920
  State_C8_F; Param1=0x810f920
  State_C7_G; Param1=0x810f924
  State_C7_F; Param1=0x810f924
  State_C6_G; Param1=0x810f928
  State_C6_F; Param1=0x810f928
  State_C5_G; Param1=0x810f92c
  State_C5_F; Param1=0x810f92c
  State_C4_G; Param1=0x810f930
  State_C4_F; Param1=0x810f930
  State_C3_G; Param1=0x810f934
  State_C3_F; Param1=0x810f934
  State_C2_G; Param1=0x810f938
  State_C2_F; Param1=0x810f938
  State_C1_G; Param1=0x810f93c
  State_C1_F; Param1=0x810f93c
FAIL: Is:
  State_C1_F; Param1=0x810f93c
TestReentr FAILED: 1
No error: Part 1
No error: Part 1.1
FAIL: Should be:
  State_Func1H_Called; Param1=0x8b31d50
  State_Func1_Called; Param1=0x8b31d50
  State_Func2H_Called; Param1=0x8b31d50; Param2=200
  State_Func2_Called; Param1=0x8b31d50; Param2=-2023406815
  State_Func3H_Called; Param1=0x8b31d50
  State_Func3_Called; Param1=0x8b31d50
  State_Return; Param1=3
  State_Func4H_Called; Param1=0x8b31d50; Param2=400
  State_Func4_Called; Param1=0x8b31d50; Param2=305419896
  State_Return; Param1=4
FAIL: Is:
  State_Func1_Called; Param1=0x8b31d50
  State_Func2_Called; Param1=0x8b31d50; Param2=200
  State_Func3_Called; Param1=0x8b31d50
  State_Return; Param1=3
  State_Func4_Called; Param1=0x8b31d50; Param2=400
  State_Return; Param1=4
TestManual FAILED: Part 2
FAIL: Should be:
  State_H1_Func1; Param1=77
  State_H2_Func1; Param1=5
  State_Func1; Param1=0
  State_HP_Func1; Param1=0; Param2=0x8b30ae0
FAIL: Is:
  State_Func1; Param1=77
TestRecall FAILED: Part 1
TestMulti FAILED: g_callcount[0] != 0
No error: Part 1
No error: Part 2
FAIL: Should be:
  State_Result_InHook; Param1=0
  State_Result; Param1=20
  State_Result_InHook; Param1=10
  State_Result; Param1=20
  State_Result_InHook; Param1=11
  State_Result; Param1=20
  State_Result_InHook; Param1=12
  State_Result; Param1=20
FAIL: Is:
  State_Result; Param1=0
  State_Result; Param1=10
  State_Result; Param1=11
  State_Result; Param1=12
TestRef FAILED: Part 3
No error: Part 1
FAIL: Should be:
  State_Func1_Pre1; Param1=0x8b30b5c
  State_Func1; Param1=0x8b304dc
  State_Func1_Ret; Param1=0x8b30b5c
FAIL: Is:
  State_Func1; Param1=0x8b304dc
  State_Func1_Ret; Param1=0x8b304dc
TestRefRet FAILED: Part 2
No error: Part 1
No error: Part 2
No error: Part 3
No error: Part 4
No error: Part 5
No error: Part 6
No error: Part 7.1
No error: Part 7.2
TestVPHooks passed
TestCPageAlloc passed
No error: GlobCtors
No error: Test0 Part1
FAIL: Should be:
  State_Deleg_0; Param1=1; Param2=0x8b30978; Param3=0; Param4=
  State_Func0; Param1=0x8b30978; Param2=
  State_Deleg_0; Param1=3; Param2=0x8b30978; Param3=1; Param4=
  State_Func0; Param1=0x8b30978; Param2=
FAIL: Is:
  State_Func0; Param1=0x8b30978; Param2=
  State_Func0; Param1=0x8b30978; Param2=
/home/travis/.travis/job_stages: line 78:  7534 Segmentation fault      ./core/sourcehook/test/test_sourcehook/test_sourcehook -v
The command "./core/sourcehook/test/test_sourcehook/test_sourcehook -v" exited with 139.
0.00s$ cd .. && mkdir build-sh-debug && cd build-sh-debug```

@KyleSanderson KyleSanderson changed the title GCC6 fails SH-Tests GCC4.9+ fails SH-Tests Jul 17, 2018
@pavolmarko
Copy link
Contributor

pavolmarko commented Jul 18, 2018 via email

@pavolmarko
Copy link
Contributor

So, the issue seems to be that GCC completely optimizes away the virtual function calls, so the hooks can not run :-)

It does this when the class the hook is being installed on is declared in the anonymous namespace (which makes sense, because if there's no class deriving from it, there can be no other subclass in the program, so there's no point doing virtual function calls).

It seems to work when I move the classes out of the anonymous namespace. I'd suggest having a pattern like:

namespace
{
StateList g_States;
SourceHook::ISourceHook *g_SHPtr;
SourceHook::Plugin g_PLID;
//..
MAKE_STATE_1(State_ModuleInMemory, bool);
// Other MAKE_STATE_s
}

class Test
{
public:
virtual void F1()
{
ADD_STATE(State_F1_Called);
}
virtual void F2(){}
//...
};

namespace
{
SH_DECL_HOOK1(Test, F299, SH_NOATTRIB, 0, bool, const char *);
SH_DECL_HOOK0_void(Test, F1, SH_NOATTRIB, 0);
// .. and other stuff
}

We may need to watch out for duplicate names between the test source files then.

Would be cool if you had time to do this! :-) Otherwise I may have come around to do it within the next ~2 weeks.

@KyleSanderson
Copy link
Member Author

Thanks for checking PM :-) . I won't have time for a bit yet so if you're able to own this that would be fantastic.

Is this going to wreck us if Valve flips to gcc-4.9 or greater? Any idea what we can do when this happens?

@pavolmarko
Copy link
Contributor

No, it seems that the core functionality works fine, only the tests are broken.

tldr; the compiler avoids virtual function calls, so SourceHook's functionality is skipped in the tests.

Long version:
SourceHook can only hook virtual functions and it works by overwriting the function's entry in the class' virtual function table.
When the compiler generates code for a virtual function call, it usually looks like this in pseudocode:

// Real code:
TestClass* test_class = /* ... */ ;
test_class->TestVirtualFunc(5);

// Pseudocode
// (1) get the vtable pointer from the TestClass instance
// This can be imagined as accessing a hidden member
TestClass_Vtable* vtable = test_class->vtable;
// (2) extract the function 
TestVirtualFuncType* func_ptr = vtable->entry_for_TestVirtualFunc;
// (3) Call the function through the function pointer. The this pointer can be imagined as a hidden
// first argument.
func_ptr(test_class 5);

SourceHook works by modifying the class' virtual function table, so step (2) will not yield a pointer to the the actual function "TestVirtualFunc", but instead to SourceHook's "hookman" function generated in the SH_DECL_HOOK* macro.

This virtual function call ("late binding") is pretty expensive compared to a non-virtual function call:

// Real code:
TestClass* test_class = /* ... */ ;
test_class->TestNonVirtualFunc(5);

// Pseudocode
TestNonVirtualFunc_Impl(test_class 5);

Here, the compiler knows the address of the TestNonVirtualFunc function, so it does not have to look it up anywhere.

Now, if a compiler sees code like this:

// Version A:
TestClass* test_class = new SuperDerivedFromTestClass();
test_class->TestVirtualFunc(5);

// Version B:
TestClass* test_class = function_from_another_compilation_unit();
test_class->TestVirtualFunc(5);// Version 2:

In Version B, |function_from_another_compilation_unit| could be returning a class derived from |TestClass| that's not even known to the compiler here, where |TestVirtualFunc| may be overridden, so it has to look up TestVirtualFunc in the vtable and call it through that.

In Version A however, the compiler knows exactly wht SuperDerivedFromTestClass is and where it generated code for SuperDerivedFromTestClass::TestVirtualFunc. Looking it up through the vtable would be a waste of resources, so the compiler actually generates code for a non-virtual function call which will be significantly faster at runtime.

This is an issue in tests for SourceHook: We want to test that virtual function calls behave in a specific way, so if the compiler is clever enough to not even generate a virtual function call, the tests fail. For example:

void Hook(int some_param) {
  printf("test\n");
}

class SuperDerivedFromTestClass {
public:
  void TestVirtualFunc(int some_param) override {
    printf("SDFTC::TVF called\n");
  }
};

TestClass* test_class = new SuperDerivedFromTestClass();
SH_ADD_HOOK(test_class, TestVirtualFunc, SH_STATIC(&Hook));
test_class->TestVirtualFunc(5);

This will only print "SDFTC::TVF called" but will not print "test", because the compiler sees that
test_class->TestVirtualFunc is defined right up there and can be called directly. The compiler does not assume that some hacky non-standard-compliant hackers want to mess with the function call.

This is what happens in this bug too: GCC can optimize away the virtual function calls in the tests.

@KyleSanderson
Copy link
Member Author

The compiler does not assume that some hacky non-standard-compliant hackers want to mess with the function call.
Exactly 😸 . Maybe I didn't ask the question appropriately, but are we going to start seeing functions that were virtual become static calls if Valve has a similar class setup in their headers that we have in sh-tests?

I suppose when this happens (if it hasn't already) we're going to start to need actual detour support in MM:S instead of the standalone conflict-ridden header we've had in SM extensions for eons.

@pavolmarko
Copy link
Contributor

pavolmarko commented Jul 30, 2018 via email

@dvander
Copy link
Member

dvander commented Jul 31, 2018

Thanks for the write-up PM! Are there ways to trick the compiler outside of crossing translation units or using shared libraries? Would storing+reading the pointer in the heap, or using some kind of union fool it?

@KyleSanderson KyleSanderson changed the title GCC4.9+ fails SH-Tests GCC4.9+ de-virtualization issues. Jul 31, 2018
@KyleSanderson
Copy link
Member Author

The test suite has been fixed by disabling this optimization at build-time as per #53 .

Leaving this open for further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dvander @KyleSanderson @pavolmarko