@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Orlando Cazalet-Hyams (OCHyams)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 8880c9375143f..38eebbd6f9b5f 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -4611,8 +4611,14 @@ renderDebugOptions(const ToolChain &TC, const Driver &D, const llvm::Triple &T,
       CmdArgs.push_back("-gembed-source");
   }
 
+  // Enable Key Instructions by default if optimisations are enabled and
+  // we're emitting DWARF.
+  Arg *OptLevel = Args.getLastArg(options::OPT_O_Group);
+  bool KeyInstructionsOnByDefault =
+      EmitDwarf && OptLevel && !OptLevel->getOption().matches(options::OPT_O0);
   if (Args.hasFlag(options::OPT_gkey_instructions,
-                   options::OPT_gno_key_instructions, false))
+                   options::OPT_gno_key_instructions,
+                   KeyInstructionsOnByDefault))
     CmdArgs.push_back("-gkey-instructions");
 
   if (EmitCodeView) {
diff --git a/clang/test/DebugInfo/KeyInstructions/flag.cpp b/clang/test/DebugInfo/KeyInstructions/flag.cpp
index e34faa6cbb347..05a34ac670feb 100644
--- a/clang/test/DebugInfo/KeyInstructions/flag.cpp
+++ b/clang/test/DebugInfo/KeyInstructions/flag.cpp
@@ -1,7 +1,5 @@
 // RUN: %clang -### -target x86_64 -c -gdwarf -gkey-instructions %s 2>&1 | FileCheck %s --check-prefixes=KEY-INSTRUCTIONS
 // RUN: %clang -### -target x86_64 -c -gdwarf -gno-key-instructions %s 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
-//// Default: Off.
-// RUN: %clang -### -target x86_64 -c -gdwarf %s 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
 
 //// Help.
 // RUN %clang --help | FileCheck %s --check-prefix=HELP
@@ -23,3 +21,34 @@ void f() {}
 // RUN: %clang_cc1 %s -triple x86_64-linux-gnu -gkey-instructions -debug-info-kind=line-tables-only -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
 // SMOKETEST-ON: keyInstructions: true
 // SMOKETEST-ON: atomGroup: 1
+
+//// Enable Key Instructions by default if optimisations are enabled and we're
+//// emitting DWARF.
+////
+//// | opt level | -gkey-instructions | feature |
+//// |         0 |                 no |     off |
+//// |         0 |                yes |      on |
+//// |       >=1 |                 no |      on |
+//// |       >=1 |                yes |      on |
+//// |       >=1 |   no & no -g flags |     off |
+//// |       >=1 | no & emit codeview |     off |
+//
+// RUN: %clang %s     -target x86_64 -gdwarf    -gmlt                    -### 2>&1 | FileCheck %s --check-prefix=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s     -target x86_64 -gdwarf    -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O0 -target x86_64 -gdwarf    -gmlt                    -### 2>&1 | FileCheck %s --check-prefix=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s -O0 -target x86_64 -gdwarf    -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gdwarf    -gmlt                    -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gdwarf    -gmlt -gkey-instructions -### 2>&1 | FileCheck %s --check-prefix=KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64                                     -### 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
+// RUN: %clang %s -O1 -target x86_64 -gcodeview -gmlt                    -### 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
+//
+// RUN: %clang %s     -target x86_64 -gdwarf    -gmlt                    -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-OFF
+// RUN: %clang %s     -target x86_64 -gdwarf    -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O0 -target x86_64 -gdwarf    -gmlt                    -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-OFF
+// RUN: %clang %s -O0 -target x86_64 -gdwarf    -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64 -gdwarf    -gmlt                    -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64 -gdwarf    -gmlt -gkey-instructions -S -emit-llvm -o - | FileCheck %s --check-prefix=SMOKETEST-ON
+// RUN: %clang %s -O1 -target x86_64                                     -S -emit-llvm -o - | FileCheck %s --check-prefixes=SMOKETEST-OFF,SMOKETEST-NO-DEBUG
+// RUN: %clang %s -O1 -target x86_64 -gcodeview -gmlt                    -S -emit-llvm -o - | FileCheck %s --check-prefixes=SMOKETEST-OFF
+// SMOKETEST-NO-DEBUG: llvm.module.flags
+// SMOKETEST-NO-DEBUG-NOT: DICompileUnit

Ping (should I move the question to discourse?)

A summary of DWARF file size/section size changes (how much change as a % of total file size in object and linked executables, as a % of aggregate DWARF section sizes in object and linked executables, and as a % of the specific DWARF section sizes (eg: {.rela,}.debug_line changes by x%) - with specific debug info flags (my preference would be for -fno-standalone-debug -gz=zstd with crel, but I understand if that's not your deployment target, etc, it's less relevant)) and which debuggers are effected in what ways (some discussion of gdb would be nice).

If this has a cost and no benefit for lldb, seems like maybe we'd want to keep it off under lldb tuning. If it has a cost and no benefit to gdb - then I'd probably say off-by-default, on-for-SCE.

Thanks for taking a look!

A summary of DWARF file size/section size changes (how much change as a % of total file size in object and linked executables, as a % of aggregate DWARF section sizes in object and linked executables, and as a % of the specific DWARF section sizes (eg: {.rela,}.debug_line changes by x%) - with specific debug info flags (my preference would be for -fno-standalone-debug -gz=zstd with crel, but I understand if that's not your deployment target, etc, it's less relevant))

Here's bloaty figures for some CTMark builds ($ bloaty <-gkey-instructions> -- <-gno-key-instructions>)

I'm only including .debug_line as bloaty reports changes to a few other sections of <= 5 bytes in the first table and <= 50 bytes in the second, which is negligible.

-O2 -g -gkey-instructions vs baseline of -O2 -g:

-fno-standalone-debug -gz=zstd -O2 -g -gkey-instructions vs baseline of -fno-standalone-debug -gz=zstd -O2 -g:

There's a compile time cost to enabling the feature (0.52% on compile-time-tracker) which was included in the compile-time-cost tables in this post which explains where we put work in to "pay for" the cost of the feature (and we have longer term general improvements we'd like to make).

