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

Fix host linker options to have -Bsymbolic #34534

Merged
merged 2 commits into from
Apr 5, 2020

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Apr 4, 2020

Looks like this got lost after #33716

See dotnet/installer#7005

@elinor-fung
Copy link
Member Author

cc @mmitche

The last command per the repro in dotnet/installer#7005 (comment) no longer crashes after patching the libhostpolicy that gets restored to the .dotnet folder

# This is required to map a symbol reference to a matching definition local to the module (.so)
# containing the reference instead of using definitions from other modules.
if(CLR_CMAKE_TARGET_LINUX)
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Xlinker -Bsymbolic -Bsymbolic-functions")
Copy link
Member

Choose a reason for hiding this comment

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

I can see that we had it this way before, but the second option is effectively ignored. -Xlinker can be followed by only one linker option (in fact, if that linker option had a parameter, it would have to be prefixed by the -Xlinker too). So the -Bsymbolic-function actually sets a compiler option -B which adds symbolic-function to the libraries search path.
However, -Bsymbolic is stronger than -Bsymbolic-functions, so -BSymbolic alone is sufficient and there was no problem before. I'd suggest just removing the -Bsymbolic-functions here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, wow. Yeah, definitely just copied the options from before. Removed -Bsymbolic-functions.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like we are using -Bsymbolic-functions in other places as well, can we extract it out in /eng/native/configurecompiler.cmake as a cleanup?

$ git grep Xlinker

src/coreclr/src/dlls/dbgshim/CMakeLists.txt:    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Xlinker -Bsymbolic -Xlinker -Bsymbolic-functions")
src/coreclr/src/dlls/mscordac/CMakeLists.txt:    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Xlinker -Bsymbolic -Xlinker -Bsymbolic-functions")
src/coreclr/src/dlls/mscordbi/CMakeLists.txt:    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Xlinker -Bsymbolic -Xlinker -Bsymbolic-functions")
src/coreclr/src/ilasm/CMakeLists.txt:    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Xlinker -Bsymbolic -Xlinker -Bsymbolic-functions")
src/coreclr/src/ildasm/exe/CMakeLists.txt:    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Xlinker -Bsymbolic -Xlinker -Bsymbolic-functions")
src/coreclr/src/jit/CMakeLists.txt:    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Xlinker -Bsymbolic -Bsymbolic-functions")
src/coreclr/src/pal/src/libunwind/aux_/ltmain.sh:      -Xlinker)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I noticed those (and them also having -Bsymbolic-function). I agree we should consolidate and clean them up. I wanted to keep this change small and targeted to just unblocking being able to insert into dotnet/installer.

Copy link
Member

Choose a reason for hiding this comment

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

Let's get in this simple focused fix and do a cleanup for the -Bsymbolic-functions after.
@am11 I'd prefer keeping this option at the specific libraries and not make it global. I've seen several times in the past a case when someone has used a LD_PRELOAD with a helper library as a workround for a problem in a specific function in libcoreclr.so.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit a9ce822 into dotnet:master Apr 5, 2020
@sfoslund
Copy link
Member

sfoslund commented Apr 5, 2020

@janvorli @mmitche it looks like this will have to be ported to preview 3 to fix dotnet/installer#7005

@mmitche
Copy link
Member

mmitche commented Apr 6, 2020

OKay will do.

mmitche pushed a commit to mmitche/runtime that referenced this pull request Apr 6, 2020
* Fix host linker options to have -Bsymbolic

* Remove -Bsymbolic-functions
mmitche added a commit that referenced this pull request Apr 6, 2020
* Fix host linker options to have -Bsymbolic

* Remove -Bsymbolic-functions

Co-authored-by: Elinor Fung <47805090+elinor-fung@users.noreply.github.com>
@elinor-fung elinor-fung deleted the hostLinkSymbolic branch April 6, 2020 20:31
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants