clang/darwin: Don't use response files with ld64

[thakis]

Summary

This morally reverts D82777 -- turns out that ld64 crashes with many
response files, so we must stop passing them to it until the crash is
fixed.

[keith]

Note in your repro case the issue is that the response file doesn't end in a trailing newline

[thakis]

In D92357#2424422, @keith wrote:

Note in your repro case the issue is that the response file doesn't end in a trailing newline

Hm, we also see crashes if we let clang write the response file that's passed to ld64 though. (ie we see this both with clang @foo.rsp -o a.out and clang -Wl,@foo.rsp -o a.out.

[keith]

It looks like at least in the -Wl case clang passes the file through directly, so I'm not surprises that it repros there:

% clang -Wl,@shell_dialogs_unittests.rsp -v
Apple clang version 12.0.0 (clang-1200.0.32.27)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Applications/Xcode-12.2.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
 "/Applications/Xcode-12.2.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ld" -demangle -lto_library /Applications/Xcode-12.2.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/libLTO.dylib -dynamic -arch x86_64 -platform_version macos 10.15.0 10.15.6 -syslibroot /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -o a.out -L/usr/local/lib @shell_dialogs_unittests.rsp -lSystem /Applications/Xcode-12.2.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/12.0.0/lib/darwin/libclang_rt.osx.a
clang: error: unable to execute command: Segmentation fault: 11
clang: error: linker command failed due to signal (use -v to see invocation)

I _don't_ actually see this reproing when using clang @shell_dialogs_unittests.rsp:

% /Users/ksmiley/dev/llvm-project/build/bin/clang-12 @shell_dialogs_unittests.rsp
clang version 12.0.0 (https://github.com/llvm/llvm-project bbf02e18f53681c079f7a88b2726d0714d92e1a0)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Users/ksmiley/dev/llvm-project/build/bin
 "/usr/bin/ld" @/var/folders/h6/6btvg47n7y1f5xxz4_n9rsd40000gn/T/response-5a15ec.txt

Which I believe makes sense because clang is processing each argument and writing the response file itself.

Is there any other case you've hit besides the missing trailing newline that causes this issue? While I'm sure Apple will fix that in due time I wonder if that's considered a "valid enough" response to warrant the revert here.

[thakis]

In D92357#2424484, @keith wrote:

It looks like at least in the -Wl case clang passes the file through directly,

Right, that's my point: We see crashes around response files even if we let clang write it.

I _don't_ actually see this reproing when using clang @shell_dialogs_unittests.rsp:

Yes, it's much more fickle to repro . See https://bugs.chromium.org/p/chromium/issues/detail?id=1147968 (linked to from the openrdar): We tried passing response files to clang instead of -Wl,-filelist like we currently do in Chromium and after a while (within about a day after the change landing; after hours after 2nd attempt) ld64 started crashing. I failed to repro this locally. With -Wl,@foo.rsp the crash reprod locally immediately (and appeared on the bots immediately), so that's what I used for filing the rdar. But even with clang writing the response file, we found ld64 to be unusuably crashy.

[thakis]

Extracted the darwinnew bits to D92399. Will rebase this once that's in.

[thakis]

rebase

[hans]

lgtm

[keith]

I'm a bit worried that without a repro case (outside of the missing newline) Apple won't get around to fixing this, and in the meantime I'm wondering if outside of the chromium build system folks will experience this. I wonder if there's an acceptable alternative here such as adding a (ideally temporary) flag to force the old behavior?

[thakis]

In D92357#2426042, @keith wrote:

I'm a bit worried that without a repro case (outside of the missing newline) Apple won't get around to fixing this, and in the meantime I'm wondering if outside of the chromium build system folks will experience this. I wonder if there's an acceptable alternative here such as adding a (ideally temporary) flag to force the old behavior?

Chromium just lets ninja write the rsp file, like many projects.

clang/lib/Driver/ToolChains/Darwin.cpp

if (!Args.hasArg(options::OPT_nostdlib, options::OPT_nodefaultlibs)) {
     if (Arg *A = Args.getLastArg(options::OPT_fveclib)) {
       if (A->getValue() == StringRef("Accelerate")) {
         CmdArgs.push_back("-framework");
         CmdArgs.push_back("Accelerate");
       }
     }
   }
 
-  ResponseFileSupport ResponseSupport = ResponseFileSupport::AtFileUTF8();
-  if (Version[0] < 607) {
+  ResponseFileSupport ResponseSupport;
+  if (LinkerIsLLDDarwinNew) {
+    // Xcode12's ld64 added support for @response files, but it's crashy:
+    // https://openradar.appspot.com/radar?id=4933317065441280
+    // FIXME: Pass this for ld64 once it no longer crashes.
+    ResponseSupport = ResponseFileSupport::AtFileUTF8();
+  } else {
     // For older versions of the linker, use the legacy filelist method instead.
     ResponseSupport = {ResponseFileSupport::RF_FileList, llvm::sys::WEM_UTF8,
                        "-filelist"};
   }
 
   std::unique_ptr<Command> Cmd = std::make_unique<Command>(
       JA, *this, ResponseSupport, Exec, CmdArgs, Inputs, Output);
   Cmd->setInputFileList(std::move(InputFileList));