Page MenuHomePhabricator

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

Authored by beanz on Jul 24 2020, 4:22 PM.

Details

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

Diff Detail

Unit TestsFailed

TimeTest
260 mslinux > Clang.Driver::Unknown Unit Message ("")
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -target x86_64-apple-darwin10 -ccc-print-phases -arch i386 -arch x86_64 /mnt/disks/ssd0/agent/llvm-project/clang/test/Driver/darwin-dsymutil.c -g 2> /mnt/disks/ssd0/agent/llvm-project/build/tools/clang/test/Driver/Output/darwin-dsymutil.c.tmp
2,310 mswindows > Clang.Driver::Unknown Unit Message ("")
Script: -- : 'RUN: at line 3'; c:\ws\w32-1\llvm-project\premerge-checks\build\bin\clang.exe -target x86_64-apple-darwin10 -ccc-print-phases -arch i386 -arch x86_64 C:\ws\w32-1\llvm-project\premerge-checks\clang\test\Driver\darwin-dsymutil.c -g 2> C:\ws\w32-1\llvm-project\premerge-checks\build\tools\clang\test\Driver\Output\darwin-dsymutil.c.tmp

Event Timeline

beanz created this revision.Jul 24 2020, 4:22 PM
Herald added a project: Restricted Project. · View Herald Transcript
bogner accepted this revision.Jul 24 2020, 4:36 PM
This revision is now accepted and ready to land.Jul 24 2020, 4:36 PM
JDevlieghere requested changes to this revision.Jul 24 2020, 4:46 PM

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.

This revision now requires changes to proceed.Jul 24 2020, 4:46 PM
beanz updated this revision to Diff 280641.Jul 24 2020, 6:00 PM

Fixing bad test case.

beanz added a comment.Jul 24 2020, 6:25 PM

@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.

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 this revision to Diff 280647.Jul 24 2020, 6:34 PM

Updated to also pass the linker -object_path_lto

beanz marked an inline comment as done.Jul 24 2020, 6:39 PM
beanz added inline comments.
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.

@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()

@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 added inline comments.Jul 24 2020, 6:57 PM
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 added inline comments.Jul 27 2020, 3:18 PM
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;