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.

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

Ignore -fPIC flag when not building relocatable code #9710

Merged
merged 1 commit into from
Oct 25, 2019
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 25, 2019

This issue was supposed to have been fixed in lld but it seems that
issues remain. See https://reviews.llvm.org/D65922.

Fixes: #9690, #9013

This issue was supposed to have been fixed in lld but it seems that
issues remain.  See https://reviews.llvm.org/D65922.

Fixes: #9690, #9013
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 25, 2019

PTAL

def test_fpic_static(self):
self.emcc_args.append('-fPIC')
self.emcc_args.remove('-Werror')
self.do_run_in_out_file_test('tests', 'core', 'test_hello_world')
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is enough to hit the bug, at least not when i tested myself. I had to build tests/hello_libcxx.cpp which I guess includes enough libc++ to see breakage.

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 test fails withou the corresponding change to strip the -fPIC. I just reconfirmed. There might be multiple bugs but the emcc.py change makes this test start passing.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, sounds good on the test then.

@@ -1883,6 +1883,9 @@ def is_link_flag(flag):
return any(flag.startswith(x) for x in ('-l', '-L', '-Wl,'))

compile_args = [a for a in newargs if a and not is_link_flag(a)]
if '-fPIC' in compile_args and not shared.Settings.RELOCATABLE:
shared.warning('ignoring -fPIC flag when not building with SIDE_MODULE or MAIN_MODULE')
compile_args.remove('-fPIC')
Copy link
Member

Choose a reason for hiding this comment

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

You explained this to me but I realize I still don't get it, sorry. What happens in this case:

emcc a.c -fPIC -c
emcc a.o -s SIDE_MODULE

It seems like the first command will lose the -fPIC, so it will try to link a non-PIC into a SIDE_MODULE. I think you said that errors? Would the only way to avoid the error be to pass SIDE_MODULE at compile time too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and yes.

For now, until we fix the underlying issue with static linking of PIC objects, then we can allow -fPIC to control this again.

Copy link
Member

Choose a reason for hiding this comment

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

Sgtm, but (separately from this PR if you want) please update the docs to mention that with the new backend, MAIN/SIDE_MODULES must have their source files built with MAIN/SIDE_MODULE. E.g. here in the list of big backend differences, https://emscripten.org/docs/compiling/WebAssembly.html#backends

@sbc100 sbc100 merged commit fe35baf into incoming Oct 25, 2019
@delete-merged-branch delete-merged-branch bot deleted the ignore_fpic branch October 25, 2019 22:30
@kripken
Copy link
Member

kripken commented Oct 25, 2019

Please add this to the docs (that with the new backend, one must add flags a compile time too). And also this is a breaking change for some users (as -fPIC used to work at compil;e time, and now wont, for SIDE/MAIN_MODULE users), please mention that in the changelog.

belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
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.

Issues with -fPIC in upstream SDK
2 participants