Assert that a subprogram should have a name when parsing DWARF

[shafik]

Summary

This is just an enforcement of the DWARF requirement that a DW_TAG_subprogram should have a DW_AT_name.

This came up when updating how we generating some debug info and one of the possible change caused several LLDB tests to fail. This was ultimately due to subprograms being generated without names but the immediate symptom did not point to that.

[aprantl]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

832

The message should be:

"DWARF validation error: DW_TAG_subprogram without DW_AT_name"

Can you double-check that we don't already have an error reporting mechanism for malformed debug info?

833

"Subprograms require a name" raises more questions than it answers:

• does that mean that LLDB will crash when this happens?
• since there is an assertion it definitely means that this code path is untested ...

If LLDB doesn't crash, then perhaps say something like:

"this is a bug in the producer"

In any case you need to be prepared for the possibility that somebody will find a compiler out in the wild that produces this kind of DWARF and will ask you to remove the assertion again. So it's probably better to leave this out.

[aprantl]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

832

Looks like other places use GetObjectFile()->GetModule()->ReportError() for this.

[JDevlieghere]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

833

Using an assertion for invalid input goes against the assertion manifesto on https://lldb.llvm.org/resources/contributing.html

[davide]

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

[davide]

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

[labath]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

833

Using an assertion for invalid input goes against the assertion manifesto on https://lldb.llvm.org/resources/contributing.html

+100

If you want to surface this somehow you can use the Module::ReportError function.

[shafik]

Everyone has brought up great feedback, let me go back and revise this.

[shafik]

Updated approach based on comments and added test for the new approach.

[shafik]

I will try to look into dwarfdump --verify separately.

[JDevlieghere]

Given that this error is non-actionable, I don't see any value in diagnosing this LLDB. It is important to have this in dwarfdump, which does not detect this right now.

It might be interesting to have LLDB run in a sort of "pedantic" mode which verifies all the DWARF it consumes with the dwarf verifier in LLVM. We have something similar in dsymutil which runs the verifier over the generated dSYM.

[jingham]

In D73921#1855961, @davide wrote:

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

Many

In D73921#1868191, @JDevlieghere wrote:

Given that this error is non-actionable, I don't see any value in diagnosing this LLDB. It is important to have this in dwarfdump, which does not detect this right now.

It might be interesting to have LLDB run in a sort of "pedantic" mode which verifies all the DWARF it consumes with the dwarf verifier in LLVM. We have something similar in dsymutil which runs the verifier over the generated dSYM.

Note that many OS X developers never debug a dSYM build of their project. They debug with .o files, then make a dSYM when they do their release builds. And they probably don't look at the output of dsymutil amidst all the noise of a build. So if we only do this in dsymutil, we are greatly narrowing the range of folks who might see & report this error to us.

[JDevlieghere]

In D73921#1868228, @jingham wrote:

In D73921#1855961, @davide wrote:

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

Many

In D73921#1868191, @JDevlieghere wrote:

Given that this error is non-actionable, I don't see any value in diagnosing this LLDB. It is important to have this in dwarfdump, which does not detect this right now.

It might be interesting to have LLDB run in a sort of "pedantic" mode which verifies all the DWARF it consumes with the dwarf verifier in LLVM. We have something similar in dsymutil which runs the verifier over the generated dSYM.

Note that many OS X developers never debug a dSYM build of their project. They debug with .o files, then make a dSYM when they do their release builds. And they probably don't look at the output of dsymutil amidst all the noise of a build. So if we only do this in dsymutil, we are greatly narrowing the range of folks who might see & report this error to us.

I think you misunderstood my suggestion. I'm not saying that we should limit this to dsymutil. I'm saying that dsymutil has a mode where it verifies the dSYM it just created. It's entirely optional and you have to pass --verify to enable it. I suggest we have something similar in LLDB, where we have a pedantic mode that, when enabled, checks all the DWARF it reads with the DWARF verifier.

As discussed offline with Shafik, I prefer this over the current approach for a few reasons:

• It would make this behavior opt-in. Verifying the DWARF can be expensive and not every user has control over the debug info it reads. It should be possible to silence these warnings if they don't change LLDB's behavior.
• It would provide much better coverage than some ad-hoc checks. Currently, not getting these kind of errors form LLDB doesn't tell you much. We may or may not have a check for a particular kind of invalid DWARF, so to be sure you'd still have to run it through dwarfdump -verify.
• It would mean we only have to maintain a single DWARF verifier, which is already tested extensively.
• It fits with our long-term goal of moving to LLVM's DWARF parser.

