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.

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

dynamic linking: Weakly defined functions are not handled correctly #13773

Closed
sbc100 opened this issue Mar 25, 2021 · 2 comments · Fixed by #14921
Closed

dynamic linking: Weakly defined functions are not handled correctly #13773

sbc100 opened this issue Mar 25, 2021 · 2 comments · Fixed by #14921

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Mar 25, 2021

If that same symbol is weakly defined in two different dynamic modules we could expect the runtime dynamic linker to pick just one.

Sadly, this doesn't seem to work today and the each module will end up calling its own local function.

Somewhat similar to #12819.
sbc100 added a commit that referenced this issue Mar 26, 2021
@sbc100
Add dylink test to show weakly defined function issue. NFC (#13774)
9928b1d 
Currently we don't support weakly defined functions that
exist in both the main and the side module.

See #13773
@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 29, 2021

It looks like function addresses and data addresses are already correctly handled, its only direct calls that seem to be a problem.

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 13, 2021

WIP fix on the llvm side: https://reviews.llvm.org/D108060

sbc100 added a commit that referenced this issue Aug 20, 2021
@sbc100
Fix test_dylink_spaghetti
8690c42 
This test was inadvertently relying on an bug in llvm which was recently
fixed: #13773

In this test the class `Class` is defined in both the main and the side
module.  If you run this code as is on the desktop the version of
`Class` defined in `main.cpp` will always win the `side init` will never
get printed.

The fix is to give the class in the side module a different name (and
alternative would be to make it visitibility hidden or static).

This change should allow the llvm fix to roll in:
https://reviews.llvm.org/D108413
sbc100 added a commit that referenced this issue Aug 20, 2021
@sbc100
Fix test_dylink_spaghetti
99a74ef 
This test was inadvertently relying on an bug in llvm which was recently
fixed: #13773

In this test the class `Class` is defined in both the main and the side
module.  If you run this code as is on the desktop the version of
`Class` defined in `main.cpp` will always win the `side init` will never
get printed.

The fix is to give the class in the side module a different name (and
alternative would be to put it in an anonymous namespace).

This change should allow the llvm fix to roll in:
https://reviews.llvm.org/D108413
@sbc100 sbc100 mentioned this issue Aug 20, 2021
Merged
sbc100 added a commit that referenced this issue Aug 20, 2021
@sbc100
Fix test_dylink_spaghetti (#14921)
110b4e9 
This test was inadvertently relying on an bug in llvm which was recently
fixed: #13773

In this test the class `Class` is defined in both the main and the side
module.  If you run this code as is on the desktop the version of
`Class` defined in `main.cpp` will always win the `side init` will never
get printed.

The fix is to give the class in the side module a different name (and
alternative would be to put it in an anonymous namespace).

This change should allow the llvm fix to roll in:
https://reviews.llvm.org/D108413
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix test_dylink_spaghetti emscripten-core/emscripten
1 participant
@sbc100