[SR-14122] Improve usability of -l flag.

[typesanitizer]

Previous ID	SR-14122
Radar	rdar://problem/73742492
Original Reporter	@typesanitizer
Type	Improvement
Status	Closed
Resolution	Done

Attachment: Download

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Improvement, StarterBug
Assignee	Rajagopalan (JIRA)
Priority	Medium

md5: 5811a36dd197d1a76e8beeedde7c7974

Issue Description:

According to the swiftc help page, one can use -l<value> to ask it to link to a library. However, if one adds a space -l <value>, then swiftc emits a strange error error: no such file or directory: 'tmp1'.

We should be consistent with -L and allow a space between the flag and the argument. Alternately, if we want to be consistent with ld64, we can emit an error instead that we got an empty value for -l ("did you add an extra space after '-l'?").

Here's an example of when one could hit this error:

echo 'public func f() { print("Hello") }' > tmp1.swift
xcrun swiftc tmp1.swift -enable-library-evolution  -emit-module-interface-path tmp1.swiftinterface -emit-library -o libtmp1.dylib
echo 'import tmp1\n\ntmp1.f()' > tmp2.swift
xcrun swiftc tmp2.swift -I . -c -o tmp2.o
xcrun swiftc tmp2.o -L . -l tmp1

Fixing this to -ltmp1 would fix the issue (as you can check by running ./tmp2).

[typesanitizer]

@swift-ci create

[typesanitizer]

If you would like to tackle this bug, feel free to ask questions here or tag me on GitHub (@ varungandhi-apple) to review your PR.

[swift-ci]

Comment by Shreya Sharma (JIRA)

Hello,
Can I work on this?

[typesanitizer]

Sure. [More generally, if something is marked as a StarterBug and there is no discussion and no one has assigned it to themselves, you should feel free to assign it to yourself.]

