Fix forwarding of -ld-path to Clang

[kabiroberai]

The Clang linker driver spells this as --ld-path so we can't forward the argument wholesale anymore, since as of #1441 we spell it -ld-path in the Swift Driver.

[kabiroberai]

cc @compnerd @MaxDesiatov worth adding that combining -use-ld and -ld-path should "just work" because one specifies the flavour whereas the other specifies the path. We already test this in testLinking but let me know if I'm missing something — happy to add more tests to cover any edge cases I might not have considered.

[kabiroberai]

ooc do we have any integration tests for this sort of thing? Seems like the exact type of situation that's super easy to accidentally regress (the appendLast forwarding was basically spooky action at a distance) but also easy to catch with an integration test.

[compnerd]

There's the swift-integration-tests, but a local test in the test suite for the generated command seems best to me.

[compnerd]

cc @compnerd @MaxDesiatov worth adding that combining -use-ld and -ld-path should "just work" because one specifies the flavour whereas the other specifies the path. We already test this in testLinking but let me know if I'm missing something — happy to add more tests to cover any edge cases I might not have considered.

Consider this usage:

-ld-path=/usr/bin/ld -use-ld=told

Should that execute /usr/bin/ld.told? Or /usr/bin/ld/told? Perhaps /usr/bin/ldtold?

Path composition is not very clear and I don't see test cases that ensure this is correctly diagnosed and heavy fs checks (which will slow down compilation) to ensure correct handling.

[kabiroberai]

Should that execute /usr/bin/ld.told? Or /usr/bin/ld/told? Perhaps /usr/bin/ldtold?

hmm should we handle that at the Swift Driver level or kick the can down the road to Clang though? iiuc Clang uses the --ld-path directly without combining the -fuse-ld arg, and taking a step back those seems like the semantics required by the Swift SDK linker.path spec too.

[kabiroberai]

@compnerd mind kicking off CI please? I can't since I don't have write perms.

[MaxDesiatov]

@swift-ci test

[MaxDesiatov]

@swift-ci test windows

[MaxDesiatov]

hmm should we handle that at the Swift Driver level or kick the can down the road to Clang though?

How does Clang handle this edge case right now? Does Clang document it explicitly? At the very least we should have it documented on our side, and tested if possible to do so.

[kabiroberai]

@MaxDesiatov @compnerd seems that Clang handles it the same way we currently do, i.e. without combining the flavor into the path:

https://github.com/apple/llvm-project/blob/1af12c15c2ba9b21e0cdf46a65bea7aaaf824def/clang/lib/Driver/ToolChain.cpp#L843-L847

--ld-path= takes precedence over -fuse-ld= and specifies the executable name. -B, COMPILER_PATH and PATH and consulted if the value does not contain a path component separator. -fuse-ld=lld can be used with --ld-path= to inform clang that the binary that --ld-path= points to is lld.

This is somewhat covered by our tests since we assert that -use-ld=foo -ld-path=/bar/baz translates to -fuse-ld=foo --ld-path=/bar/baz and not /bar/baz.foo etc. Open to suggestions on how to strengthen the tests and/or documentation, though I do think this PR is orthogonal to that (and hence worth merging first) because it's a bugfix.

[MaxDesiatov]

This is somewhat covered by our tests since we assert that -use-ld=foo -ld-path=/bar/baz translates to -fuse-ld=foo --ld-path=/bar/baz and not /bar/baz.foo etc.

Sorry, I couldn't find this test, would you be able to provide a link to the exact line where this is covered?

[kabiroberai]

@MaxDesiatov sure, it's within testLinking — see the asserts for -fuse-ld and --ld-path

https://github.com/apple/swift-driver/blob/fa4a8eae03d683585294233003c3180385556ded/Tests/SwiftDriverTests/SwiftDriverTests.swift#L1686-L1698

[MaxDesiatov]

Thank you!

[kabiroberai]

🙌 can we cherry-pick this into 5.10? I believe this needs to be fixed there too.