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

runtime, tests: require -Werror=strict-prototypes so every function has a prototype #483

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Dec 16, 2024

In order to have C callgate wrappers, we need to know function prototypes, but ReturnType foo() functions have no prototype, as foo(void) is required.

ABI determination (and soon to be API, too) is done eagerly for all functions, so this errors when a function has no prototype. We can later relax this to only requiring prototypes on functions with callgates.

Furthermore, often projects enable -Werror=strict-prototypes themselves, like dav1d. Turning this on here for the IA2 runtime and tests means that we don't have to disable it in other projects like dav1d.

@kkysen kkysen requested a review from ayrtonm December 16, 2024 13:47
@kkysen kkysen force-pushed the kkysen/strict-prototypes branch 2 times, most recently from 90727a5 to 04100a6 Compare December 16, 2024 15:05
@ayrtonm
Copy link
Contributor

ayrtonm commented Dec 16, 2024

The change itself looks fine though I don't understand why generating C call gates requires this flag across the codebase rather than just the call gate source file. I'm also not sure how you planned on getting type declarations for C call gates so I might be missing something.

@kkysen
Copy link
Contributor Author

kkysen commented Dec 17, 2024

The change itself looks fine though I don't understand why generating C call gates requires this flag across the codebase rather than just the call gate source file.

It doesn't; it only requires them for the call gate sources as you said, but it seems a lot simpler to enforce this way, and seems like a better default in general, so I just did it everywhere.

I'm also not sure how you planned on getting type declarations for C call gates so I might be missing something.

I'm getting them from the clang::FunctionProtoType. Basically adding it in parallel to the AbiSignature information but storing API type information. Still trying to figure out how to set everything up so the types used are visible where the function is declared (I'm thinking an approach like defining them in the same file as the wrapped function like you did in #451 would likely work best).

@ayrtonm
Copy link
Contributor

ayrtonm commented Dec 17, 2024

seems like a better default in general, so I just did it everywhere.

I think the fewer requirements we have in terms of compiler flags the better

still trying to figure out how to set everything up so the types used are visible where the function is declared

yeah that's the main issue I was asking about

@kkysen
Copy link
Contributor Author

kkysen commented Dec 17, 2024

How are we determining the ABI for untyped f() functions in the first place?

@ayrtonm
Copy link
Contributor

ayrtonm commented Dec 17, 2024

We currently treat fn() like fn(void). I understand that isn't always correct since the caller can pass in arguments but I'd say that handling this properly isn't a priority right now.

This is done by pushing the param regs onto the stack in the prologue
(if there's a post condition function), and then
popping them before we call the post condition function.

Next we'll work on supporting more than the 6 param regs,
as well as passing the return value.
…ich isn't supported yet

This is a fatal error (ICE) since the caller can just set `--no-enable-dav1d_get_picture-post-condition`.
… has a prototype

In order to have C callgate wrappers, we need to know function prototypes,
but `ReturnType foo()` functions have no prototype, as `foo(void)` is required.

ABI determination (and soon to be API, too) is done eagerly for all functions,
so this errors when a function has no prototype.
We can later relax this to only requiring prototypes on functions with callgates.

Furthermore, often projects enable `-Werror=strict-prototypes` themselves, like `dav1d`.
Turning this on here for the IA2 runtime and tests means that
we don't have to disable it in other projects like `dav1d`.
@kkysen kkysen force-pushed the kkysen/call-gate-post-condition-6-param-regs branch from 1dcaf11 to 5eaa9bc Compare December 18, 2024 18:50
@kkysen kkysen force-pushed the kkysen/strict-prototypes branch from 04100a6 to 657aafb Compare December 18, 2024 18:50
Base automatically changed from kkysen/call-gate-post-condition-6-param-regs to main December 18, 2024 21:32
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