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

DHooks x64 Linux enabler #2292

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Conversation

rtldg
Copy link
Contributor

@rtldg rtldg commented Mar 2, 2025

Companion to alliedmodders/metamod-source#212

For x86_64SystemVDefault.cpp, I copied x86_64MicrosoftDefault.cpp and then replaced things so you can see the diff in the second commit (link) on this branch.

Currently includes the changes from #2290

TODO:

  • Handle a TODO note in x86_64SystemVDefault.cpp
  • More testing...
  • Even more testing...

@rtldg
Copy link
Contributor Author

rtldg commented Mar 2, 2025

Seems like the tests are going to be failing until alliedmodders/metamod-source#212 is merged :(

@rtldg rtldg marked this pull request as draft March 2, 2025 11:39
Copy link
Member

@Kenzzer Kenzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much to say at the moment, just that any hooks with 'vector' is going to make all of this explode.

if (m_vecArgTypes[i].custom_register == None)
{
// No matter what, the stack is allocated in slices of 8 bytes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How true does this hold for Linux ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out to not be the case because (non-pointer trivial) objects can be passed inline on the stack which is really unfortunate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I recalled reading, but I just wanted to make sure. As things stands right now, I'm doubtful the callconv implem is robust enough. You don't have/nor should you have to handle all cases, it's most likely going to look messy, but it's fine to just iterate m_vecArgTypes and just take them at face value (i.e we handle int, float, ptrs & vector, and we error by default if object/ref/or otherwise, because we don't know how to handle it)

void x86_64SystemVDefault::SaveReturnValue(CRegisters* registers)
{
// It doesn't matter what the return value is, it will always be fitting on 8 bytes (or less)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is definitively not true for SystemV, return value can get exploded on two registers.

Copy link
Contributor Author

@rtldg rtldg Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is definitively not true for SystemV, return value can get exploded on two registers.

This should only matter in DHooks for ReturnType_Vector being split into XMM0/XMM1, right?

EDIT: Well, SourceHook != DHooks so I guess the pain remains...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given our present situation, yes it's not critical to handle all use-case. But it does need to be handled for vector return...

Comment on lines +278 to +279
// the actual underlining type does not matter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, there's more to investigate

@rtldg
Copy link
Contributor Author

rtldg commented Mar 2, 2025

I'll investigate more for those review comments

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

Successfully merging this pull request may close these issues.

2 participants