Jobs: do not inject the swiftautolink file into archives

[compnerd]

It has been observed that a static library may sometimes contain the
autolink extracted rules. This causes autolink-extract to not be able
to process the archive as a dependency, causing a build failure.

[artemcm]

@swift-ci please test

[compnerd]

We should figure out if we can craft a test case for this, but this stands a good chance of fixing the stray autolink extract file getting into the static library.

[finagolfin]

Heh, I've been seeing this for months and just filed a bug for it, SR-14981. I was just trying out a similar fix that works, but I included bitcode since that's what the C++ driver does, .filter { $0.type == .object || $0.type == .llvmBitcode }. You may want to do the same.

As for a test, I think checking to make sure the Test.autolink file is not included here would works, but I haven'tjust tried out amending that test adding this line there and it detects the file, XCTAssertFalse(commandContainsTemporaryPath(cmd, "Test.autolink")).

[compnerd]

The bitcode should not be in this path; there is a separate path that can permit the bitcode files as well. There is no guarantee that the normal archiver/librarian will support the bitcode. That is why there is a special case for the LTO.

For the testing, I couldn't figure out how to get the driver to inject the autolink file. I agree that adding the explicit check there is better than nothing, but some reproducible before/after tests would be very much more comforting.

[finagolfin]

We should get this into trunk and Swift 5.5 soon, as this will break any Swift projects using CMake and static libraries, which ironically includes this project through its Yams dependency, as noted on the linked bug.

What's holding this up, @compnerd, are you writing more tests? Can't we just get this in with the test I suggested and you can add more tests later?

[CodaFi]

Since we still do not have a clear understanding of exactly how this occurs, but we also have evidence this is impacting our adopters, let’s merge and nominate. If we find a reproducer, we’ll make a test of it then

[compnerd]

Sounds like a plan @CodaFi! @buttaface, Im not convinced that test case adds much value as that does not routinely pick up the actual command list. I can certainly add that as a test case if you feel strongly about that. But that can easily be a follow up.

[tomerd]

thanks for taking care of this @compnerd and @CodaFi, I assume we are following up on the llvmBitcode point made by @buttaface in a follow up PR?

[compnerd]

Yes, I'll do a follow up for the bitcode issue, that is totally a valid issue.

[compnerd]

@buttaface oh, going back through the code now, I see why you were bringing it up here. I don't know what bit of code I was mixing it up with, because I remember the LTO and non-LTO paths as being different.

[finagolfin]

we still do not have a clear understanding of exactly how this occurs

I found the -driver-print-bindings flag to be useful in figuring out what's going on here:

> ./swift-DEVELOPMENT-SNAPSHOT-2021-07-24-a-centos8/usr/bin/swiftc -emit-library swift/test/Interpreter/Inputs/lto/module1.swift -module-name Test -v -static -disallow-use-new-driver -driver-print-bindings
<unknown>:0: warning: legacy driver is now deprecated; consider avoiding specifying `-disallow-use-new-driver`
Swift version 5.6-dev (LLVM 294fb71bbb58dbf, Swift b6af50d7df4ac93)
Target: x86_64-unknown-linux-gnu
# "x86_64-unknown-linux-gnu" - "swift-frontend", inputs: ["swift/test/Interpreter/Inputs/lto/module1.swift"], output: {object: "/tmp/module1-c6d728.o"}
# "x86_64-unknown-linux-gnu" - "swift-autolink-extract", inputs: ["/tmp/module1-c6d728.o"], output: {autolink: "/tmp/module1-3634b9.autolink"}
# "x86_64-unknown-linux-gnu" - "ar", inputs: ["/tmp/module1-c6d728.o", "/tmp/module1-3634b9.autolink"], output: {image: "libTest.a"}

It reports the autolink file as an input to the final static linking step even without the new driver, so I tried this simple patch and I was able to reproduce this bug with the old C++ Driver too:

diff --git a/lib/Driver/UnixToolChains.cpp b/lib/Driver/UnixToolChains.cpp
index 30905f6b722..132ab5545cb 100644
--- a/lib/Driver/UnixToolChains.cpp
+++ b/lib/Driver/UnixToolChains.cpp
@@ -393,9 +393,9 @@ toolchains::GenericUnix::constructInvocation(const StaticLinkJobAction &job,
   addPrimaryInputsOfType(Arguments, context.Inputs, context.Args,
                          file_types::TY_Object);
   addPrimaryInputsOfType(Arguments, context.Inputs, context.Args,
-                         file_types::TY_LLVM_BC);
+                         file_types::TY_AutolinkFile);
   addInputsOfType(Arguments, context.InputActions, file_types::TY_Object);
-  addInputsOfType(Arguments, context.InputActions, file_types::TY_LLVM_BC);
+  addInputsOfType(Arguments, context.InputActions, file_types::TY_AutolinkFile);
 
   InvocationInfo II{AR, Arguments};
 

-driver-print-bindings shows a similar situation with the new swift-driver, we have to actively filter the autolink file out or it will be added. As for why there would sometimes be no autolink file, I think it's because the autolink file would sometimes be empty and ar seems to throw away empty files from archives.

[compnerd]

The problem was I couldn't get the autolink extract file to be generated consistently. Are you saying that what I didn't understand was that it needs -driver-print-bindings specified to guarantee the order and ensure that the autolink commands are passed into the inputs array? If so, that sounds like exactly what I was in search of for a test.

[finagolfin]

No, I didn't understand what you meant earlier by "Im not convinced that test case adds much value as that does not routinely pick up the actual command list," but I think I know our miscommunication now.

You're worried that the autolink file is not always in the library, but I'm only worried about the linking command produced by this driver. The former is inconsistent, the latter is 100% reproducible. Try reverting this pull and adding the single line testcase I gave above and running the tests repeatedly: I believe it will fail 100% of the time, because it simply checks what linker command-line is generated and not the final static library for an autolink file.

Anyway, my explanation above is simply to share with @CodaFi and you guys what my investigation into the root cause found, I wasn't thinking of the test case when I wrote that. I don't think -driver-print-bindings is needed for any of this, it's just a convenient debugging command that I used to check the internal state of the two drivers.

[compnerd]

Interesting, however, I can see your point about the command line being a good proxy for the issue. It is further indication about what I was suspecting that is happening. If it reproducibly creates the invalid command line, I think that adding a test for that still is validating something important and makes sense to have as a test case. I will try to get a follow up PR to add that then.