[davide]

In D73921#1868286, @JDevlieghere wrote:

In D73921#1868228, @jingham wrote:

In D73921#1855961, @davide wrote:

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

Many

In D73921#1868191, @JDevlieghere wrote:

Given that this error is non-actionable, I don't see any value in diagnosing this LLDB. It is important to have this in dwarfdump, which does not detect this right now.

It might be interesting to have LLDB run in a sort of "pedantic" mode which verifies all the DWARF it consumes with the dwarf verifier in LLVM. We have something similar in dsymutil which runs the verifier over the generated dSYM.

Note that many OS X developers never debug a dSYM build of their project. They debug with .o files, then make a dSYM when they do their release builds. And they probably don't look at the output of dsymutil amidst all the noise of a build. So if we only do this in dsymutil, we are greatly narrowing the range of folks who might see & report this error to us.

I think you misunderstood my suggestion. I'm not saying that we should limit this to dsymutil. I'm saying that dsymutil has a mode where it verifies the dSYM it just created. It's entirely optional and you have to pass --verify to enable it. I suggest we have something similar in LLDB, where we have a pedantic mode that, when enabled, checks all the DWARF it reads with the DWARF verifier.

As discussed offline with Shafik, I prefer this over the current approach for a few reasons:

• It would make this behavior opt-in. Verifying the DWARF can be expensive and not every user has control over the debug info it reads. It should be possible to silence these warnings if they don't change LLDB's behavior.
• It would provide much better coverage than some ad-hoc checks. Currently, not getting these kind of errors form LLDB doesn't tell you much. We may or may not have a check for a particular kind of invalid DWARF, so to be sure you'd still have to run it through dwarfdump -verify.
• It would mean we only have to maintain a single DWARF verifier, which is already tested extensively.
• It fits with our long-term goal of moving to LLVM's DWARF parser.

I second this motion.
Realistically what this patch is currently doing is diagnosing a very narrow problem emitting a somewhat obscure diagnostic for users. People who use debuggers aren't necessarily asked to understand DWARF.
If we really want to move towards a verification mode, the plan suggested above is much more reasonable than having piecemeal diagnostic sprinkled over the parser.

[shafik]

I am going to abandon this change b/c the consensus seems to be that we want a different solution and I don't know how much work would require ATM but I may revisit another time,

