Skip to content

Keep shared libraries from calling main program's startup/cleanup procs on Linux #5173

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

Merged

Conversation

Feoramund
Copy link
Contributor

Fixes #5169

I've tested this and it works on my machine. I'm not sure if it needs to be done for any other procs, or if this is the ideal way to do it, but if not, hopefully it helps expand the understanding on the issue.

@Kelimion
Copy link
Member

That it's practically a one-liner is encouraging.

@Feoramund
Copy link
Contributor Author

Please if you could, make sure to test that this doesn't break anything on Windows as a result. I chose Protected visibility instead of Hidden, since it seemed more permissive and still worked. If it breaks Windows, it should be okay to just stuff it behind an architecture-based condition.

@Kelimion
Copy link
Member

With this patch applied, I get the following.

Windows:

Init from inside alpha.
Init from inside beta.
Init from the main program.
Another init from the main program.
32
16

Linux:

Init from inside beta.
Init from inside alpha.
Init from the main program.
Another init from the main program.
32
16

I don't know why, but on Linux it consistently prints beta before alpha, and on Windows it's consistently alpha, then beta. But it's progress.

@Feoramund
Copy link
Contributor Author

Interesting. I just noticed the order is actually different too as you pointed out, so it's technically not mirroring Windows.

I would consider this a bug in its own right, as I can imagine people may need to rely on certain linking order behaviors in the future, especially if init procs are involved.

@Kelimion
Copy link
Member

That may be down to the difference between msvc and clang's linkers, who knows. Would have to be looked into. Other than that the fix looks fine to me, but let's let Bill take a gander first.

@laytan
Copy link
Collaborator

laytan commented May 18, 2025

Looks similar to a patch I did recently that was having the same symptoms with other procs and globals being used from other modules: 1053ec3

Weird that doesn't apply it to these procs too.

@Feoramund
Copy link
Contributor Author

Feoramund commented May 18, 2025

It looks like the startup/cleanup procs are never seen by either lb_find_ident or lb_find_value_from_entity, which are the two ways you get to lb_set_entity_from_other_modules_linkage_correctly.

It looks like they are getting skipped on lb_build_expr_internal due to them being constant values, so we don't get to case_ast_node(i, Ident, expr) which could lead to a call to lb_find_ident.

Is this possibly where another check for it should be?

@Feoramund Feoramund force-pushed the fix-linux-shared-lib-runtime-call branch from 4d2b6e0 to 713360a Compare May 22, 2025 13:40
@Feoramund
Copy link
Contributor Author

I updated the patch to use the same visibility/linkage as lb_correct_entity_linkage.

@gingerBill gingerBill merged commit 34e998c into odin-lang:master May 22, 2025
7 checks passed
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.

Odin shared libraries call the main program's _startup_runtime on Linux
4 participants