Skip to content

Don't compile dynamic linking code into non-PIC version of libc. NFC #18638

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
Merged
merged 1 commit into from
Jan 31, 2023
Merged

Don't compile dynamic linking code into non-PIC version of libc. NFC #18638

merged 1 commit into from
Jan 31, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 31, 2023

Replaces #18637

@sbc100 sbc100 changed the title Don't compiler dynamic linking code into non-PIC version of libc. NFC Don't compile dynamic linking code into non-PIC version of libc. NFC Jan 31, 2023
@sbc100 sbc100 requested a review from tlively January 31, 2023 06:46
@sbc100 sbc100 mentioned this pull request Jan 31, 2023
Closed
tlively
tlively approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM although I do not understand this deep __deps magic.

src/library_dylink.js
@@ -240,12 +240,18 @@ var LibraryDylink = {
_emscripten_dlopen_js__deps: [function() { error(dlopenMissingError); }],
_dlsym_js__deps: [function() { error(dlopenMissingError); }],
_dlsym_catchup_js__deps: [function() { error(dlopenMissingError); }],
dlopen__deps: [function() { error(dlopenMissingError); }],
_emscripten_dlopen__deps: [function() { error(dlopenMissingError); }],
__dlsym__deps: [function() { error(dlopenMissingError); }],
Copy link
Member

Choose a reason for hiding this comment

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

What does a function expression in __deps mean/do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This magic basically means that users see errors at build time when they use those symbols.

@sbc100 sbc100 merged commit 008017b into main Jan 31, 2023
@sbc100 sbc100 deleted the skip_dynlink_c branch January 31, 2023 17:45
sbc100 added a commit that referenced this pull request Mar 4, 2023
@sbc100
Add dladdr to list of dummy dlfnc symbols
ec5b250 
This is needed as of #18638 since dynlink.c is not longer included
in the build when MAIN_MODULE is not set.

Fixes: #1195
@sbc100 sbc100 mentioned this pull request Mar 4, 2023
Merged
sbc100 added a commit that referenced this pull request Mar 4, 2023
@sbc100
Add dladdr to list of dummy dlfnc symbols
8395216 
This is needed as of #18638 since dynlink.c is not longer included
in the build when MAIN_MODULE is not set.

Fixes: #1195
sbc100 added a commit that referenced this pull request Mar 6, 2023
@sbc100
Add dladdr to list of dummy dlfnc symbols (#18896)
3bdf783 
This is needed as of #18638 since dynlink.c is not longer included
in the build when MAIN_MODULE is not set.

Fixes: #1195
sbc100 added a commit that referenced this pull request Mar 9, 2023
@sbc100
Remove unused stubs from library_dylink.js. NFC
c64423b 
Since #18638 its no longer possible to generated undefined references
to the internal (_js-suffixed) versions of these symbols because
the `dynlink.c` file is simply not compiled it.

This should really have been part of #18638.
@sbc100 sbc100 mentioned this pull request Mar 9, 2023
Closed
impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023
@sbc100 @impact-maker
Add dladdr to list of dummy dlfnc symbols (emscripten-core#18896)
f40cc98 
This is needed as of emscripten-core#18638 since dynlink.c is not longer included
in the build when MAIN_MODULE is not set.

Fixes: emscripten-core#1195
impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023
@sbc100 @impact-maker
Add dladdr to list of dummy dlfnc symbols (emscripten-core#18896)
fb72111 
This is needed as of emscripten-core#18638 since dynlink.c is not longer included
in the build when MAIN_MODULE is not set.

Fixes: emscripten-core#1195
juj added a commit to juj/emscripten that referenced this pull request May 22, 2023
@juj
Fix regression between 3.1.8 -> 3.1.38 where overriding dlopen() with…
74f61aa 
… a gracefully failing version broke, and add a test to verify that end user code overriding dlopen() cannot regress again. Possibly regressed by emscripten-core#18638.
@juj juj mentioned this pull request May 22, 2023
Closed
juj added a commit to juj/emscripten that referenced this pull request May 22, 2023
@juj
Fix regression between 3.1.8 -> 3.1.38 where overriding dlopen() with…
2ab054e 
… a gracefully failing version broke, and add a test to verify that end user code overriding dlopen() cannot regress again. Possibly regressed by emscripten-core#18638.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlively tlively tlively approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbc100 @tlively