Skip to content

Conversation

@davezarzycki
Copy link
Contributor

The '-Wl,-z,defs' flag is useful, so let's disable it only where needed.

The '-Wl,-z,defs' flag is useful, so let's disable it only where needed.
@davezarzycki
Copy link
Contributor Author

@swift-ci please smoke test

@davezarzycki davezarzycki merged commit 5015ee1 into swiftlang:master Nov 15, 2017
@davezarzycki davezarzycki deleted the cmake_localize_hack2 branch November 15, 2017 15:49
# into swift. The use of this flag pollutes all targets, and we are not able
# to remove it on a per-target basis which breaks cross-compilation.
string(REGEX REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
if("${CMAKE_SYSTEM_NAME}" STREQUAL "Windows")
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. This flag only comes in cross compiling. The CMAKE_SYSTEM_NAME is UNIX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to refine this. What is the right way to check for Windows in this case?

Copy link
Member

Choose a reason for hiding this comment

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

You cannot at this level, hence I was suggesting that we sink this further. Pushing this into the add_library call would be sufficiently low that we can check the target for the library being built.

Copy link
Contributor Author

@davezarzycki davezarzycki Nov 16, 2017

Choose a reason for hiding this comment

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

Is it really necessary on Windows to modify add_library? What specific Swift binaries need this flag removed? For example, on Linux only SwiftRemoteMirror "needs" the flag removed, and after I looked into the scenario, I'm convinced that -Wl,-z,defs is correctly identifying a bug, so removing the flag only makes the problem worse, not better.

Rather than add infrastructure to formalize the removal of -Wl,-z,defs, it seems better/wiser to contain the problem to the affected libraries until they can be cleaned up.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it really is necessary to modify add_library. The point is that we are sharing the flags incorrectly. The flag is for build not for host when cross-compiling. Im compiling a compiler for Linux and the stdlib and tests for Windows. The flags are not valid for the host, but are valid for the build. It is any library that is going to run on the host that needs the alteration. This would include things like the stdlib, the runtime, SwiftDemangle, SwiftRemoteMirror, etc.

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.

2 participants