Skip to content

Allow linker flags to pass through to wasm-ld by default.#10747

Merged
sbc100 merged 1 commit intomasterfrom
lld_linker_flags
Mar 27, 2020
Merged

Allow linker flags to pass through to wasm-ld by default.#10747
sbc100 merged 1 commit intomasterfrom
lld_linker_flags

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 21, 2020

Rather then maintain an ever growing whitelist of supported linker
flags, instead list the linker flags that we want to explicitly ignore
for compatibility purposed with existing build systems.

@sbc100 sbc100 requested a review from kripken March 21, 2020 15:43
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, but we've gone back and forth on this and I don't have a deep understanding of real-world linker usage, so it does worry me.

cc @Beuc and @Brion who have large real-world build systems that it might be good to verify work with this.

emcc.py Outdated
if using_lld:
for flag, takes_arg in UNSUPPORTED_LLD_FLAGS.items():
if f.startswith(flag):
shared.warning('ignoring unsupported linker flag: `%s`', f)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this code was just moved around, but maybe this should integrate with the warning manager?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do that as part of the outstanding warning manager changes.

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 23, 2020

I imagine there will be more linker flags we need to add to this list, but it should be a finite about of work to get them added.

@sbc100 sbc100 force-pushed the lld_linker_flags branch 3 times, most recently from 44b2645 to 8a545de Compare March 23, 2020 19:16
@Beuc
Copy link
Contributor

Beuc commented Mar 24, 2020

At first glance I'm not sure of the impact.
RenPyWeb's build system is fully automated (emsdk_env.sh && make, basically), fully static too, you can check https://github.com/renpy/renpyweb

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 24, 2020

I seem to be getting unrelated build errors when trying to build renpyweb:

simd_none.lo  
../wrjpgcom.c:449:13: error: implicitly declaring library function 'strlen' with type 'unsigned long (const char *)'
      [-Werror,-Wimplicit-function-declaration]
        if (strlen(argv[argn]) + 2 >= (size_t) MAX_COM_LENGTH) {
            ^
../wrjpgcom.c:449:13: note: include the header <string.h> or explicitly provide a declaration for 'strlen'
../wrjpgcom.c:454:9: error: implicitly declaring library function 'strcpy' with type 'char *(char *, const char *)'
      [-Werror,-Wimplicit-function-declaration]
        strcpy(comment_arg, argv[argn]+1);
        ^
../wrjpgcom.c:454:9: note: include the header <string.h> or explicitly provide a declaration for 'strcpy'
../wrjpgcom.c:469:11: error: implicitly declaring library function 'strcat' with type 'char *(char *, const char *)'
      [-Werror,-Wimplicit-function-declaration]
          strcat(comment_arg, " ");
          ^

Seems to be releated to the NEED_BSD_STRINGS configure setting? .. but its unrelated to this change since I'm building with master still.

@sbc100 sbc100 force-pushed the lld_linker_flags branch from 8a545de to e87a69c Compare March 24, 2020 16:19
@Beuc
Copy link
Contributor

Beuc commented Mar 25, 2020

This patch seems to depend on others (since 1.39.11), it doesn't apply cleanly and I can't test, but for the record here's a matching log excerpt of ./scripts/libjpeg-turbo.sh in 1.39.11:

/bin/bash ./libtool --mode=link --tag=CC renpyweb/emsdk/upstream/emscripten/emcc  -O3   -o wrjpgcom  wrjpgcom.o libjpeg.la 
shared:WARNING: Assuming object file output in the absence of `-c`, based on output filename. Please add with `-c` or `-r` to avoid this warning
/bin/bash ./libtool --mode=link --tag=CC renpyweb/emsdk/upstream/emscripten/emcc  -O3   -o jcstest  jcstest.o libjpeg.la 
renpyweb/emsdk/upstream/emscripten/emcc -O3 -o wrjpgcom wrjpgcom.o  ./.libs/libjpeg.a
renpyweb/emsdk/upstream/emscripten/emcc -O3 -o jcstest jcstest.o  ./.libs/libjpeg.a

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 26, 2020

I'd like to go ahead and land this, and then add the blacklist as we get reports from the wild about flags that people want to be able to use (but are not supported by wasm-ld).

Rather then maintain an ever growing whitelist of supported linker
flags, instead list the linker flags that we want to explicitly ignore
for compatibility purposed with existing build systems.
@sbc100 sbc100 force-pushed the lld_linker_flags branch from e87a69c to 151dd6d Compare March 26, 2020 01:32
@sbc100 sbc100 merged commit 35746b5 into master Mar 27, 2020
@sbc100 sbc100 deleted the lld_linker_flags branch March 27, 2020 00:45
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.

3 participants