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

Drop explicitly passed -lc unless -nodefaultlibs or similar is passed #22765

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Drop explicitly passed -lc unless -nodefaultlibs or similar is passed #22765

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3414463
Drop explicitly passed -lc unless -nodefaultlibs or similar is passed
hoodmane Oct 18, 2024
a4b1f72
Update test, add warning
hoodmane Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15169,3 +15169,6 @@ def test_fp16(self, opts):

def test_embool(self):
self.do_other_test('test_embool.c')

def test_user_passed_lc(self):
self.run_process([EMCC, '-lc', '-sDISABLE_EXCEPTION_CATCHING=0', '-sMIN_SAFARI_VERSION=150000'])
hoodmane marked this conversation as resolved.
Show resolved Hide resolved
16 changes: 12 additions & 4 deletions tools/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2749,7 +2749,7 @@ def map_to_js_libs(library_name):
return None


def process_libraries(state, linker_inputs):
def process_libraries(state, linker_inputs, nodefaultlibc):
new_flags = []
libraries = []
suffixes = STATICLIB_ENDINGS + DYNAMICLIB_ENDINGS
Expand All @@ -2761,6 +2761,12 @@ def process_libraries(state, linker_inputs):
new_flags.append((i, flag))
continue
lib = removeprefix(flag, '-l')
if lib == 'c' and not nodefaultlibc:
# Drop explicitly passed -lc unless they also passed -nodefaultlibs or
# similar. It's important to link libc after our other injected system
# libraries like libbulkmemory, but user linked libraries go ahead of
# system libraries, so if the user passes `-lc` then we can get crashes.
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I agree this fixed the program at hand, it also seems a little to magical. What if user explictly wants to do something like this:

-lmylib_prelibc -lc -lmylib_postlibc

In this case removing the explicit -lc provided by the user seems wrong/non-intuitive.

Perhaps it would be better to fix rust not to pass the explicit -lc, do you know why rust insists on doing this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we should at least warn the user when we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we should at least warn the user when we do this?

I suppose so. But passing -lc at all is also very likely to be wrong if it's likely to be depending on other libraries that are automatically injected at the very end to work correctly. I guess the warning could say to pass -nolibc if they need to link libc in a particular order though -nolibc seems to also disable linking of libmalloc and libnoexit if they should be added.

What about the option of injecting the system libs ahead of the user object files? Do you think that would have any negative consequences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it would be better to fix rust not to pass the explicit -lc, do you know why rust insists on doing this?

Well it seems to work for them with all of the other linker they use. I'm trying to fix it from rust's side too, but it seems to me like it's the linker's responsibility to correctly handle this and not the consumer's responsibility not to pass it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any downside to remove it that you know of? Why is it even there?

Copy link
Contributor Author

@hoodmane hoodmane Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, the workaround on the rust side is pretty short nad has no downsides I know of. It was rejected two years ago when I tried to merge the fix to rust, but I opened a new PR with the same fix and maybe it will be accepted this time. I think their response was roughly "if it breaks to pass -lc maybe it breaks when you pass all sorts of things, we need to figure out what the actual cause is and implement a real fix".

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose so. But passing -lc at all is also very likely to be wrong if it's likely to be depending on other libraries that are automatically injected at the very end to work correctly. I guess the warning could say to pass -nolibc if they need to link libc in a particular order though -nolibc seems to also disable linking of libmalloc and libnoexit if they should be added.

Yes, anyone of passes -lc in emscripten is likely going to have a hard time because they would need to also add -lmalloc and -lnoexit and maybe -lbulkmemory. i.e. in emscripten libc is split up into multiple libraryies which are order dependent.

So I think that removing -lc is likely the right thing to do but I also worry that doing it silently could break some advanced users who are passing all of those libraries. i.e. it will fix rust but perhaps break some other use case.

Another solution would be to merge all out libc libraries into a single one but then we would end up with even more variants, e.g. libc-noexit-bulkmemory-debug-mt.a. This complexity would be largely hidden, but it would mean would have to build probably over 100 different libc versions :-/ Having separate libraries allows us to avoid that.

What about the option of injecting the system libs ahead of the user object files? Do you think that would have any negative consequences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading the Rust codebase, I am not at all sure where it gets decided that it should pass -lc in the first place, but it's easy to filter it out as it's constructing the linker invocation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"if it breaks to pass -lc maybe it breaks when you pass all sorts of things, we need to figure out what the actual cause is and implement a real fix".

I guess now that we know more about the problem we can make a better argument for removing it. We can say "supplying -lc on its own just doesn't work with emscripten and will break the build". Also, since the compiler always includes libc by default this flag is redundant and pointless. Removing it is simply better for everyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I'd just really like either Emscripten or Rust to merge a fix for this, I'll see what comments I get on the Rust PR.


logger.debug('looking for library "%s"', lib)

Expand Down Expand Up @@ -3026,12 +3032,12 @@ def package_files(options, target):


@ToolchainProfiler.profile_block('calculate linker inputs')
def phase_calculate_linker_inputs(options, state, linker_inputs):
def phase_calculate_linker_inputs(options, state, linker_inputs, nodefaultlibc):
using_lld = not (options.oformat == OFormat.OBJECT and settings.LTO)
state.link_flags = filter_link_flags(state.link_flags, using_lld)

# Decide what we will link
process_libraries(state, linker_inputs)
process_libraries(state, linker_inputs, nodefaultlibc)

# Interleave the linker inputs with the linker flags while maintainging their
# relative order on the command line (both of these list are pairs, with the
Expand Down Expand Up @@ -3072,8 +3078,10 @@ def run(linker_inputs, options, state, newargs):

target, wasm_target = phase_linker_setup(options, state, newargs)

nodefaultlibc = '-nodefaultlibs' in newargs or '-nolibc' in newargs or '-nostdlib' in newargs

# Link object files using wasm-ld or llvm-link (for bitcode linking)
linker_arguments = phase_calculate_linker_inputs(options, state, linker_inputs)
linker_arguments = phase_calculate_linker_inputs(options, state, linker_inputs, nodefaultlibc)

# Embed and preload files
if len(options.preload_files) or len(options.embed_files):
Expand Down
Loading