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

rewriter: static fns with the same name result in duplicate extern IA2_fn_ptr_* __ia2_* declarations #428

Open
kkysen opened this issue Oct 7, 2024 · 8 comments

Comments

@kkysen
Copy link
Contributor

kkysen commented Oct 7, 2024

When there are multiple static functions with the same name, they result in duplicate extern IA2_fn_ptr_* __ia2_* declarations in callgate_wrapper.h. I'm not certain this an error, since other errors are in the way first, but it seems like an error.

For example, in dav1d, this happens with init_internal, which is defined in lib.c and scan.c and used when calling pthread_once.

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 7, 2024

We took the issue of call gates for static functions with the same name into account in a few places but ultimately decided to declare them all in the output header which prevents them. This was just to minimize the number of build artifacts and simplify the build process, but I think there's multiple ways we might be able to handle this.

I'm not certain this an error, since other errors are in the way first, but it seems like an error. For example, in dav1d, this happens with init_internal, which is defined in lib.c and scan.c and used when calling pthread_once.

If both these files define a static init_internal function and take their address (must happen in both .c's) then it's an issue we need to address.

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 7, 2024

-Wl,-zunique-symbol might come in handy here if call gate names cannot be local.

@kkysen
Copy link
Contributor Author

kkysen commented Oct 7, 2024

If both these files define a static init_internal function and take their address (must happen in both .c's) then it's an issue we need to address.

Yup, we do. They're both passed to pthread_once.

@ayrtonm ayrtonm self-assigned this Oct 7, 2024
@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 9, 2024

I think the easiest thing to handle the most common cases of this would be to assert that duplicate static functions have the same function signature and generate only one declaration in the header.

@kkysen
Copy link
Contributor Author

kkysen commented Oct 9, 2024

I think the easiest thing to handle the most common cases of this would be to assert that duplicate static functions have the same function signature and generate only one declaration in the header.

Yeah that would improve things. Move the error to the rewriter, and if the functions are the same, then just remove duplicates.

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 11, 2024

My tests for this cover static functions defined in headers, duplicate definitions of static functions in source files and different definitions of static functions with the same name and function signature in source files. @kkysen any other cases I should cover?

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 11, 2024

oh I just realized the duplicate declarations don't actually cause an error and if the function signatures don't match it'll trigger a compile error. Since the generated header isn't considered developer-facing this is much lower priority than I thought. I'll push the tests I wrote and the change I made for #414 (it's needed for static functions defined in headers) but I don't think it's worth spending time on moving the errors to the rewriter for now.

ayrtonm added a commit that referenced this issue Oct 11, 2024
…th __used__

When we make call gates for static functions that have their address taken, the
static function is only referenced from the asm for the call gate. When
compiling with optimizations dead code elimination may get rid of the static
functions unless they are marked with __attribute__((__used__)). The rewriter
previously prepended this attribute in this case to avoid that. This commit
removes the prepended attribute and instead redeclares these functions with that
attribute. Closes #414 and fixes a test added for #428.
ayrtonm added a commit that referenced this issue Oct 11, 2024
…th __used__

When we make call gates for static functions that have their address taken, the
static function is only referenced from the asm for the call gate. When
compiling with optimizations dead code elimination may get rid of the static
functions unless they are marked with __attribute__((__used__)). The rewriter
previously prepended this attribute in this case to avoid that. This commit
removes the prepended attribute and instead redeclares these functions with that
attribute. Closes #414 and fixes a test added for #428.
ayrtonm added a commit that referenced this issue Oct 11, 2024
…th __used__

When we make call gates for static functions that have their address taken, the
static function is only referenced from the asm for the call gate. When
compiling with optimizations dead code elimination may get rid of the static
functions unless they are marked with __attribute__((__used__)). The rewriter
previously prepended this attribute in this case to avoid that. This commit
removes the prepended attribute and instead redeclares these functions with that
attribute. Closes #414 and fixes a test added for #428.
@kkysen
Copy link
Contributor Author

kkysen commented Oct 13, 2024

My tests for this cover static functions defined in headers, duplicate definitions of static functions in source files and different definitions of static functions with the same name and function signature in source files. @kkysen any other cases I should cover?

That sounds like all of them to me.

oh I just realized the duplicate declarations don't actually cause an error and if the function signatures don't match it'll trigger a compile error. Since the generated header isn't considered developer-facing this is much lower priority than I thought. I'll push the tests I wrote and the change I made for #414 (it's needed for static functions defined in headers) but I don't think it's worth spending time on moving the errors to the rewriter for now.

Is the only solution for now to just IA2_IGNORE them? And be lucky that they don't actually cross compartment boundaries?

@ayrtonm ayrtonm removed their assignment Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants