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

ThunkLibs: Fix hostcall packer generation #1805

Closed
wants to merge 3 commits into from
Closed

Conversation

skmp
Copy link
Contributor

@skmp skmp commented Jun 27, 2022

Overview

This fixes the generation of the hostcall packers, so we don't need to wrap thunks with C++ templates.

Depends

#1818

@skmp skmp force-pushed the skmp/gen-hostcall-packers branch 2 times, most recently from 8327a45 to 9d16716 Compare June 27, 2022 18:20
@skmp skmp requested a review from neobrain June 27, 2022 18:29
@neobrain
Copy link
Member

I'm not convinced it's beneficial to rid of PackedArguments at the expense of moving more logic to the generator. The template is very lightweight, and my measurements indicated it doesn't contribute significantly to the compile time of large thunklibs like libGL.

IMO we should move towards minimizing logic in the generator. The struct-repacking branch gave me a bad feeling about how robust a code generator with lots of manual branching is, and the PackedArguments template is a good way to avoid that pitfall. I'd prefer shelving this PR for now until I've done more tinkering with the idea.

@skmp
Copy link
Contributor Author

skmp commented Jun 29, 2022

I'm not convinced it's beneficial to rid of PackedArguments at the expense of moving more logic to the generator. The template is very lightweight, and my measurements indicated it doesn't contribute significantly to the compile time of large thunklibs like libGL.

The code was already there, the logic it wasn't moved. I just fixed the generation so it can be used. Before it was generated, broken, and we also had the PackedArguments.

IMO we should move towards minimizing logic in the generator. The struct-repacking branch gave me a bad feeling about how robust a code generator with lots of manual branching is, and the PackedArguments template is a good way to avoid that pitfall. I'd prefer shelving this PR for now until I've done more tinkering with the idea.

I don't necessarily agree there. The PackedArguments template is a non-generalized huge thing, so it is obviously not a great solution either way. Plus it freezes up my editor.

Also, we can't use templates when doing cross bitness thunking, and evne for x86-64 / arm64 not all the packing rules are the same. We'll have to generate custom structs for everything for correctness, anyway.

Personally, I prefer codegeneration over templates for these things. Templates can be used where they are a good fit, like handling the conversions and stuff.

@skmp skmp force-pushed the skmp/gen-hostcall-packers branch from 9d16716 to 9cb78dd Compare June 30, 2022 10:05
@skmp
Copy link
Contributor Author

skmp commented Jun 30, 2022

This has been updated to add hostcall_ aliases generation, needed by #1817

@skmp skmp force-pushed the skmp/gen-hostcall-packers branch from 9cb78dd to cb8fb94 Compare June 30, 2022 11:05
@skmp skmp force-pushed the skmp/gen-hostcall-packers branch from cb8fb94 to b338f87 Compare June 30, 2022 11:11
@skmp skmp requested a review from Sonicadvance1 August 1, 2022 21:55
@Sonicadvance1
Copy link
Member

"This fixes the generation of the hostcall packers" I don't understand what is being fixed here or why it is broken.

@skmp
Copy link
Contributor Author

skmp commented Aug 3, 2022

"This fixes the generation of the hostcall packers" I don't understand what is being fixed here or why it is broken.

"so we don't need to wrap thunks with C++ templates."

@skmp
Copy link
Contributor Author

skmp commented Aug 3, 2022

As mentioned in a previous comment, the hostpackers were broken, and unused. I don't understand what is there to not understand here.

@neobrain
Copy link
Member

neobrain commented Aug 5, 2022

FWIW #1868 adds a feature that makes the _hostcall symbols unnecessary and hence removes them entirely.

@Sonicadvance1
Copy link
Member

Nothing changed on this so far.

@skmp skmp marked this pull request as draft August 10, 2022 16:00
@skmp skmp modified the milestones: 2210, 2209 Aug 10, 2022
@skmp skmp self-assigned this Aug 10, 2022
@skmp
Copy link
Contributor Author

skmp commented Aug 29, 2022

Nothing changed on this so far.

I've been blocked by lack of review/alternative proposals apart from "i don't like this / I don't think it is needed". Now, more time has to be wasted on rebases as well.

@skmp
Copy link
Contributor Author

skmp commented Sep 1, 2022

(Closing this as there is an ongoing powergrab by @Sonicadvance1, I will migrate my work to https://github.com/skmp/fex-emu-ng.git)

@skmp skmp closed this Sep 1, 2022
@Sonicadvance1 Sonicadvance1 deleted the skmp/gen-hostcall-packers branch August 23, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔖 Ready
Development

Successfully merging this pull request may close these issues.

3 participants