Skip to content

Conversation

@jslegendre
Copy link
Contributor

@jslegendre jslegendre commented Jul 5, 2021

This commit adds support for the -lto_library flag, allowing users to specify a custom LTO library on Darwin. This also fixes an issue where the default LTO library is used even if Driver is run from inside an alternate toolchain.

@gottesmm

This commit adds support for the -lto_library flag, allowing users to specify a custom LTO library on Darwin. This also fixes an issue where the default LTO library is used even if Driver is run from inside an alternate toolchain.
@jslegendre
Copy link
Contributor Author

jslegendre commented Jul 5, 2021

@swift-ci smoke test

@harlanhaskins
Copy link
Contributor

Thank you for your contribution! CI is currently limited to established contributors, so you'll need someone with Contributor access to be able to invoke CI. (I can do that for you)

@swift-ci please smoke test

Also, given that the swift-driver is now the default driver as of Xcode 13, could you also implement this in the swift-driver? It is structured somewhat similarly, so it shouldn't be too difficult now that you've done this work in the legacy driver.

Flags<[FrontendOption, NoInteractiveOption]>,
HelpText<"Specify the LTO type to either 'llvm-thin' or 'llvm-full'">;

def lto_library : Separate<["-"], "lto_library">,
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nitpick: we usually prefer dashes to separate pieces of arguments, e.g. -lto-library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see this is passed down to the linker. Still, I think we should promote linker-style arguments to Swift-style when we expose them from the Driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple enough. Done 👍

@jslegendre
Copy link
Contributor Author

Thank you for your contribution! CI is currently limited to established contributors, so you'll need someone with Contributor access to be able to invoke CI. (I can do that for you)

@swift-ci please smoke test

Also, given that the swift-driver is now the default driver as of Xcode 13, could you also implement this in the swift-driver? It is structured somewhat similarly, so it shouldn't be too difficult now that you've done this work in the legacy driver.

I actually already have. As this is my first PR here I was waiting to see the feedback on this one before opening that one.

Co-authored-by: Owen Voorhees <[email protected]>
@jslegendre
Copy link
Contributor Author

@harlanhaskins Could you run the tests?

@owenv
Copy link
Contributor

owenv commented Jul 7, 2021

@swift-ci please smoke test

@jslegendre
Copy link
Contributor Author

@owenv What is the procedure now that the tests are done? Anything else I should do?

Copy link
Contributor

@owenv owenv left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the delay in reviewing this fully. This LGTM except for one place where I think might be checking the wrong path relative to swift. Once that's addressed (if needed), I think we can get this merged

} else {
// Check for relative libLTO.dylib. This would be the expected behavior in an
// Xcode toolchain.
StringRef P = llvm::sys::path::parent_path(getDriver().getSwiftProgramPath());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think P is going to be <toolchain root>/usr/bin/ here, so we'll end up checking for libLTO in <toolchain root>/usr/bin/lib instead of <toolchain root>/usr/lib. I can't think of a good way to write a lit test for this unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. Fixed in latest commit.

@harlanhaskins
Copy link
Contributor

@swift-ci please smoke test

1 similar comment
@owenv
Copy link
Contributor

owenv commented Jul 13, 2021

@swift-ci please smoke test

@owenv
Copy link
Contributor

owenv commented Jul 14, 2021

@swift-ci please test Windows

@owenv owenv merged commit b14f2b9 into swiftlang:main Jul 15, 2021
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