[DWARF] Omitting the explicit import of an anonymous namespace is a debugger-tuning decision, not a target decision.

[probinson]

Summary

GDB and LLDB don't mind if we omit the explicit import of an anonymous namespace.

[probinson]

Ping. I did want to get this finished off (moving all triple-based decisions to tuning-based decisions) before 3.8 branches.

[echristo]

lib/Frontend/CompilerInvocation.cpp

363–370

This seems like overkill for a single case.

[echristo]

lib/Frontend/CompilerInvocation.cpp

428–429

Why not just a positive for debugger tuning SCE?

[probinson]

lib/Frontend/CompilerInvocation.cpp

363–370

I suppose... having the big hairy test inline isn't *that* unreadable.

428–429

Because the default (i.e., no tuning specified) behavior should be to conform to the DWARF spec, which basically says you need the explicit import. There's a new extra RUN line in the test, with no tuning specified, to verify this.
GDB and LLDB are the oddballs here, they implement a special case for namespaces whose name meets certain criteria, and do something beyond what DWARF says to do. So, the condition is written to express that.

[probinson]

FTR, the previous wiring-up-thru-Clang patches put the there-must-be-a-tuning decision in the driver; CC1 does not assume or require a non-default tuning.

[echristo]

lib/Frontend/CompilerInvocation.cpp

428–429

I don't necessarily agree with that interpretation on the explicit import - I did skim the thread, perhaps you could highlight what makes you think this?

[probinson]

lib/Frontend/CompilerInvocation.cpp

428–429

Basically, a namespace is a "context" for declarations, and the DWARF mechanism for making declarations from one context available in another context is with DW_TAG_imported_declaration and DW_TAG_imported_module.
The C++ spec describes the behavior "as if" there was an explicit using directive, and DW_TAG_imported_module is the DWARF mechanism for describing a using directive.

The meaning of DWARF is determined by the DWARF spec, not the C++ spec, and the DWARF spec does not say there's anything special about a namespace that has no name. There is a perfectly reasonable DWARF mechanism for getting the desired effect, so there's no reason for DWARF to make a special rule for an unnamed namespace. Therefore, an anonymous namespace should be explicitly imported into the containing namespace. The explicit import would be marked artificial of course.

[probinson]

lib/Frontend/CompilerInvocation.cpp

428–429

Ping. Have I missed something in the DWARF spec that makes you think my interpretation is incorrect? Wouldn't be the first time...

[probinson]

Ping.

[probinson]

Ping. FWIW in today's DWARF meeting, we agreed that the Appendix D example of an anonymous namespace should have an explicit import from the containing scope (see DWARF v4, Figure 57 on p.233; it was essentially unchanged in the v5 draft).

[probinson]

Ping #4.

[echristo]

lib/Frontend/CompilerInvocation.cpp

428–429

I don't have anything to add to the reasoning the David has given you. We both agree and let's just make this a positive tuning for SCE.

[probinson]

lib/Frontend/CompilerInvocation.cpp

428–429

Fine. It'll be on just for SCE.
// Pedantic DWARF requires explicit import but only SCE insists.

[echristo]

lib/Frontend/CompilerInvocation.cpp

428–429

Please don't add that comment to this, I don't believe that it is valid or useful.

[probinson]

lib/Frontend/CompilerInvocation.cpp

428–429

The DWARF committee disagrees with your validity opinion, but I will take the comment out.

[echristo]

lib/Frontend/CompilerInvocation.cpp

428–429

Bring it up on the list then.

[probinson]

lib/Frontend/CompilerInvocation.cpp

428–429

I brought it up during the document review; see my ping comment from Jan 28.

[echristo]

lib/Frontend/CompilerInvocation.cpp

428–429

I saw. If you want that to be a required element of the spec then we need to change multiple wordings in the DWARF spec. I think the right place to bring that up is with the committee. The example is perfectly valid DWARF with the import, it's just unnecessary.

[probinson]

Abandoning dead patch. This wound up being done a different way.

lib/Frontend/CompilerInvocation.cpp

