[Darwin] [Driver] Clang should invoke dsymutil for lto builds -g*

[beanz]

Summary

Clang should always add a dsymutil step whenever debug information is
generated and the compiler is acting as the linker driver with
temporary object files.

https://bugs.llvm.org/show_bug.cgi?id=46841

[JDevlieghere]

When you don’t pass any specific options to the linker, it’s going to generate a temporary lto.o file in /tmp and delete it after the link. When dsymutil will try to read that, it won't be there anymore. Unless I'm missing I don't think this is going to work. If it turns out I'm mistaken please add that situation as a test case.

[beanz]

Fixing bad test case.

[beanz]

@JDevlieghere you are right, I'm missing the change to how clang determines it needs to pass the linker the object file path. Update coming momentarily.

[dsanders]

In D84565#2173615, @JDevlieghere wrote:

When you don’t pass any specific options to the linker, it’s going to generate a temporary lto.o file in /tmp and delete it after the link. When dsymutil will try to read that, it won't be there anymore. Unless I'm missing I don't think this is going to work. If it turns out I'm mistaken please add that situation as a test case.

Are you thinking of the -Wl,-object_path_lto,path/foo-lto.o code in AddLLVM.cmake where dsymutil is invoked after clang finishes? I'm using the same code path as this patch in https://reviews.llvm.org/D84127 using an empty source file to force ContainsCompileOrAssembleAction() to be true and get dsymutil to be invoked. When clang manages both the per-arch links and the dsymutil, the temporary objects get unique names (e.g. -object_path_lto /var/folders/0s/8zdwsz0d1glcx6v1nc_nv5gw0000gn/T/cc-95f3fc.o) and stick around until after dsymutil is invoked.

clang/lib/Driver/Driver.cpp

2037

This should fix the same issue as the workaround I was using in D84127.

I'm not entirely convinced that this is all of it though. It seems strange to me that these two scripts produce different build products (aside from ret0.o):

$ rm -rf external ret0 ret0.*
$ echo 'int main() { return 0; }' > ret0.c
$ use_cflags="--sysroot=$SYSROOT -g"
$ ../build/bin/clang -o ret0 ret0.c ${=use_cflags}
$ ls
ret0      ret0.c    ret0.dSYM

$ rm -rf external ret0 ret0.*
$ echo 'int main() { return 0; }' > ret0.c
$ use_cflags="--sysroot=$SYSROOT -g"
$ ../build/bin/clang -c -o ret0.o ret0.c ${=use_cflags}
$ ../build/bin/clang -o ret0 ret0.o ${=use_cflags}
$ ls
ret0   ret0.c ret0.o

I'd expect them to either consistently extract the dSYM or consistently lipo the slices together including the debug info. I don't have a strong opinion about which is right, it's just the inconsistency that's bothering me

[beanz]

Updated to also pass the linker -object_path_lto

[beanz]

clang/lib/Driver/Driver.cpp

2037

The reason they behave differently is because in the case where clang compiles + links in the same invocation the intermediate object files are deleted. In MachO we do not link debug information in ld, instead we do it in dsymutil. dsymutil cannot run across a temporary object file that has been deleted, so the clang driver runs it for you.

Clang doesn't run it in the case of just linking object files because it takes time and isn't strictly required as long as you keep all your object files around so that your debugger can find the unlinked debug info in the object files. This is why one of the common ways people speed up debug builds with mach-o is to skip dSYM generation.

[dsanders]

In D84565#2173744, @beanz wrote:

@JDevlieghere you are right, I'm missing the change to how clang determines it needs to pass the linker the object file path. Update coming momentarily.

Isn't that already based on isUsingLTO() and getLTOMode() or is there another bit? I'm looking at darwin::Linker::AddLinkArgs()

[dsanders]

In D84565#2173770, @dsanders wrote:

In D84565#2173744, @beanz wrote:

@JDevlieghere you are right, I'm missing the change to how clang determines it needs to pass the linker the object file path. Update coming momentarily.

Isn't that already based on isUsingLTO() and getLTOMode() or is there another bit? I'm looking at darwin::Linker::AddLinkArgs()

Never mind. I've just seen the update :-)

[dsanders]

clang/lib/Driver/Driver.cpp

2037

That makes sense. Although when you do want dSYM's, it does end up requiring the build system to know the logic clang is using and either move the one clang emitted or invoke dsymutil to make one depending on what clang did.

At the moment, the options that cause dsymutil to be invoked by clang that I know of are -flto, compiling/assembling, and multiple values for -arch. It would be easier on the build system if it could force them to be emitted and control where they end up.

[compnerd]

clang/lib/Driver/ToolChains/Darwin.cpp

173

Why not hoist the nullptr check into the assignment?

