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: Redeclare static functions that have their address taken with used #441

Closed
wants to merge 4 commits into from

Conversation

ayrtonm
Copy link
Contributor

@ayrtonm ayrtonm commented Oct 11, 2024

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. Compiling with optimizations 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
Copy link
Contributor Author

ayrtonm commented Oct 11, 2024

Still need to add LIT annotations and a reference for the expected stdout to the new test.

…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
Copy link
Contributor Author

ayrtonm commented Oct 11, 2024

guess clang is more strict than gcc here. I could add a __used__ pointer referencing the function in IA2_DEFINE_WRAPPER but I'll see if there's a less silly solution.

@ayrtonm
Copy link
Contributor Author

ayrtonm commented Oct 11, 2024

I went with the __used__ pointer solution and the build looks good. I'm going to hold off on debugging this until I replace criterion with our test runner.

@ayrtonm
Copy link
Contributor Author

ayrtonm commented Oct 11, 2024

TODO: push the tests with the case mentioned in #414

@@ -48,7 +48,9 @@
#define IA2_CALL(opaque, id) opaque
#define IA2_CAST(func, ty) (ty) (void *) func
#else
#define IA2_DEFINE_WRAPPER(func) IA2_DEFINE_WRAPPER_##func
#define IA2_DEFINE_WRAPPER(func) \
__attribute__((__used__)) static void *keep_##func = (void *)func; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love adding a function pointer to data just to mark a function as used. We don't have any other way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't put much thought into this. There might be a way to get __attribute__((used)) typeof(func) func; before the function definition, but I'd need to think about how that would work with the types func's signature depends on.

@kkysen
Copy link
Contributor

kkysen commented Oct 13, 2024

Compiling with optimizations may get rid of the static functions unless they are marked with __attribute__((__used__)).

Would marking them with __attribute__((visibility("default")), which we need for #443, supersede marking them as used?

@ayrtonm ayrtonm closed this Oct 14, 2024
@ayrtonm
Copy link
Contributor Author

ayrtonm commented Oct 14, 2024

@rinon will handle this by checking that the macro expansion starts at the beginning of the function definition location instead. There's a test in this PR you might want to reuse, but it mainly covers the cases in #428 which is not super relevant here.

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.

Rewriter fails to emit used attribute for static fns when using a macro
3 participants