[lldb] Fix Type::GetByteSize for pointer types

[labath]

Summary

The function was returning an incorrect (empty) value on the first
invocation. Given that this only affected the first invocation, this
bug/typo went mostly unaffected. DW_AT_const_value were particularly
badly affected by this as the GetByteSize call is
SymbolFileDWARF::ParseVariableDIE is likely to be the first call of this
function, and its effects cannot be undone by retrying.

Depends on D86348.

[davide]

I came to the same conclusion when analyzing https://bugs.llvm.org/show_bug.cgi?id=47257 (but you beat me to the punch).

[shafik]

lldb/source/Symbol/Type.cpp

378

Wouldn't it be better to turn m_byte_size into an Optional? As this fix shows this having this additional state we carry around is error prone.

[labath]

lldb/source/Symbol/Type.cpp

378

IIRC, this was done this way to reduce sizeof(Type). Note that the external interface is Optional<uint64_t> and the only place that knows (should know?) about this encoding is this function.

[davide]

lldb/source/Symbol/Type.cpp

378

This is a much larger project, probably worth considering.
With that in mind, it wouldn't have prevented this bug from happening, as this falls through and returns an Optional<T> anyway.
Yay fuzz testing (that found this), I would say.

[shafik]

lldb/source/Symbol/Type.cpp

378

It would have prevented this bug as you just return m_byte_size no matter what.

[davide]

@labath something I noticed when finding this (and related bugs) is that frame var carries a decent diagnostic

(int *) l_125 = <empty constant data>

and the expression parser returns something not particularly useful:

(lldb) p l_125 
error: <lldb wrapper prefix>:43:31: no member named 'l_125' in namespace '$__lldb_local_vars'
    using $__lldb_local_vars::l_125;
          ~~~~~~~~~~~~~~~~~~~~^
error: <user expression 0>:1:1: use of undeclared identifier 'l_125'
l_125

From my testing infrastructure/fuzzing perspective the two are indistinguishable, as the script I've written chokes on both, but it would be better from an ergonomics point of view if p would return something meaningful, if possible (even if there's a bug in lldb). Do you think it's worth filing a PR? (also, cc: @teemperor for ideas as he spent a fair amount of time working on the expression parser)

[davide]

lldb/source/Symbol/Type.cpp

378

If you think it's an improvement, please consider submitting a review, I would be eager to look at it!

[labath]

In D86436#2234110, @davide wrote:

@labath something I noticed when finding this (and related bugs) is that frame var carries a decent diagnostic

(int *) l_125 = <empty constant data>

and the expression parser returns something not particularly useful:

(lldb) p l_125 
error: <lldb wrapper prefix>:43:31: no member named 'l_125' in namespace '$__lldb_local_vars'
    using $__lldb_local_vars::l_125;
          ~~~~~~~~~~~~~~~~~~~~^
error: <user expression 0>:1:1: use of undeclared identifier 'l_125'
l_125

From my testing infrastructure/fuzzing perspective the two are indistinguishable, as the script I've written chokes on both, but it would be better from an ergonomics point of view if p would return something meaningful, if possible (even if there's a bug in lldb). Do you think it's worth filing a PR? (also, cc: @teemperor for ideas as he spent a fair amount of time working on the expression parser)

For this particular case, I think the best diagnostic would be one of those module-level warnings we print to stderr, as I think that a DW_AT_const_value attribute with no data means the debug info is broken.

Speaking more generally, it may be interesting to try to present variables with no value (e.g. optimized out) to clang as undefined (extern) variables. That way we could get past the "compile" stage, and handle this stuff in the "link" stage. Here, I believe we have more control so it may be easier to print something like "Cannot evaluate expression because variable 'foo' is optimized out. But I am not really an expert here so I am just guessing...
I'm not really an expert here, and we are sort of limited by what error messages we can get clang to produce. The current error message is not completely unreasonable because it basically says "this variable does not exist"

lldb/source/Symbol/Type.cpp

378

FWIW, there is a way to make this more robust without the memory size increase due to llvm::Optional:

if (!m_byte_size_has_value) {
  if (Optional<uint64_t> byte_size = ComputeByteSize()) {
    m_byte_size_has_value = true;
    m_byte_size = *byte_size;
  }
}
if (m_byte_size_has_value)
  return m_byte_size;
return None;

That way the optional-to-flat conversion is fully contained in 9 lines of code, which can be easily audited for correctness.

lldb/source/Symbol/Type.cpp