I'm happy to get more numbers if there're any other configurations you'd like to see.

and which debuggers are effected in what ways (some discussion of gdb would be nice). If this has a cost and no benefit for lldb, seems like maybe we'd want to keep it off under lldb tuning. If it has a cost and no benefit to gdb - then I'd probably say off-by-default, on-for-SCE.

SCE (full disclosure - most tested) and GDB (*) and both work with the feature (both provide the promised improved optimisied-code stepping), but LLDB doesn't yet. I'd still like to gently suggest we enable it for LLDB too for uniformity, especially as we've tried to pay some of the costs in other areas, so the net "feature doesn't exist" -> "feature on by default" cost is < 0.3% increased instructions:u on compile-tim-tracker (and from the other bits of work, max rss has come down by 0.87%), unless that seems unreasonable or generally undesirable.

(*) Checked with 15.0.50 and 12.0.90. GCC's -gstatement-frontiers option (which is also enabled by default for optimisations) requires the same debugger support as key-instructions and the earliest mention I could find was gcc-8.1 (2018) https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gcc/Debugging-Options.html - I would assume (but have not verified) that gdb versions from around then would support gcc's -gstatement-frontiers and therefore key instructions.

Cool - thansks for all the details. I'm fine with making the change in general then, but should get some thougths from the apple folks, probably @JDevlieghere or @adrian-prantl

I think you've all gone above and beyond to pay for the (marginal) cost increase from this feature, and we should go ahead and land this.

Thanks for all the hard work and data gathering!

Thanks all for the comments!

In addition to the cc'd apple folks, @labath do you have an opinion on the topic of enabling this by default for all debuggers versus not doing so for LLDB tuning? (There's a question in the summary, and it's been discussed a little in the comments).

One less exciting question that remains, if anyone has any ideas off the top of their heads:

Clang languages/extensions - I've implemented support for C/C++, but I've not tested or specifically implemented anything for other languages (?) or extensions (?) like CUDA. Is there a good way to filter for this in the driver code in this patch?

Ideally, I'd also go with the uniform approach of enabling the feature everywhere. It's true that this doesn't have any value for lldb right now, but it also shouldn't break anything (lldb ignores is_stmt), and it's not really true that this (disabling the flag on -glldb) will completely avoid having to deal with this input as it's fairly common to compile linux binaries with -ggdb (the default) and debug with lldb.

Enabling the flag may also provide the incentive for someone to add support for this to lldb (I wish that someone was me, but it looks like that's not going to happen). Even if that happens a year from now, it would be nice if we could say that all binaries compiled within the last year would benefit from that.

Thanks everyone for the discussion. It sounds like the consensus so far is to enable for all debugger tunings by default, for "plain" C/C++ if optimisations are enabled (though no apple folks have chimed in).

I've added a check to filter out not-plain C/C++. Please could someone review the code and approve if they're happy?