Use -lxxx internally rather than full library path
#14342
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbc100
force-pushed
the
wasm_ld_lib_resolve
branch
from
3a7c7b6 to
aac5d8c
Compare
dschuff
approved these changes
Jun 2, 2021
kripken
reviewed
Jun 2, 2021
emcc.py
Outdated
| @@ -462,7 +462,7 @@ def is_supported(f): | |||
| # lld allows various flags to have either a single -foo or double --foo | |||
| if f.startswith(flag) or f.startswith('-' + flag): | |||
| diagnostics.warning('linkflags', 'ignoring unsupported linker flag: `%s`', f) | |||
| return False, takes_arg | |||
| return False, takes_arg and '=' not in f | |||
There was a problem hiding this comment.
I don't follow this. Perhaps a comment about why = is special?
sbc100
force-pushed
the
wasm_ld_lib_resolve
branch
5 times, most recently
from
a7d01cd to
1581b6e
Compare
kripken
reviewed
Jun 2, 2021
emcc.py
Outdated
Comment on lines
465
to
467
| # Skip the next argument if this linker flag takes and argument and that | ||
| # argument was not specified as a separately (i.e. it was specified as | ||
| # single arg containing an `=` char.) |
There was a problem hiding this comment.
Suggested change
| # Skip the next argument if this linker flag takes and argument and that | |
| # argument was not specified as a separately (i.e. it was specified as | |
| # single arg containing an `=` char.) | |
| # Skip the next argument if this linker flag takes an argument and that | |
| # argument was not specified separately (i.e. it was specified as | |
| # single arg containing an `=` char.) |
Is that right?
If so then this makes sense to me. But then isn't this a separate fix from the PR?
There was a problem hiding this comment.
Ah yes, this is a bug fix, but it also happens to be needed as part of this change. I split it out as #14356.
The reason this change depends on it is that other wise test_supported_linker_flags will start failing. The reason this change depends on that fix goes like this:
- We specify the
-L/path/to/sysrootas the first system link flag returned byemsdk_ldflags - System link flags get added to user link flags
- If, as in that test, that last user link flag is an ignored one of the form
-Wl,-rpath=foothen the next argument will be ignored.. which will be the-L/path/to/sysroot - This will cause
-lcto not be found, but that didn't matter previously because we were using full paths the libc.a prior to this change.
sbc100
force-pushed
the
wasm_ld_lib_resolve
branch
2 times, most recently
from
8cdcef8 to
aa6e451
Compare
kripken
approved these changes
Jun 3, 2021
Since we already supply the correct `-L` path we can rely on wasm-ld to find, for example, libc.a by simply pasing `-lc` rather than its full path. This is in line with what gcc and clang do. This is almost NFC, except in that case where the user supplies and library path via `-L` which happens to contains a file called `libc.a`. In this case, as with clang or gcc, that file will be chosen over the system one. This makes the linker command lines shorter, more readable, more inline with other compilers.
sbc100
force-pushed
the
wasm_ld_lib_resolve
branch
from
aa6e451 to
fd0f829
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since we already supply the correct
-Lpath we can rely on wasm-ld tofind, for example,
libc.aby simply passing-lcrather than its fullpath.
This is in line with what gcc and clang do.
This is almost NFC, except in that case where the user supplies a
library path via
-Lwhich happens to contain a system library(e.g.
libc.a). In this case, as with clang or gcc, that file will bechosen over the system one.
This makes the linker command lines shorter, more readable, more inline
with other compilers.