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

Use linker tools on OSX & Linux to limit exports (#4651) #5659

Merged
merged 12 commits into from
Jan 21, 2021

Conversation

steven-johnson
Copy link
Contributor

As a way to address #4651, but as a hopefully-simpler alternative to #5655, this adds linker-specific scripts for Linux and OSX builds that exports only the Halide:: and halide_ symbols in our shared libraries, hiding everything else (most notable, everything LLVM).

It doesn't attempt to do the same for MSVC builds (yet). Even without that, though, this is arguably strictly better than what we had before.

Marking as "draft" for now because the CMake scripting is awful but I didn't want to wait to figure out the right CMake syntax before trying this out. And also, some ugly TODOs and stuff.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@steven-johnson
Copy link
Contributor Author

Feel free to push your corrections directly to this PR

@alexreinking
Copy link
Member

Feel free to push your corrections directly to this PR

That's the plan 🙂

@abadams
Copy link
Member

abadams commented Jan 21, 2021

The linker scripts are much less arcane than I feared

@alexreinking
Copy link
Member

Okay, I have tested my CMake changes on Linux with GCC and the following linkers: GNU LD, GOLD (via -fuse-ld=gold), and LLD (via -fuse-ld=lld). Changing the linker is a toolchain-level change and so requires a clean rebuild to re-run the check.

Makefile Show resolved Hide resolved
@steven-johnson steven-johnson marked this pull request as ready for review January 21, 2021 18:50
@steven-johnson
Copy link
Contributor Author

Looks good! Just needs reviewer approval.

@LebedevRI
Copy link
Contributor

The caveat with this approach alone as opposed to #5655 is that while it resolves the issue for the final library,
during the compile time however, the functions aren't known to be hidden, which may be suboptimal.

@alexreinking
Copy link
Member

@LebedevRI - IIRC the issue was that we consume standard library types through our API which makes the situation on Windows a lot harder. For instance, I don't think you can export a class that takes in a std::vector.

@LebedevRI
Copy link
Contributor

@LebedevRI - IIRC the issue was that we consume standard library types through our API which makes the situation on Windows a lot harder. For instance, I don't think you can export a class that takes in a std::vector.

I don't know the windows situation, but i would like to note that even with the current linker script approach,
it is not done for windows, only *nix-like. So i'm a little unsure why an inability to do so on one platform
is a blocker for doing so for the other platforms?

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.

None yet

4 participants