I will note that we do currently have a lot of these and they have in the past be very helpful solving problems but clearly the bar is higher now for new ones.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

 TypeSP DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
                            ParsedDWARFTypeAttributes &attrs) {
   Log *log(LogChannelDWARF::GetLogIfAny(DWARF_LOG_TYPE_COMPLETION |
                                         DWARF_LOG_LOOKUPS));
 
   SymbolFileDWARF *dwarf = die.GetDWARF();
   const dw_tag_t tag = die.Tag();
 
+  if (tag == DW_TAG_subprogram && !attrs.name) {
+    dwarf->GetObjectFile()->GetModule()->ReportError(
+        "DWARF validation error: DW_TAG_subprogram without DW_AT_name");
+  }
+
   bool is_variadic = false;
   bool is_static = false;
   bool has_template_params = false;
 
   unsigned type_quals = 0;
 
   std::string object_pointer_name;
   if (attrs.object_pointer) {

lldb/test/Shell/SymbolFile/DWARF/missing_dw_at_name_error.s

+# This test verifies that we catch that a subprogram is missing a DW_AT_name
+# during processing.
+
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple x86_64-apple-macosx10.14.0  %s -filetype=obj > %t.o
+# RUN: lldb-test symbols --dump-clang-ast %t.o 2>&1 | FileCheck %s
+
+# CHECK: DWARF validation error: DW_TAG_subprogram without DW_AT_name
+
+# Generated from:
+#
+# int f() {
+#  return 1;
+# }
+#
+# The debug-info was modified by hand.
+  .section  __TEXT,__text,regular,pure_instructions
+  .build_version macos, 10, 14  sdk_version 10, 14
+  .globl  __Z1fv                  ## -- Begin function _Z1fv
+  .p2align  4, 0x90
+__Z1fv:                                 ## @_Z1fv
+Lfunc_begin0:
+  .file 1 "/Users/shafik/code" "simple_function.cpp"
+  .loc  1 1 0                   ## simple_function.cpp:1:0
+  .cfi_startproc
+## %bb.0:
+  pushq %rbp
+  .cfi_def_cfa_offset 16
+  .cfi_offset %rbp, -16
+  movq  %rsp, %rbp
+  .cfi_def_cfa_register %rbp
+Ltmp0:
+  .loc  1 2 3 prologue_end      ## simple_function.cpp:2:3
+  movl  $1, %eax
+  popq  %rbp
+  retq
+Ltmp1:
+Lfunc_end0:
+  .cfi_endproc
+                                        ## -- End function
+  .section  __DWARF,__debug_str,regular,debug
+Linfo_string:
+  .asciz  "Apple clang version 11.0.0 (clang-1100.0.31.5)" ## string offset=0
+  .asciz  "simple_function.cpp"   ## string offset=47
+  .asciz  "/Users/shafik/code"    ## string offset=67
+  .asciz  "f"                     ## string offset=86
+  .asciz  "_Z1fv"                 ## string offset=88
+  .asciz  "int"                   ## string offset=94
+  .section  __DWARF,__debug_abbrev,regular,debug
+Lsection_abbrev:
+  .byte 1                       ## Abbreviation Code
+  .byte 17                      ## DW_TAG_compile_unit
+  .byte 1                       ## DW_CHILDREN_yes
+  .byte 37                      ## DW_AT_producer
+  .byte 14                      ## DW_FORM_strp
+  .byte 19                      ## DW_AT_language
+  .byte 5                       ## DW_FORM_data2
+  .byte 3                       ## DW_AT_name
+  .byte 14                      ## DW_FORM_strp
+  .byte 16                      ## DW_AT_stmt_list
+  .byte 23                      ## DW_FORM_sec_offset
+  .byte 27                      ## DW_AT_comp_dir
+  .byte 14                      ## DW_FORM_strp
+  .ascii  "\264B"                 ## DW_AT_GNU_pubnames
+  .byte 25                      ## DW_FORM_flag_present
+  .byte 17                      ## DW_AT_low_pc
+  .byte 1                       ## DW_FORM_addr
+  .byte 18                      ## DW_AT_high_pc
+  .byte 6                       ## DW_FORM_data4
+  .byte 0                       ## EOM(1)
+  .byte 0                       ## EOM(2)
+  .byte 2                       ## Abbreviation Code
+  .byte 46                      ## DW_TAG_subprogram
+  .byte 0                       ## DW_CHILDREN_no
+  .byte 17                      ## DW_AT_low_pc
+  .byte 1                       ## DW_FORM_addr
+  .byte 18                      ## DW_AT_high_pc
+  .byte 6                       ## DW_FORM_data4
+  .byte 64                      ## DW_AT_frame_base
+  .byte 24                      ## DW_FORM_exprloc
+  .byte 110                     ## DW_AT_linkage_name
+  .byte 14                      ## DW_FORM_strp
+  .byte 58                      ## DW_AT_decl_file
+  .byte 11                      ## DW_FORM_data1
+  .byte 59                      ## DW_AT_decl_line
+  .byte 11                      ## DW_FORM_data1
+  .byte 73                      ## DW_AT_type
+  .byte 19                      ## DW_FORM_ref4
+  .byte 63                      ## DW_AT_external
+  .byte 25                      ## DW_FORM_flag_present
+  .byte 0                       ## EOM(1)
+  .byte 0                       ## EOM(2)
+  .byte 3                       ## Abbreviation Code
+  .byte 36                      ## DW_TAG_base_type
+  .byte 0                       ## DW_CHILDREN_no
+  .byte 3                       ## DW_AT_name
+  .byte 14                      ## DW_FORM_strp
+  .byte 62                      ## DW_AT_encoding
+  .byte 11                      ## DW_FORM_data1
+  .byte 11                      ## DW_AT_byte_size
+  .byte 11                      ## DW_FORM_data1
+  .byte 0                       ## EOM(1)
+  .byte 0                       ## EOM(2)
+  .byte 0                       ## EOM(3)
+  .section  __DWARF,__debug_info,regular,debug
+Lsection_info:
+Lcu_begin0:
+.set Lset0, Ldebug_info_end0-Ldebug_info_start0 ## Length of Unit
+  .long Lset0
+Ldebug_info_start0:
+  .short  4                       ## DWARF version number
+.set Lset1, Lsection_abbrev-Lsection_abbrev ## Offset Into Abbrev. Section
+  .long Lset1
+  .byte 8                       ## Address Size (in bytes)
+  .byte 1                       ## Abbrev [1] 0xb:0x44 DW_TAG_compile_unit
+  .long 0                       ## DW_AT_producer
+  .short  4                       ## DW_AT_language
+  .long 47                      ## DW_AT_name
+.set Lset2, Lline_table_start0-Lsection_line ## DW_AT_stmt_list
+  .long Lset2
+  .long 67                      ## DW_AT_comp_dir
+                                        ## DW_AT_GNU_pubnames
+  .quad Lfunc_begin0            ## DW_AT_low_pc
+.set Lset3, Lfunc_end0-Lfunc_begin0     ## DW_AT_high_pc
+  .long Lset3
+  .byte 2                       ## Abbrev [2] 0x2a:0x1d DW_TAG_subprogram
+  .quad Lfunc_begin0            ## DW_AT_low_pc
+.set Lset4, Lfunc_end0-Lfunc_begin0     ## DW_AT_high_pc
+  .long Lset4
+  .byte 1                       ## DW_AT_frame_base
+  .byte 86
+  .long 88                      ## DW_AT_linkage_name
+  .byte 1                       ## DW_AT_decl_file
+  .byte 1                       ## DW_AT_decl_line
+  .long 71                      ## DW_AT_type
+                                        ## DW_AT_external
+  .byte 3                       ## Abbrev [3] 0x47:0x7 DW_TAG_base_type
+  .long 94                      ## DW_AT_name
+  .byte 5                       ## DW_AT_encoding
+  .byte 4                       ## DW_AT_byte_size
+  .byte 0                       ## End Of Children Mark
+Ldebug_info_end0:
+  .section  __DWARF,__debug_macinfo,regular,debug
+Ldebug_macinfo:
+  .byte 0                       ## End Of Macro List Mark
+  .section  __DWARF,__apple_objc,regular,debug
+Lobjc_begin:
+  .long 1212240712              ## Header Magic
+  .short  1                       ## Header Version
+  .short  0                       ## Header Hash Function
+  .long 1                       ## Header Bucket Count
+  .long 0                       ## Header Hash Count
+  .long 12                      ## Header Data Length
+  .long 0                       ## HeaderData Die Offset Base
+  .long 1                       ## HeaderData Atom Count
+  .short  1                       ## DW_ATOM_die_offset
+  .short  6                       ## DW_FORM_data4
+  .long -1                      ## Bucket 0
+  .section  __DWARF,__apple_namespac,regular,debug
+Lnamespac_begin:
+  .long 1212240712              ## Header Magic
+  .short  1                       ## Header Version
+  .short  0                       ## Header Hash Function
+  .long 1                       ## Header Bucket Count
+  .long 0                       ## Header Hash Count
+  .long 12                      ## Header Data Length
+  .long 0                       ## HeaderData Die Offset Base
+  .long 1                       ## HeaderData Atom Count
+  .short  1                       ## DW_ATOM_die_offset
+  .short  6                       ## DW_FORM_data4
+  .long -1                      ## Bucket 0
+  .section  __DWARF,__debug_gnu_pubn,regular,debug
+.set Lset8, LpubNames_end0-LpubNames_begin0 ## Length of Public Names Info
+  .long Lset8
+LpubNames_begin0:
+  .short  2                       ## DWARF Version
+.set Lset9, Lcu_begin0-Lsection_info    ## Offset of Compilation Unit Info
+  .long Lset9
+  .long 79                      ## Compilation Unit Length
+  .long 42                      ## DIE offset
+  .byte 48                      ## Attributes: FUNCTION, EXTERNAL
+  .asciz  "f"                     ## External Name
+  .long 0                       ## End Mark
+LpubNames_end0:
+  .section  __DWARF,__debug_gnu_pubt,regular,debug
+.set Lset10, LpubTypes_end0-LpubTypes_begin0 ## Length of Public Types Info
+  .long Lset10
+LpubTypes_begin0:
+  .short  2                       ## DWARF Version
+.set Lset11, Lcu_begin0-Lsection_info   ## Offset of Compilation Unit Info
+  .long Lset11
+  .long 79                      ## Compilation Unit Length
+  .long 71                      ## DIE offset
+  .byte 144                     ## Attributes: TYPE, STATIC
+  .asciz  "int"                   ## External Name
+  .long 0                       ## End Mark
+LpubTypes_end0:
+
+.subsections_via_symbols
+  .section  __DWARF,__debug_line,regular,debug
+Lsection_line:
+Lline_table_start0: