[DWARF] Avoid entry_values production for SCE

[djtodoro]

Summary

SONY debugger does not prefer debug entry values feature, so the plan is to avoid production of the entry values by default when the tuning is SCE debugger.

The feature still can be enabled with the -debug-entry-values option for the testing/development purposes.

This patch addresses PR46643.

[probinson]

Between this code and DwarfDebug, I'm having a hard time understanding the various controls over emitting entry-values.

• There's an existing tuning check in DwarfDebug, now you're adding the complementary check in TargetOptions. Does this make the DwarfDebug check redundant?
• There are 3 different flags to sort out whether to emit this stuff. SupportsDebugEntryValues is strictly based on the target, I get that. Then there's EnableDebugEntryValues in the TargetOptions, which seems to be tied to a tool-command-line option; and separately there's EmitDwarfDebugEntryValues in DwarfDebug, with its own command-line option. Can we simplify this at all?

[djtodoro]

In D83462#2141719, @probinson wrote:

Between this code and DwarfDebug, I'm having a hard time understanding the various controls over emitting entry-values.

I see.. I think we can/should simplify it.

• There's an existing tuning check in DwarfDebug, now you're adding the complementary check in TargetOptions. Does this make the DwarfDebug check redundant?

It is is redundant now.. Thanks!

• There are 3 different flags to sort out whether to emit this stuff. SupportsDebugEntryValues is strictly based on the target, I get that. Then there's EnableDebugEntryValues in the TargetOptions, which seems to be tied to a tool-command-line option; and separately there's EmitDwarfDebugEntryValues in DwarfDebug, with its own command-line option. Can we simplify this at all?

I think yes. We needed the first two (SupportsDebugEntryValues and EnableDebugEntryValues from TargetOptions) in order to enable it by default for some targets and to leave an experimental option for developers who want to extend the support for the other architectures. There is a situation when different debuggers come into the game, so we thought to control it on the very end (within DwarfDebug with the EmitDwarfDebugEntryValues), but at that place we can control only caller-side-dwarf-symbols (dw_tag_call_sites) but not the dw_entry_values (callee side; since it is being produced during LiveDebugValues phase). So, having this patch, we can get rid of the DwarfDebug::EmitDwarfDebugEntryValues option, since we can experiment everything with the fields from TargetOptions.

[djtodoro]

• Remove unnecessary DwarfDebug entry-values option

[vsk]

+1 for removing EmitDwarfDebugEntryValues, that's a nice cleanup. Thanks!

[echristo]

So the tuning here for SCE is also a "does not support" or something else?

[probinson]

In D83462#2142036, @echristo wrote:

So the tuning here for SCE is also a "does not support" or something else?

I'm told our debugger currently does not support the entry-value opcode. Locations with that opcode would be dropped/ignored (presumably just the individual location-list elements, although I haven't verified).

I am nudging our folks in the direction of supporting it, but need to demonstrate value first; that the additional location descriptions increase availability, and that the expressions can be evaluated usefully. I have to say a few preliminary experiments don't make me feel too positive about that second part, as the entry-value expressions tend to rely on registers that aren't saved by the ABI, so unwinding won't recover them.

In the meantime I'd prefer that they weren't emitted. for the usual size reasons.

[djtodoro]

• Update the comment around ShouldEmitDebugEntryValues()

[aprantl]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

429 ↗

(On Diff #277690)

Wouldn't we want (tuneForGDB() || tuneForLLDB()) to override the target default? If someone were to debug with a hypothetical GDB port for SCE targets, for example?

llvm/test/DebugInfo/MIR/X86/DW_OP_entry_value.mir

18

Wouldn't you want to check for DW_OP_{{.*}}entry_value?

[djtodoro]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

429 ↗

(On Diff #277690)

We can still do that by using the -debug-entry-values option?

llvm/test/DebugInfo/MIR/X86/DW_OP_entry_value.mir

18

It makes sense, thanks! The default @entry encoding for SCE will be GNU in this case, but it is safe to check both in the case the behavior changes.

[djtodoro]

• Improve the test

[djtodoro]

Any additional comment on this? :)

[probinson]

LGTM

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

429 ↗

(On Diff #277690)

The tuning decision is made in TargetOptionsImpl, which is unusual but that's where the tuning option is actually kept, so I think it's okay.

llvm/lib/CodeGen/TargetOptionsImpl.cpp

 /// that the rounding mode of the FPU can change from its default.
 bool TargetOptions::HonorSignDependentRoundingFPMath() const {
   return !UnsafeFPMath && HonorSignDependentRoundingFPMathOption;
 }
 
 /// NOTE: There are targets that still do not support the debug entry values
 /// production.
 bool TargetOptions::ShouldEmitDebugEntryValues() const {
-  return SupportsDebugEntryValues || EnableDebugEntryValues;
+  return (SupportsDebugEntryValues && DebuggerTuning != DebuggerKind::SCE) ||
+         EnableDebugEntryValues;
 }

llvm/test/DebugInfo/MIR/X86/DW_OP_entry_value.mir

 # RUN: llc -start-before=livedebugvalues -mtriple=x86_64-apple-darwin -o %t %s -filetype=obj
 # RUN: llvm-dwarfdump %t | FileCheck %s
-#
-# int global;
-# int foo(int p, int q, int r) {
-#   global = p + 1;
-#   asm __volatile("" : : : "edi", "esi", "edx");
-#   return 123;
-# }
+
+# RUN: llc -start-before=livedebugvalues -debugger-tune=sce -mtriple=x86_64-sce-ps4 -o %t1 %s -filetype=obj
+# RUN: llvm-dwarfdump %t1 | FileCheck %s -check-prefix=SCE
+
+## Based on:
+## int global;
+## int foo(int p, int q, int r) {
+##   global = p + 1;
+##   asm __volatile("" : : : "edi", "esi", "edx");
+##   return 123;
+## }
 
 # CHECK: DW_TAG_formal_parameter
 # CHECK:   DW_OP_entry_value
 
+# SCE-NOT: DW_OP_GNU_entry_value
+
 --- |
   ; ModuleID = 'multiple-param-dbg-value-entry.ll'
   source_filename = "multiple-param-dbg-value-entry.c"
   target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
   @global = common dso_local local_unnamed_addr global i32 0, align 4, !dbg !0
 
   define dso_local i32 @foo(i32 %p, i32 %q, i32 %r) local_unnamed_addr !dbg !11 {