Improve usability of -l flag

[RAJAGOPALAN-GANGADHARAN]

Adding space to -l flag causes swiftc to produce abstract error. This PR aims at improving usability to -l flag by allowing it to take a space.

Resolves SR-14122.

[RAJAGOPALAN-GANGADHARAN]

Requesting review from @varungandhi-apple

I have few queries:

1. How do I run linker.swift tests, it reports as unsupported on my machine.
2. Should there be a test under options-repl.swift with space

[varungandhi-apple]

How do I run linker.swift tests, it reports as unsupported on my machine.

That test has a REQUIRES: rdar65281056. These REQUIRES variables are passed in based on the target. In this case, this test was turned off and investigating why it was failing/unreliable and turning it back on is tracked by rdar://65281056 which refers to a ticket in Apple's internal bug tracker.

When I wrote it in the bug report, I hadn't realized that this test was actually turned off. In the meantime, you could add new test file until that is turned back on.

Should there be a test under options-repl.swift with space

I don't think there needs to be a separate test but you could modify this line:

// RUN: %swift_driver -sdk "" -lldb-repl -D A -DB -D C -DD -L /path/to/libraries -L /path/to/more/libraries -F /path/to/frameworks -lsomelib -framework SomeFramework -sdk / -I "this folder" -module-name Test -target %target-triple -### | %FileCheck -check-prefix=LLDB-OPTS %s