The first thing that needs to be done here is you need to track down where the flag is defined in Options.td (this defines the flags accepted by the driver) and figure out how to make it allow a space as well, in addition to no space. (It's simpler to allow that, rather than adding a diagnostic and have a user waste their time to go and add the space.)

Testing wise, we have some tests under test/Driver in the Swift repository, some of which are making sure the linker invocation is fine. You'll probably want to modify the {{linker.swift }}test file, which is checking that we passing linker arguments correctly. Since the platform linker may not support having a space between -l and the argument, it would be useful to add one test line which uses a space between -l and the argument, and make sure that generated linker invocation still has no space.

[swift-ci]

Comment by Shreya Sharma (JIRA)

I'm still stuck on this one. I'm facing a problem in understanding the errors.

In `/include/swift/Option/Options.td` I've tried defining an alias, or using JoinedOrSeparate and the error led me to `test/Driver/options-repl.swift` but no success so far.

[typesanitizer]

Turns out, I underestimated the complexity, sorry about that. (Also, please feel free to ask for help sooner so that you don't spend a lot of time spinning your wheels.)

Hmm, it looks like changing the parsing changes the output as well. You must've seen this in the FileCheck output:

/Users/varun/foss-swift/swift/test/Driver/options-repl.swift:32:19: error: LLDB-OPTS-DAG: expected strin
g not found in input
// LLDB-OPTS-DAG: -lsomelib
                  ^
<stdin>:1:58: note: scanning from here
/Library/Developer/CommandLineTools/usr/bin/lldb "--repl=-target x86_64-apple-macosx10.9 -enable-objc-in
terop -sdk / -I \"this folder\" -F /path/to/frameworks -module-cache-path BUILD_DIR/swift-test-results/x
86_64-apple-macosx10.9/clang-module-cache -swift-version 4 -D A -D B -D C -D D -ignore-module-source-inf
o -L /path/to/libraries -L /path/to/more/libraries -l somelib -framework SomeFramework"
                                                         ^ 

We need to make sure that we don't pass in the space to LLDB in case it breaks.

If you look at the test, there is the -lldb-repl flag being used which is creating the REPL invocation. In the compiler, that manifests as OPT_lldb_repl which in turn is connected to REPLJobAction::Mode::RequireLLDB. Looking at that further, it seems like the invocation is being constructed in ToolChain::constructInvocation. That function has the following code which seems to be constructing the arguments:

  context.Args.AddAllArgs(FrontendArgs, options::OPT_l, options::OPT_framework,
                          options::OPT_L); 

I'm guessing that the AddAllArgs invocation is indirectly responsible for creating the extra space. If you look around a little bit there, you'll notice:

  template<typename ...OptSpecifiers>
  void AddLastArg(ArgStringList &Output, OptSpecifiers ...Ids) const {
    if (Arg *A = getLastArg(Ids...)) // Calls claim() on all Ids's Args.
      A->render(*this, Output);
  } 

The render is probably what is being called at the end. I would try to search for where JoinedOrSeparate is getting rendered in LLVM code; there must be a place where it is saying that it gets rendered as separate, and maybe there is a way to render it as joined instead.

I tried searching for how JoinedOrSeparate is rendered and ended up here:

  RenderStyleKind getRenderStyle() const {
    if (Info->Flags & RenderJoined)
      return RenderJoinedStyle;
    if (Info->Flags & RenderSeparate)
      return RenderSeparateStyle;
    switch (getKind()) {
    case GroupClass:
    case InputClass:
    case UnknownClass:
      return RenderValuesStyle;
    case JoinedClass: // <-- before
    case JoinedAndSeparateClass:
      return RenderJoinedStyle;
    case CommaJoinedClass:
      return RenderCommaJoinedStyle;
    case FlagClass:
    case ValuesClass:
    case SeparateClass:
    case MultiArgClass:
    case JoinedOrSeparateClass: // <-- now
    case RemainingArgsClass:
    case RemainingArgsJoinedClass:
      return RenderSeparateStyle; // <--
    }
    llvm_unreachable("Unexpected kind!");
  } 

I suspect that's why it is getting rendered as separate, whereas earlier we had it be JoinedClass so it was getting rendered as joined. (I tried using JoinedAndSeparate but that seems to be doing something different.)

We could manually do some string concatenation to make sure we render -l without spaces (or use one of the existing APIs such as GetOrMakeJoinedArgString or AddJoinedArg + getAllArgValues), but I'm a bit wary of doing that since we'll need to touch multiple constructInvocation methods.

I'm trying to see if there is a better way where we specify how the argument can be rendered in one place, and make sure constructInvocation continues to work correctly.

[typesanitizer]

Hmm, there are only 2 places which are relying on this. So I think you can add a new helper function, say addLinkedLibrary }}which does the aforementioned{{ AddJoinedArg + getAllArgValues combination and use that helper function from the following two places

ToolChain::InvocationInfo
ToolChain::constructInvocation(const InterpretJobAction &job,
                               const JobContext &context) 
ToolChain::InvocationInfo
ToolChain::constructInvocation(const REPLJobAction &job,
                               const JobContext &context) const

[swift-ci]

Comment by Shreya Sharma (JIRA)

Thank you so much for the detailed explanation. I can see why `JoinedOrSeparate` fails. I tried `JoinedAndSeparate` here to see the result but ended up with errors. Can you please explain how `JoinedAndSeparate` works?

[typesanitizer]

What JoinedAndSeparate seems to be doing (I'm not 100% sure myself), looking at the tests in LLVM (check out TEST(Option, SlurpJoinedAndSeparate)), is that if you have

-myflagmyarg1 myarg2 

And -myflag marked as JoinedAndSeparate, it will treat the flag as having two arguments myarg1 and myarg2. But that's not exactly what we want, if we have

-larg1 arg2

We would like the -l to only "consume" arg1. If -l also "consumed" arg2, that would be bad, which is why I think the tests start failing if you use JoinedAndSeparate, because it changes how existing commandlines get parsed.

Given that, I think the right fix here is to continue with JoinedOrSeparate (because that gives us the correct parsing behavior) and adjust the printing behavior as follows:

1. Write a function which uses getAllArgValues and AddJoinedArg to select all the -l arguments from one argument list (representing the original compiler invocation), and appends the joined versions to another argument list (an out parameter).

2. Replace the addition of options::OPT_l in the two constructInvocation functions I mentioned earlier with a call to the function you wrote in step 1.

Hope that helps!

[swift-ci]

Comment by Rajagopalan Gangadharan (JIRA)

Hello theindigamer (JIRA User) , is this ticket still available for grabs? If yes, could I work on this 🙂

[typesanitizer]

Sure, go for it.

[swift-ci]

Comment by Rajagopalan Gangadharan (JIRA)

theindigamer (JIRA User) I'm not able to follow the instructions to reproduce the desired effect. What I mean by this is If I try -ltmp1 I get linker error. Am I missing something?

[swift-ci]

Comment by Rajagopalan Gangadharan (JIRA)

theindigamer (JIRA User) Ping

[typesanitizer]

You're on Linux, so there are a few differences between platforms.

• Library evolution, swiftinterfaces and dynamic libraries are not supported.

• Typically the extensions used on Linux are .a (for static libraries) and .so (for dynamic libraries). .dylib is the dynamic library extension for macOS. I would not expect the linker on Linux to look for the dylib extension, since the Linux linker (generally BFD linker aka ld or gold) is different from the macOS linker (ld64). In your case, it shows that gold is being used.

For Linux, I would try a variation of (I haven't tested this but I think it ought to work, maybe with a tweak or two).

'public func f() { print("Hello") }' > tmp1.swift
swiftc tmp1.swift -emit-module-path tmp1.swiftmodule -emit-library -o libtmp1.a
echo 'import tmp1\n\ntmp1.f()' > tmp2.swift
swiftc tmp2.swift -I . -c -o tmp2.o
swiftc tmp2.o -L . -ltmp1 

Wrt pings, I appreciate that you called my attention to it, but I'm generally not checking my work email or attending to Jira over the weekend.

[swift-ci]

Comment by Rajagopalan Gangadharan (JIRA)

Oh ya, I feel dumb should have realized this sooner. Thank you so much for this detailed information. And regarding the ping - sorry will keep this in my mind from next time 🙂