for (const auto &Sanitizer : Sanitizers) {
     SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
     if (K == 0)
       Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
     else
       S.set(K, true);
   }
 }
 
+// Convenience functions for debugger tuning.
+static bool tuneForGDB(CodeGenOptions &Opts) {
+  return Opts.getDebuggerTuning() == CodeGenOptions::DebuggerKindGDB;
+}
+static bool tuneForLLDB(CodeGenOptions &Opts) {
+  return Opts.getDebuggerTuning() == CodeGenOptions::DebuggerKindLLDB;
+}
+
 static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
                              DiagnosticsEngine &Diags,
                              const TargetOptions &TargetOpts) {
   using namespace options;
   bool Success = true;
   llvm::Triple Triple = llvm::Triple(TargetOpts.Triple);
 
   unsigned OptimizationLevel = getOptimizationLevel(Args, IK, Diags);
Opts.setDebuggerTuning(
             .Case("lldb", CodeGenOptions::DebuggerKindLLDB)
             .Case("sce", CodeGenOptions::DebuggerKindSCE));
   }
   Opts.DwarfVersion = getLastArgIntValue(Args, OPT_dwarf_version_EQ, 0, Diags);
   Opts.DebugColumnInfo = Args.hasArg(OPT_dwarf_column_info);
   Opts.EmitCodeView = Args.hasArg(OPT_gcodeview);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
-  Opts.DebugExplicitImport = Triple.isPS4CPU(); 
+  Opts.DebugExplicitImport = !tuneForGDB(Opts) && !tuneForLLDB(Opts);
 
   for (const auto &Arg : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ))
     Opts.DebugPrefixMap.insert(StringRef(Arg).split('='));
 
   if (const Arg *A =
           Args.getLastArg(OPT_emit_llvm_uselists, OPT_no_emit_llvm_uselists))
     Opts.EmitLLVMUseLists = A->getOption().getID() == OPT_emit_llvm_uselists;
 
   Opts.DisableLLVMOpts = Args.hasArg(OPT_disable_llvm_optzns);

test/CodeGenCXX/debug-info-anon-namespace.cpp

-// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -triple x86_64-scei-ps4 -O0 %s -o - | FileCheck --check-prefix=PS4 %s
-// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -triple x86_64-unknown-linux-gnu -O0 %s -o - | FileCheck --check-prefix=NON-PS4 %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited -debugger-tuning=gdb -O0 %s -o - | FileCheck --check-prefix=NOIMPORT %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited -debugger-tuning=lldb -O0 %s -o - | FileCheck --check-prefix=NOIMPORT %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited -debugger-tuning=sce -O0 %s -o - | FileCheck --check-prefix=IMPORT %s
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -debug-info-kind=limited -O0 %s -o - | FileCheck --check-prefix=IMPORT %s
 
 namespace
 {
   int a = 5;
 }
 int *b = &a;
 
 namespace
 {
   namespace {
     int a1 = 5;
   }
   int a2 = 7;
 }
 int *b1 = &a1;
 int *b2 = &a2;
 
 
-// PS4:  [[NS:![0-9]+]] = !DINamespace
-// PS4:  [[NS2:![0-9]+]] = !DINamespace
-// PS4: !DIImportedEntity(tag: DW_TAG_imported_module, scope: !0, entity: [[NS]])
-// PS4: !DIImportedEntity(tag: DW_TAG_imported_module, scope: [[NS]], entity: [[NS2]], line: {{[0-9]+}})
-// NON-PS4-NOT: !DIImportedEntity
+// IMPORT:  [[NS:![0-9]+]] = !DINamespace
+// IMPORT:  [[NS2:![0-9]+]] = !DINamespace
+// IMPORT: !DIImportedEntity(tag: DW_TAG_imported_module, scope: !0, entity: [[NS]])
+// IMPORT: !DIImportedEntity(tag: DW_TAG_imported_module, scope: [[NS]], entity: [[NS2]], line: {{[0-9]+}})
+// NOIMPORT-NOT: !DIImportedEntity