Skip to content

Revisit MustBePassedByReference(ClangType)'s implementation #135

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
Closed
PathogenDavid opened this issue Jan 8, 2021 · 1 comment
Closed

Revisit MustBePassedByReference(ClangType)'s implementation #135

PathogenDavid opened this issue Jan 8, 2021 · 1 comment
Labels
Area-Translation Issues concerning the translation from libclang into Biohazrd Concept-Correctness Issues concerning the correctness of the translation from an ABI perspective

Comments

@PathogenDavid
Copy link
Member

PathogenDavid commented Jan 8, 2021 •
edited
Loading

The ClangType overload of MustBePassedByReference contains a tiny version of type reduction that very likely misses some edge cases. (Like attributed types, although we disabled those recently to resolve #124.)

https://github.com/InfectedLibraries/Biohazrd/blob/357faf9a59ed39864325d58c9a2d92315fa3e5a6/Biohazrd/ClangSharpExtensions.cs#L99-L112

I am fairly certain this could/should be modified to just use Clang's CanonicalType instead.
@PathogenDavid PathogenDavid added Concept-Correctness Issues concerning the correctness of the translation from an ABI perspective Area-Translation Issues concerning the translation from libclang into Biohazrd RelativelySmall Issues that are relatively small and could be good candidates for someone's first issue. and removed RelativelySmall Issues that are relatively small and could be good candidates for someone's first issue. labels Jan 8, 2021
@PathogenDavid
Copy link
Member Author

(Removing the RelativelySmall label because while this issue is relatively simple to implement, validating it is fairly complicated.)

PathogenDavid added a commit that referenced this issue Jun 21, 2021
@PathogenDavid
Reworked how function/method ABI concerns are handled to take advanta…
8d30b35 
…ge of Clang's code generation module.

In short, this change replaces ClangSharpExtensions.[Record]MustBePassedByReference with ClangSharp.Pathogen.PathogenArrangedFunction, which uses Clang's code generation module to determine how a function call should be constructed. This moves Biohazrd's handling of whether a type is returned/passed by reference from TranslatedRecord to TranslatedFunction and TranslatedParameter.

This change does not include support for function pointers or vtable entries as those will require extra effort.
(VTables need to be reworked to fix #147 first. Function pointers don't support most ABI concerns in general #186.)

Fixes #35 (Fixed leaking return buffer semantics on global functions and static methods. Fixed as a side-effect.)
Contributes to #53 (Cleaning up ABI handling.)
Contributed to #108 (Linux support.)
Fixes #187 (Records with constructors not being returned by reference on Microsoft x64.)
Fixes #135 (Indirectly since this problematic method was obsoleted.)
PathogenDavid added a commit that referenced this issue Jun 21, 2021
@PathogenDavid
Reworked how function/method ABI concerns are handled to take advanta…
00b00aa 
…ge of Clang's code generation module.

In short, this change replaces ClangSharpExtensions.[Record]MustBePassedByReference with ClangSharp.Pathogen.PathogenArrangedFunction, which uses Clang's code generation module to determine how a function call should be constructed. This moves Biohazrd's handling of whether a type is returned/passed by reference from TranslatedRecord to TranslatedFunction and TranslatedParameter.

This change does not include support for function pointers or vtable entries as those will require extra effort.
(VTables need to be reworked to fix #147 first. Function pointers don't support most ABI concerns in general #186.)

Fixes #35 (Fixed leaking return buffer semantics on global functions and static methods. Fixed as a side-effect.)
Contributes to #53 (Cleaning up ABI handling.)
Contributes to #108 (Linux support.)
Fixes #187 (Records with constructors not being returned by reference on Microsoft x64.)
Fixes #135 (Indirectly since this problematic method was obsoleted.)
PathogenDavid added a commit that referenced this issue Jul 4, 2021
@PathogenDavid
Reworked how function/method ABI concerns are handled to take advanta…
b7425e7 
…ge of Clang's code generation module.

In short, this change replaces ClangSharpExtensions.[Record]MustBePassedByReference with ClangSharp.Pathogen.PathogenArrangedFunction, which uses Clang's code generation module to determine how a function call should be constructed. This moves Biohazrd's handling of whether a type is returned/passed by reference from TranslatedRecord to TranslatedFunction and TranslatedParameter.

This change does not include support for function pointers or vtable entries as those will require extra effort.
(VTables need to be reworked to fix #147 first. Function pointers don't support most ABI concerns in general #186.)

Fixes #35 (Fixed leaking return buffer semantics on global functions and static methods. Fixed as a side-effect.)
Contributes to #53 (Cleaning up ABI handling.)
Contributes to #108 (Linux support.)
Fixes #187 (Records with constructors not being returned by reference on Microsoft x64.)
Fixes #135 (Indirectly since this problematic method was obsoleted.)
@PathogenDavid PathogenDavid mentioned this issue Jul 4, 2021
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Translation Issues concerning the translation from libclang into Biohazrd Concept-Correctness Issues concerning the correctness of the translation from an ABI perspective
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@PathogenDavid