Page MenuHomePhabricator

Make LLVM_ENABLE_LTO function with multi-arch values for CMAKE_OSX_ARCHITECTURES
AcceptedPublic

Authored by dsanders on Jul 19 2020, 10:18 AM.

Details

Summary

Each ld invocation within the clang command needs a unique filename on the
-object_path_lto option so that dsymutil can extract debug info for all compiled
slices. In the current code, every slice uses the same temporary filename and
as a result only one slice of debug info ends up in the dSYM.

clang already knows how to use -object_path_file and dsymutil for multiarch
builds correctly so use that method instead. There is however a strange quirk.
clang will only do this if there is at least one non-object in the command that
performs a link (this doesn't make sense in my opinion). To work around this
we include a nearly-empty file (empty causes warnings) in the linking clang
invocation.

Diff Detail

Unit TestsFailed

TimeTest
7,240 mslinux > libomp.env::kmp_set_dispatch_buf.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic
1,340 mslinux > libomp.worksharing/for::kmp_set_dispatch_buf.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

Event Timeline

dsanders created this revision.Jul 19 2020, 10:18 AM

tools/lto/hide-linkonce-odr.ll failed on my test run but it seems to be down to my system rather than the compiler (a library is genuinely missing). Will check if it fails without this change too

tools/lto/hide-linkonce-odr.ll failed on my test run but it seems to be down to my system rather than the compiler (a library is genuinely missing). Will check if it fails without this change too

It fails for me without the change too

beanz accepted this revision.Jul 24 2020, 3:05 PM

I was talking to @bogner about this. I think this patch is reasonable, but it is important to note that this patch (and the previous version) are both workarounds for a bug in clang. Anytime the clang driver is provided a flag to generate debug info and it is linking temporary object files, clang should insert a dsymutil step. Right now it only inserts the step if there is a compiler or assembler input to the compiler. I filed a bug to track it (https://bugs.llvm.org/show_bug.cgi?id=46841), and I will upload a patch to the clang driver to fix this issue.

@dsanders, could you add a comment that this is a workaround for the bug in the clang driver and a link to the bug? That will allow us to track making this behavior conditional and maybe eventually removing it.

This revision is now accepted and ready to land.Jul 24 2020, 3:05 PM
dsanders marked an inline comment as done.Jul 24 2020, 5:41 PM

I was talking to @bogner about this. I think this patch is reasonable, but it is important to note that this patch (and the previous version) are both workarounds for a bug in clang. Anytime the clang driver is provided a flag to generate debug info and it is linking temporary object files, clang should insert a dsymutil step. Right now it only inserts the step if there is a compiler or assembler input to the compiler. I filed a bug to track it (https://bugs.llvm.org/show_bug.cgi?id=46841), and I will upload a patch to the clang driver to fix this issue.

Thanks. That makes sense to me, I thought it was pretty strange that output depended on whether it took 1 or >1 steps to get the result.
I also think the emit+move dance is a workaround to not being able to tell clang where to put them. I've posted D84572 for that bit

@dsanders, could you add a comment that this is a workaround for the bug in the clang driver and a link to the bug? That will allow us to track making this behavior conditional and maybe eventually removing it.

Sure

llvm/cmake/modules/AddLLVM.cmake
1987–1997

I've posted https://reviews.llvm.org/D84572 which will (eventually) allow us to remove this emit+move dance by adding -external-dsym-dir=${LLVM_EXTERNALIZE_DEBUGINFO_OUTPUT_DIR} to the cflags/cxxflags in order to emit the dSYM in the desired location to begin with.

dsanders updated this revision to Diff 280638.Jul 24 2020, 5:46 PM
  • Add comment referencing the bug
dsanders updated this revision to Diff 280639.Jul 24 2020, 5:49 PM
  • Comment on the emit+move too
dsanders marked an inline comment as done.Jul 27 2020, 2:21 PM
dsanders added inline comments.
llvm/cmake/modules/AddLLVM.cmake
1997

I'm testing a small bug fix for this before I land it as it currently fails on a missing file when there's no debug info.

dsanders updated this revision to Diff 282725.Aug 3 2020, 2:27 PM

Fix the non-LTO path. It wasn't externalizing the debug info in single-slice builds

dsanders updated this revision to Diff 282738.Aug 3 2020, 3:02 PM

Last one our own testing hit: Fix the case where lack of debug info means we didn't extract dsyms

dsanders updated this revision to Diff 282787.Aug 3 2020, 7:38 PM

Make standalone clang builds work correctly. LLVM_SOURCE_DIR isn't set for those
Small naming change to match that directory