[DWARFVerifier] Fix verification of nameless variables.

[fdeazeve]

Summary

The verifier builds a list of all "possible" names of a variable and then
verifies that the name inside __debug_names is one of those possible names.
For a nameless variable, the list is empty, but its name in the accelerator
table is "", so that logic fails.

One could also question whether accelerator table entries with an empty name
should be valid to begin with. The specification says that:

• All other debugging information entries without a DW_AT_name attribute are
• excluded.

But it also says that:

• DW_TAG_variable debugging information entries with a DW_AT_location attribute
• that includes a DW_OP_addr or DW_OP_form_tls_address operator are included;
• otherwise, they are excluded.

So it's not clear which rules takes precedence. Changing the logic to skip
generating entries in this case is likely more complex and probably deserves its
own discussion, as such this commit proposes making the verifier "clean" first:
its logic is arguably buggy and if we want to forbid empty names the verifier
implementation should check for that explicitly, not in this very implicit way.

[fdeazeve]

You can repro the issue with:

echo 'auto x = "hello!";' | clang++ -x c++ - -c -gdwarf-5 -o - | llvm-dwarfdump --debug-names - --verify

[JDevlieghere]

I can't speak for the committee, but my person interpretation of the standard is that these variables should not be included. I get that from both the order in which those points are define as well as the general spirit behind the accelerated access (i.e. looking things up by name).

It's fine to patch the verifier like this, but let's start a thread on the dwarf-discuss mailing list about the generation side.

[dblaikie]

But maybe the deeper question: Why are we creating DW_AT_variables for string literals? They aren't variables as far as a user would be concerned. GCC doesn't do this...

[fdeazeve]

In D153550#4441881, @dblaikie wrote:

But maybe the deeper question: Why are we creating DW_AT_variables for string literals? They aren't variables as far as a user would be concerned. GCC doesn't do this...

This is either an accident or some obscure historical reason, I'll try to do some git archeology to figure out

[fdeazeve]

In D153550#4441881, @dblaikie wrote:

But maybe the deeper question: Why are we creating DW_AT_variables for string literals? They aren't variables as far as a user would be concerned. GCC doesn't do this...

@dblaikie apparently this was done last year: https://reviews.llvm.org/D123534

[dblaikie]

In D153550#4448765, @fdeazeve wrote:

In D153550#4441881, @dblaikie wrote:

But maybe the deeper question: Why are we creating DW_AT_variables for string literals? They aren't variables as far as a user would be concerned. GCC doesn't do this...

@dblaikie apparently this was done last year: https://reviews.llvm.org/D123534

ah, right... - thanks for the reference! & then, yeah, just not putting these things in the index seems good. (even if we did revisit the original decision to include this debug info unconditionally - adding it under a flag, etc, we'd still want to fix the index issue)

[fdeazeve]

Abandoning in favor of D153550

[scott.linder]

In D153550#4450315, @fdeazeve wrote:

Abandoning in favor of D153550

For posterity, I think this is a copy-paste typo, and meant to link to D153809 (?)

[fdeazeve]

In D153550#4453242, @scott.linder wrote:

In D153550#4450315, @fdeazeve wrote:

Abandoning in favor of D153550

For posterity, I think this is a copy-paste typo, and meant to link to D153809 (?)

Oooops, thank you for catching this, it was indeed a copy-paste mistake.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

if (DIE.getTag() != EntryOr->tag()) {
       error() << formatv("Name Index @ {0:x}: Entry @ {1:x}: mismatched Tag of "
                          "DIE @ {2:x}: index - {3}; debug_info - {4}.\n",
                          NI.getUnitOffset(), EntryID, DIEOffset, EntryOr->tag(),
                          DIE.getTag());
       ++NumErrors;
     }
 
     auto EntryNames = getNames(DIE);
+    // If the entry has no name, its name in the table will be "".
+    if (EntryNames.empty())
+      EntryNames.push_back("");
     if (!is_contained(EntryNames, Str)) {
       error() << formatv("Name Index @ {0:x}: Entry @ {1:x}: mismatched Name "
                          "of DIE @ {2:x}: index - {3}; debug_info - {4}.\n",
                          NI.getUnitOffset(), EntryID, DIEOffset, Str,
                          make_range(EntryNames.begin(), EntryNames.end()));
       ++NumErrors;
     }
   }

llvm/test/DebugInfo/Generic/debug-names-nameless-var.ll

+; RUN: %llc_dwarf -debugger-tune=lldb -accel-tables=Dwarf -filetype=obj -o %t %s
+; RUN: llvm-dwarfdump -debug-names %t | FileCheck %s
+; RUN: llvm-dwarfdump -debug-names -verify %t | FileCheck --check-prefix=VERIFY %s
+
+@nameless_var = constant i8 0, !dbg !0
+
+; CHECK: Name count: 1
+; CHECK: String: 0x{{[0-9a-f]*}} ""
+; VERIFY: No errors
+
+!llvm.dbg.cu = !{!10}
+!llvm.module.flags = !{!14, !15}
+!llvm.ident = !{!20}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(scope: null, file: !11, line: 1, type: !5, isLocal: true, isDefinition: true)
+!5 = !DIBasicType(size: 8, encoding: DW_ATE_signed_char)
+!10 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !11, producer: "blah", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, globals: !12, splitDebugInlining: false, nameTableKind: Apple, sysroot: "/")
+!11 = !DIFile(filename: "blah", directory: "blah")
+!12 = !{!0}
+!14 = !{i32 7, !"Dwarf Version", i32 5}
+!15 = !{i32 2, !"Debug Info Version", i32 3}
+!20 = !{!"blah"}