case eEncodingIsTypedefUID: {
 
     // If we are a pointer or reference, then this is just a pointer size;
     case eEncodingIsPointerUID:
     case eEncodingIsLValueReferenceUID:
     case eEncodingIsRValueReferenceUID: {
       if (ArchSpec arch = m_symbol_file->GetObjectFile()->GetArchitecture()) {
         m_byte_size = arch.GetAddressByteSize();
         m_byte_size_has_value = true;
+        return m_byte_size;
       }
     } break;
   }
   return {};
 }
 
 uint32_t Type::GetNumChildren(bool omit_empty_base_classes) {
   return GetForwardCompilerType().GetNumChildren(omit_empty_base_classes, nullptr);

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

 # Test handling of (optimized-out/location-less) variables whose value is
 # specified by DW_AT_const_value
 
 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
 # RUN: %lldb %t \
-# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4" \
+# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4 udata_ptr" \
 # RUN:   -o exit | FileCheck %s
 
-# CHECK-LABEL: target variable udata data1 data2 data4 data8 string strp ref4
+# CHECK-LABEL: target variable
 ## Variable specified via DW_FORM_udata. This is typical for clang (10).
 # CHECK: (unsigned long) udata = 4742474247424742
 ## Variables specified via fixed-size forms. This is typical for gcc (9).
 # CHECK: (unsigned long) data1 = 47
 # CHECK: (unsigned long) data2 = 4742
 # CHECK: (unsigned long) data4 = 47424742
 # CHECK: (unsigned long) data8 = 4742474247424742
 ## Variables specified using string forms. This behavior purely speculative -- I
 ## don't know of any compiler that would represent character strings this way.
 # CHECK: (char [7]) string = "string"
 # CHECK: (char [7]) strp = "strp"
 ## Bogus attribute form. Let's make sure we don't crash at least.
 # CHECK: (char [7]) ref4 = <empty constant data>
+## A variable of pointer type.
+# CHECK: (unsigned long *) udata_ptr = 0xdeadbeefbaadf00d
 
         .section        .debug_abbrev,"",@progbits
         .byte   1                       # Abbreviation Code
         .byte   17                      # DW_TAG_compile_unit
         .byte   1                       # DW_CHILDREN_yes
         .byte   37                      # DW_AT_producer
         .byte   8                       # DW_FORM_string
         .byte   3                       # DW_AT_name
         .byte   8                       # DW_FORM_string
         .byte   0                       # EOM(1)
         .byte   0                       # EOM(2)
+        .byte   2                       # Abbreviation Code
+        .byte   15                      # DW_TAG_pointer_type
+        .byte   0                       # DW_CHILDREN_no
+        .byte   73                      # DW_AT_type
+        .byte   19                      # DW_FORM_ref4
+        .byte   0                       # EOM(1)
+        .byte   0                       # EOM(2)
         .byte   4                       # Abbreviation Code
         .byte   1                       # DW_TAG_array_type
         .byte   1                       # DW_CHILDREN_yes
         .byte   73                      # DW_AT_type
         .byte   19                      # DW_FORM_ref4
         .byte   0                       # EOM(1)
         .byte   0                       # EOM(2)
         .byte   5                       # Abbreviation Code
.Lchar:
         .asciz  "char"                  # DW_AT_name
         .byte   1                       # DW_AT_byte_size
         .byte   6                       # DW_AT_encoding
 .Lulong:
         .byte   6                       # Abbrev DW_TAG_base_type
         .asciz  "unsigned long"         # DW_AT_name
         .byte   8                       # DW_AT_byte_size
         .byte   7                       # DW_AT_encoding
+.Lulong_ptr:
+        .byte   2                       # Abbrev DW_TAG_pointer_type
+        .long   .Lulong-.Lcu_begin0     # DW_AT_type
 
         .byte   10                      # Abbrev DW_TAG_variable
         .asciz  "udata"                 # DW_AT_name
         .long   .Lulong-.Lcu_begin0     # DW_AT_type
         .uleb128 4742474247424742       # DW_AT_const_value
 
         .byte   11                      # Abbrev DW_TAG_variable
         .asciz  "data1"                 # DW_AT_name
.Lulong_ptr:
         .long   .Lchar_arr-.Lcu_begin0  # DW_AT_type
         .long   .Lstrp                  # DW_AT_const_value
 
         .byte   17                      # Abbrev DW_TAG_variable
         .asciz  "ref4"                  # DW_AT_name
         .long   .Lchar_arr-.Lcu_begin0  # DW_AT_type
         .long   .Lulong-.Lcu_begin0     # DW_AT_const_value
 
+        .byte   10                      # Abbrev DW_TAG_variable
+        .asciz  "udata_ptr"             # DW_AT_name
+        .long   .Lulong_ptr-.Lcu_begin0 # DW_AT_type
+        .uleb128 0xdeadbeefbaadf00d     # DW_AT_const_value
+
         .byte   0                       # End Of Children Mark
 .Ldebug_info_end0:
 
         .section        .debug_str,"MS",@progbits,1
 .Lstrp:
         .asciz "strp"