To include a -l otherlib (and do some setup for otherlib and add a check that we still emit -lotherlib (without the space).

[RAJAGOPALAN-GANGADHARAN]

When I wrote it in the bug report, I hadn't realized that this test was actually turned off. In the meantime, you could add new test file until that is turned back on.

I tried writing the same test which I added, to a new file. Turns out its expecting -l boo for that case. Is it an expected behavior? The driver code has to take care of l flag while handling a repl/Interpret(not sure what that is). So I think it is ok to emit a space in this case. Or maybe it is assumed that there will never be a space after l flag? Please correct me if I am wrong.

To include a -l otherlib (and do some setup for otherlib and add a check that we still emit -lotherlib (without the space).

This is done and works as expected!

[varungandhi-apple]

I tried writing the same test which I added, to a new file. Turns out its expecting -l boo for that case. Is it an expected behavior?

For places where you are checking some invocation of a different executable (such as checking the flags passed to clang, lldb or the linker) by the compiler, the output needs to omit the space, because that was the old behavior. The only change is in what the Swift compiler itself accepts. If the test requires -l boo to pass in the FileCheck lines when -l boo is passed in the input, that means the implementation is incorrect. You could use a debugger to step through the code or use print debugging (say inside the newly introduced loop) to figure out why it's adding the space.

[RAJAGOPALAN-GANGADHARAN]

I understand, this a wip commit fixing the memory leak you pointed out earlier. Will do a clang-format in the end when I figure out why its adding the space. Thank you for reviewing!

[compnerd]

This really seems like a very odd departure. None of the unix-ish compiler drivers support a separated -l option, only a separated -I option. Most of the flag behaviours in swiftc are very similar to clang which I think is a very good model to follow as it means that there is less confusion about users switching between the tools especially when they are so intricately tied via the ClangImporter. I think that it improving the usability of linker options is the goal, a better approach might be to follow in the footsteps of MSVC (anything following /LINK is passed to the linker - https://docs.microsoft.com/en-us/cpp/build/reference/link-pass-options-to-linker?view=msvc-160).

[varungandhi-apple]

None of the unix-ish compiler drivers support a separated -l option, only a separated -I option.

I don't think that's really a good justification. Here's a snippet from ld64's man page:

     -Ldir   Add dir to the list of directories in which to search for
             libraries.  Directories specified with -L are searched in
             the order they appear on the command line and before the
             default search path. In Xcode4 and later, there can be a
             space between the -L and directory.

Requiring people to remember odd rules isn't helpful. Also, it's not the case that for people used to -lfoo, -l foo will be difficult to understand, or that we're disallowing -lfoo.

Most of the flag behaviours in swiftc are very similar to clang

Well then maybe this should be changed in Clang too... If we keep flag behaviors aside for a little bit and look at flag names, Swift has definitely prioritized internal consistency and readability/usability over consistency with Clang. Compare:

-S -emit-llvm vs -emit-ir
-Xclang vs -Xfrontend
-mllvm vs -Xllvm
-S vs -emit-assembly (but -S also supported)
-c vs -emit-object (but -c is also supported)

This change is in a similar vein, where the old thing continues to work but there is an incremental improvement.

I think that [if] improving the usability of linker options is the goal, a better approach might be to follow in the footsteps of MSVC (anything following /LINK is passed to the linker

Isn't that's a bigger departure from we have already today and other tools? I'm a little confused because it seems like you're not in favor of making this small change but you're in favor of making a larger change?

I'm not opposed to making larger improvements (I haven't given too much thought to this particular proposal), but I think we should try to make small improvements too if possible until large improvements get made.

[RAJAGOPALAN-GANGADHARAN]

@varungandhi-apple : Did clang formatting, Figured out the arguments of -l is sent by linker_option_Group - handled it seperately.

I do have one question l with a space seems to be a problem with lldb but while passing it to clang it is not a problem though (looks like clang can have space after -l flag). So is it necessary to do that?

Thank you for reviewing.

[compnerd]

@varungandhi-apple correct, I'm against this change. I think that it is making a small change that really doesn't have much value (IMO). The separated -l option just adds another spelling when I think that we should either go the other way (we could disallow the separated -I if that is a concern). As it is, I've found that the separated spelling already causes some problem in the build and we end up using the joined form in the build rules. Note that using the separated -l form will cause more problems for the CMake build (-l will be uniqued, so if a user does -l foo -l bar you end up with -l foo bar). Using a "larger" departure, makes more sense if the goal here is to improve the UX. /LINK ... basically is treated as -- is normally done, everything which follows is passed to the linker.

In the case of swiftc, we do need -Xfrontend and -Xclang because we have two frontends to contend with: clang and swift and it makes sense that -Xfrontend passes flags to the swift frontend as the driver is swiftc. -mllvm has always been an internal AUIU, -Xllvm is what clang/clang-cl uses as well. The -S and -c are both there I agree, though I think that we should move towards removing them in swiftc. There are more output formats in swiftc than in clang (object, assembly, sil, sib, bc, swiftmodule, swiftdoc, swiftinterface off the top of my head), and having a uniform option set to emit them makes sense.

For the linker flags, it makes more sense to change the behaviour more drastically rather than these small divergences that add up and we have to provide backwards support indefinitely.

[varungandhi-apple]

I think that it is making a small change that really doesn't have much value (IMO).

I know this is a subjective thing, so I tried to ask some other folks on what they thought (considering the possibility that I was over-valuing the change because I filed the JIRA). I got +1s from @jckarter and @beccadax on this change, even if it only improves usability by a small amount.

(we could disallow the separated -I if that is a concern).

This isn't a concern (at least for me) and it could break downstream code. As I said earlier, I very much believe "Requiring people to remember odd rules isn't helpful". From the compiler's point of view, being specific about spaces in certain places and not others isn't really buying us anything.

As it is, I've found that the separated spelling already causes some problem in the build and we end up using the joined form in the build rules. Note that using the separated -l form will cause more problems for the CMake build

I'm not suggesting changing the build rules though. If the concern here is that it will lead to issues when someone is changing the build, due to CMake reasons, then they can fix it then. Even with the current state, you will break things if you add a space after the -l because of swiftc not accepting it, so that's not a change in the status quo.

[varungandhi-apple]

I do have one question l with a space seems to be a problem with lldb but while passing it to clang it is not a problem though (looks like clang can have space after -l flag). So is it necessary to do that?

It's not strictly necessary but I think it would be preferable to be consistent in our output.

[varungandhi-apple]

Since this is a driver change to option parsing, I think this needs a matching PR for apple/swift-driver.

[compnerd]

What do you think of spelling the separate version -link-library ? It seems much more inline with the swiftc flags.

[RAJAGOPALAN-GANGADHARAN]

Since this is a driver change to option parsing, I think this needs a matching PR for apple/swift-driver.

Yes will take a look into that!

[RAJAGOPALAN-GANGADHARAN]

@varungandhi-apple pardon me for these amateur level questions. But using the guide provided I'm unable to build swift driver for linux.

remark: Incremental compilation has been disabled: libCYaml.so has no swiftDeps file
remark: Incremental compilation has been disabled: libFoundation.so has no swiftDeps file
Illegal instruction (core dumped)

This is an error I get when I use cmake to build yams. I am unable to use swift package manager (swift build) on yams, argument-parser etc as I just get Illegal Instruction (core dumped).

Could you point me to any relevant information regarding building on linux. Thank you!

P.S. I got swift core libs to build with --xctest flag. Its the other requirements that I am unable to build.

Edit : I was able to compile swift driver on mac and start working on the pr. But still any help on linux would be nice!

[varungandhi-apple]

What do you think of spelling the separate version -link-library? It seems much more inline with the swiftc flags.

Sure, if you'd like to add the separately, that's fine by me. I don't see that as an alternative to this PR.

pardon me for these amateur level questions

You don't need to apologize. If something is not working, that is something that we need to fix in the code or docs. It seems like there is a bug in the build for Linux, we shouldn't be crashing like this. I don't know of a solution off the top of my head, but I will take a look at it and get back to you.

[varungandhi-apple]

@swift-ci smoke test

[varungandhi-apple]

@swift-ci smoke test

[varungandhi-apple]

Here's the failure on macOS:

******************* TEST 'Swift(macosx-x86_64) :: Driver/linker-library-with-space.swift' FAILED ********************
Script:
--
: 'RUN: at line 1';   env SDKROOT=/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.0.sdk '/Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64/bin/swiftc' -toolchain-stdlib-rpath -Xlinker -rpath -Xlinker /usr/lib/swift -module-cache-path /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache -swift-version 4  -Xfrontend -define-availability -Xfrontend 'SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0' -sdk "" -driver-print-jobs -target x86_64-unknown-linux-gnu -Ffoo -Fsystem car -F cdr -framework bar -Lbaz -lboo -Xlinker -undefined /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/Driver/linker-library-with-space.swift 2>&1 > /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/Driver/Output/linker-library-with-space.swift.tmp.linux.txt
: 'RUN: at line 2';   /Applications/Xcode-beta.app/Contents/Developer/usr/bin/python3 /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/utils/PathSanitizingFileCheck --sanitize BUILD_DIR=/Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64 --sanitize SOURCE_DIR=/Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift --use-filecheck /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/llvm-macosx-x86_64/bin/FileCheck  -check-prefix LINUX-lib-flag-space /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/Driver/linker-library-with-space.swift < /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/Driver/Output/linker-library-with-space.swift.tmp.linux.txt
--
Exit Code: 1

Command Output (stderr):
--
/Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/Driver/linker-library-with-space.swift:18:26: error: LINUX-lib-flag-space: expected string not found in input
// LINUX-lib-flag-space: -o linker
                         ^
<stdin>:3:590: note: scanning from here
/Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/llvm-macosx-x86_64/./bin/clang -fuse-ld=gold -pie -Xlinker -rpath -Xlinker usr/lib/swift/linux usr/lib/swift/linux/x86_64/swiftrt.o /var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/linker-library-with-space-146e20.o -F foo -iframework car -F cdr @/var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/linker-library-with-space-92c32a.autolink -L usr/lib/swift/linux -lswiftCore --target=x86_64-unknown-linux-gnu -framework bar -L baz -lboo -Xlinker -rpath -Xlinker /usr/lib/swift -Xlinker -undefined -o main
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             ^

Input file: <stdin>
Check file: /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/swift/test/Driver/linker-library-with-space.swift

-dump-input=help explains the following input dump.

Input was:
<<<<<<
          1: BUILD_DIR/bin/swift-frontend -frontend -c -primary-file SOURCE_DIR/test/Driver/linker-library-with-space.swift -target x86_64-unknown-linux-gnu -disable-objc-interop -F foo -Fsystem car -F cdr -module-cache-path BUILD_DIR/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache -swift-version 4 -define-availability "SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0" -module-name main -o /var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/linker-library-with-space-146e20.o
          2: BUILD_DIR/bin/swift-autolink-extract /var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/linker-library-with-space-146e20.o -o /var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/linker-library-with-space-92c32a.autolink
          3: /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/llvm-macosx-x86_64/./bin/clang -fuse-ld=gold -pie -Xlinker -rpath -Xlinker usr/lib/swift/linux usr/lib/swift/linux/x86_64/swiftrt.o /var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/linker-library-with-space-146e20.o -F foo -iframework car -F cdr @/var/folders/_8/79jmzf2142z2xydc_01btlx00000gn/T/linker-library-with-space-92c32a.autolink -L usr/lib/swift/linux -lswiftCore --target=x86_64-unknown-linux-gnu -framework bar -L baz -lboo -Xlinker -rpath -Xlinker /usr/lib/swift -Xlinker -undefined -o main
check:18                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  X~~~~~~ error: no match found
>>>>>>

[varungandhi-apple]

apple/swift-driver#795

@swift-ci smoke test

[RAJAGOPALAN-GANGADHARAN]

Here's the failure on macOS:

Fixed the failing part, could you trigger this again please?

I have fixed the failure on the swift-driver side as well. So please check for that as well

[varungandhi-apple]

apple/swift-driver#795

@swift-ci smoke test

[RAJAGOPALAN-GANGADHARAN]

@varungandhi-apple the failing tests are not related to my pr(or maybe) do I have to do something for this?

[varungandhi-apple]

It looks like a build system problem. Let me retry.

apple/swift-driver#795

@swift-ci smoke test

[varungandhi-apple]

Merged since tests were passing.