[PATCH] D8506: Set a debugger "target" to guide DWARF choices

[Eric Christopher]

echristo added a comment.

Replied inline. Some of this probably feels a bit vague, let me know if you've got any questions :)

-eric


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:234-235
@@ -211,4 +233,4 @@
 
   // Turn on accelerator tables for Darwin by default, pubnames by
-  // default for non-Darwin/PS4, and handle split dwarf.
+  // default for GDB, and handle split dwarf.
   if (DwarfAccelTables == Default)
----------------
Yeah, anything you're changing under the default (or triple) mode should go into a separate patch I'd think?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:234-242
@@ -211,11 +233,11 @@
 
   // Turn on accelerator tables for Darwin by default, pubnames by
-  // default for non-Darwin/PS4, and handle split dwarf.
+  // default for GDB, and handle split dwarf.
   if (DwarfAccelTables == Default)
     HasDwarfAccelTables = IsDarwin;
   else
     HasDwarfAccelTables = DwarfAccelTables == Enable;
 
   if (SplitDwarf == Default)
     HasSplitDwarf = false;
   else
----------------
echristo wrote:
> Yeah, anything you're changing under the default (or triple) mode should go into a separate patch I'd think?
I've actually come around to thoughts of a triple based set of defaults. I.e.  go ahead and use the triple to default behaviors like this and use the tuning parameter otherwise. i.e. feature based on the default debugger for the platform. It'll involve splitting this patch up a bit (see previous comment) but that seems reasonable yes?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:237
@@ -214,3 +236,3 @@
   if (DwarfAccelTables == Default)
     HasDwarfAccelTables = IsDarwin;
   else
----------------
For now yes. Post-dwarf5 it'll be more interesting.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:257
@@ -234,3 +256,3 @@
   // Everybody else uses GNU's.
-  UseGNUTLSOpcode = !(IsDarwin || IsPS4) || DwarfVersion < 3;
+  UseGNUTLSOpcode = !(IsDarwin || TT.isPS4CPU()) || DwarfVersion < 3;
 
----------------
Yeah. I agree with Adrian here. Let's use "isDebuggerLLDB()".

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:259
@@ -236,3 +258,3 @@
 
   Asm->OutStreamer->getContext().setDwarfVersion(DwarfVersion);
 
----------------
Yeah, let's put it with tuning with a what you just said as a comment (if you have a gdb bug to point at that'd be good).

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:572-589
@@ -561,1 +571,20 @@
 
+  // The "debugger tuning" concept allows us to present a more intuitive
+  // interface that unpacks into different sets of defaults for the various
+  // individual feature-flag settings, that suit the preferences of the
+  // various debuggers.  However, it's worth remembering that debuggers are
+  // not the only consumers of debug info, and some variations in DWARF might
+  // better be treated as target/platform issues. Fundamentally,
+  // o if the feature is useful (or not) to a particular debugger, regardless
+  //   of the target, that's a tuning decision;
+  // o if the feature is useful (or not) on a particular platform, regardless
+  //   of the debugger, that's a target decision.
+  // It's not impossible to see both factors in some specific case.
+  //
+  /// \defgroup DebuggerTuning Predicates to tune DWARF for a given debugger.
+  ///
+  /// The "tuning" predicates should be used to set defaults for individual
+  /// feature flags in DwarfDebug; if a given feature has a more specific
+  /// command-line option, that option should take precedence over the tuning.
+  /// @{
+  bool tuneDebugForGDB() const { return DebuggerTuning == DebuggerKind::GDB; }
----------------
This could all go with the enum.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:590-592
@@ +589,5 @@
+  /// @{
+  bool tuneDebugForGDB() const { return DebuggerTuning == DebuggerKind::GDB; }
+  bool tuneDebugForLLDB() const { return DebuggerTuning == DebuggerKind::LLDB; }
+  bool tuneDebugForSCE() const { return DebuggerTuning == DebuggerKind::SCE; }
+  /// @}
----------------
Possibly rename without using Debug. We're in the debug code already.


http://reviews.llvm.org/D8506