if (Arg *A = Args.getLastArg(options::OPT_g_Group)
  if (!A->getOption().matches(options::OPT_g0) &&
      !A->getOption().matches(options::OPT_gstabs))
    return true;

clang/lib/Driver/Driver.cpp

if (Inputs.size() == 1 || Act->getType() == types::TY_Nothing)
       Actions.append(Inputs.begin(), Inputs.end());
     else
       Actions.push_back(C.MakeAction<LipoJobAction>(Inputs, Act->getType()));
 
     // Handle debug info queries.
     Arg *A = Args.getLastArg(options::OPT_g_Group);
     bool enablesDebugInfo = A && !A->getOption().matches(options::OPT_g0) &&
                             !A->getOption().matches(options::OPT_gstabs);
+    bool isGeneratingTemporaryObject =
+        ContainsCompileOrAssembleAction(Actions.back()) || isUsingLTO();
     if ((enablesDebugInfo || willEmitRemarks(Args)) &&
-        ContainsCompileOrAssembleAction(Actions.back())) {
+        isGeneratingTemporaryObject) {
 
       // Add a 'dsymutil' step if necessary, when debug info is enabled and we
-      // have a compile input. We need to run 'dsymutil' ourselves in such cases
-      // because the debug info will refer to a temporary object file which
-      // will be removed at the end of the compilation process.
+      // are linking a temporary object. This occurs when we have a compiler
+      // or assmbler input or if LTO is enabled. We need to run 'dsymutil'
+      // ourselves in such cases because the debug info will refer to the
+      // temporary object file which will be removed at the end of the
+      // compilation process.
       if (Act->getType() == types::TY_Image) {
         ActionList Inputs;
         Inputs.push_back(Actions.back());
         Actions.pop_back();
         Actions.push_back(
             C.MakeAction<DsymutilJobAction>(Inputs, types::TY_dSYM));
       }
 

clang/lib/Driver/ToolChains/Darwin.h

public:
 
   void ConstructJob(Compilation &C, const JobAction &JA,
                     const InputInfo &Output, const InputInfoList &Inputs,
                     const llvm::opt::ArgList &TCArgs,
                     const char *LinkingOutput) const override;
 };
 
 class LLVM_LIBRARY_VISIBILITY Linker : public MachOTool {
-  bool NeedsTempPath(const InputInfoList &Inputs) const;
+  bool NeedsTempPath(const InputInfoList &Inputs,
+                     const llvm::opt::ArgList &Args) const;
   void AddLinkArgs(Compilation &C, const llvm::opt::ArgList &Args,
                    llvm::opt::ArgStringList &CmdArgs,
                    const InputInfoList &Inputs, unsigned Version[5]) const;
 
 public:
   Linker(const ToolChain &TC) : MachOTool("darwin::Linker", "linker", TC) {}
 
   bool hasIntegratedCPP() const override { return false; }

clang/lib/Driver/ToolChains/Darwin.cpp

void darwin::MachOTool::AddMachOArch(const ArgList &Args,
   CmdArgs.push_back("-arch");
   CmdArgs.push_back(Args.MakeArgString(ArchName));
 
   // FIXME: Is this needed anymore?
   if (ArchName == "arm")
     CmdArgs.push_back("-force_cpusubtype_ALL");
 }
 
-bool darwin::Linker::NeedsTempPath(const InputInfoList &Inputs) const {
+bool darwin::Linker::NeedsTempPath(const InputInfoList &Inputs,
+                                   const ArgList &Args) const {
+  Arg *A = Args.getLastArg(options::OPT_g_Group);
+  if (A && !A->getOption().matches(options::OPT_g0) &&
+      !A->getOption().matches(options::OPT_gstabs))
+    return true;
+
   // We only need to generate a temp path for LTO if we aren't compiling object
   // files. When compiling source files, we run 'dsymutil' after linking. We
   // don't run 'dsymutil' when compiling object files.
   for (const auto &Input : Inputs)
     if (Input.getType() != types::TY_Object)
       return true;
 
   return false;
if (Args.hasArg(options::OPT_rdynamic) && Version[0] >= 137)
     CmdArgs.push_back("-export_dynamic");
 
   // If we are using App Extension restrictions, pass a flag to the linker
   // telling it that the compiled code has been audited.
   if (Args.hasFlag(options::OPT_fapplication_extension,
                    options::OPT_fno_application_extension, false))
     CmdArgs.push_back("-application_extension");
 
-  if (D.isUsingLTO() && Version[0] >= 116 && NeedsTempPath(Inputs)) {
+  if (D.isUsingLTO() && Version[0] >= 116 && NeedsTempPath(Inputs, Args)) {
     std::string TmpPathName;
     if (D.getLTOMode() == LTOK_Full) {
       // If we are using full LTO, then automatically create a temporary file
       // path for the linker to use, so that it's lifetime will extend past a
       // possible dsymutil step.
       TmpPathName =
           D.GetTemporaryPath("cc", types::getTypeTempSuffix(types::TY_Object));
     } else if (D.getLTOMode() == LTOK_Thin)

clang/test/Driver/darwin-dsymutil.c

 // RUN: %clang -target x86_64-apple-darwin10 -ccc-print-bindings \
 // RUN:   -o bar/foo %s -g 2> %t
 // RUN: FileCheck -check-prefix=CHECK-LOCATION < %t %s
 
 // CHECK-LOCATION: "x86_64-apple-darwin10" - "darwin::Dsymutil", inputs: ["bar/foo"], output: "bar/foo.dSYM"
 
 // Check that we don't crash when translating arguments for dsymutil.
 // RUN: %clang -m32 -arch x86_64 -g %s -###
+
+// Check that clang provides the linker with a temporary object file path and
+// runs dsymutil when -flto is specified and no source is provided
+// RUN: touch %t.o
+// RUN: %clang -target x86_64-apple-darwin10 -arch x86_64 %t.o -g -flto -### \
+// RUN:   2> %t
+// RUN: FileCheck -check-prefix=CHECK-DSYMUTIL-LTO < %t %s
+
+// CHECK-DSYMUTIL-LTO: "/usr/bin/ld" "-demangle" "-object_path_lto"
+// CHECK-DSYMUTIL-LTO: "/usr/bin/dsymutil" "-o" "a.out.dSYM" "a.out"