[lldb] Detection of `this` arguments incomplete

[SingleAccretion]

I've encountered what seems like a bug in this code:

llvm-project/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Lines 3140 to 3147 in fd78472

	if (encoding_mask & Type::eEncodingIsPointerUID) {
	is_static = false;
	if (encoding_mask & (1u << Type::eEncodingIsConstUID))
	type_quals |= clang::Qualifiers::Const;
	if (encoding_mask & (1u << Type::eEncodingIsVolatileUID))
	type_quals |= clang::Qualifiers::Volatile;
	}

In my case, the entry for "this" looked like this:

0x000001a4:       DW_TAG_formal_parameter
                    DW_AT_artificial	(0x01)
                    DW_AT_type	(0x000001e0 "WebAssemblyPtrWrapper<DerivedType>")

...

0x000001e0:   DW_TAG_structure_type
                DW_AT_name	("WebAssemblyPtrWrapper<DerivedType>")
                DW_AT_byte_size	(0x04)

And was still picked up, despite the seeming intention of filtering out non-pointer arguments.

[llvmbot]

@llvm/issue-subscribers-lldb

Author: None (SingleAccretion)

I've encountered what seems like a bug in this code:

llvm-project/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Lines 3140 to 3147 in fd78472

	if (encoding_mask & Type::eEncodingIsPointerUID) {
	is_static = false;
	if (encoding_mask & (1u << Type::eEncodingIsConstUID))
	type_quals |= clang::Qualifiers::Const;
	if (encoding_mask & (1u << Type::eEncodingIsVolatileUID))
	type_quals |= clang::Qualifiers::Volatile;
	}

In my case, the entry for "this" looked like this:

0x000001a4:       DW_TAG_formal_parameter
                    DW_AT_artificial	(0x01)
                    DW_AT_type	(0x000001e0 "WebAssemblyPtrWrapper<DerivedType>")

...

0x000001e0:   DW_TAG_structure_type
                DW_AT_name	("WebAssemblyPtrWrapper<DerivedType>")
                DW_AT_byte_size	(0x04)

And was still picked up, despite the seeming intention of filtering out non-pointer arguments.

[Michael137]

0x000001a4:       DW_TAG_formal_parameter
                    DW_AT_artificial	(0x01)
                    DW_AT_type	(0x000001e0 "WebAssemblyPtrWrapper<DerivedType>")

Can you post the DWARF for the entire subprogram? And if possible also the source code. Are you using C++23's "deducing this" perhaps?

What filtering are you referring to? The loop you linked will skip parsing any artificial parameter, including the one whose DWARF you showed. One problem I can see here though is that is_static will be set to true for this function. Definitely a bug

[SingleAccretion]

Can you post the DWARF for the entire subprogram?

The full(er) DWARF is as follows, though I suspect it is not particularly useful:

; Declaration
0x00000178:   DW_TAG_structure_type
                DW_AT_calling_convention	(DW_CC_pass_by_value)
                DW_AT_name	("DerivedType")
                DW_AT_byte_size	(16)
                DW_AT_decl_file	("C:\Users\Accretion\source\dotnet\wasmtime\tests\all\debug\testsuite\generic.cpp")
                DW_AT_decl_line	(22)

0x00000181:     DW_TAG_inheritance
                  DW_AT_data_member_location	(0)
                  DW_AT_type	(0x000001bb "BaseType")

0x00000187:     DW_TAG_member
                  DW_AT_name	("DerivedValue")
                  DW_AT_decl_file	("C:\Users\Accretion\source\dotnet\wasmtime\tests\all\debug\testsuite\generic.cpp")
                  DW_AT_decl_line	(23)
                  DW_AT_data_member_location	(8)
                  DW_AT_type	(0x00000171 "long long")

0x00000193:     DW_TAG_subprogram
                  DW_AT_linkage_name	("_ZN11DerivedType14InstanceMethodEv")
                  DW_AT_name	("InstanceMethod")
                  DW_AT_decl_file	("C:\Users\Accretion\source\dotnet\wasmtime\tests\all\debug\testsuite\generic.cpp")
                  DW_AT_decl_line	(25)
                  DW_AT_declaration	(0x01)
                  DW_AT_external	(0x01)
                  DW_AT_type	(0x0000016a "int")

0x000001a4:       DW_TAG_formal_parameter
                    DW_AT_artificial	(0x01)
                    DW_AT_type	(0x000001e0 "WebAssemblyPtrWrapper<DerivedType>")

0x000001aa:       NULL

; Definition
0x000003ba:   DW_TAG_subprogram
                DW_AT_low_pc	(0x00000000000004a0)
                DW_AT_high_pc	(0x0000000000000540)
                DW_AT_frame_base	(DW_OP_call_frame_cfa)
                DW_AT_specification	(0x00000193 "_ZN11DerivedType14InstanceMethodEv")

0x000003cb:     DW_TAG_variable
                  DW_AT_name	("__vmctx")
                  DW_AT_type	(0x000000000000004c "WasmtimeVMContext *")
                  DW_AT_location	(0x000003a8: 
                     (0x00000000000004cc, 0x00000000000004fc): DW_OP_reg5
                     (0x00000000000004fc, 0x0000000000000528): DW_OP_reg13)

0x000003d8:     DW_TAG_formal_parameter
                  DW_AT_location	(0x000003de: 
                     (0x00000000000004d3, 0x00000000000004fc): DW_OP_breg14 +0, DW_OP_plus_uconst 0xc, DW_OP_breg5 +136, DW_OP_deref, DW_OP_swap, DW_OP_const4u 0xffffffff, DW_OP_and, DW_OP_plus
                     (0x00000000000004fc, 0x000000000000051d): DW_OP_breg14 +0, DW_OP_plus_uconst 0xc, DW_OP_breg13 +136, DW_OP_deref, DW_OP_swap, DW_OP_const4u 0xffffffff, DW_OP_and, DW_OP_plus)
                  DW_AT_name	("__this")
                  DW_AT_type	(0x000004be "WebAssemblyPtrWrapper<DerivedType>")

0x000003e5:     NULL

; The pointer wrapper type
0x000004be:   DW_TAG_structure_type
                DW_AT_name	("WebAssemblyPtrWrapper<DerivedType>")
                DW_AT_byte_size	(0x04)

0x000004c4:     DW_TAG_template_type_parameter
                  DW_AT_name	("T")
                  DW_AT_type	(0x00000178 "DerivedType")

0x000004cd:     DW_TAG_member
                  DW_AT_name	("__ptr")
                  DW_AT_type	(0x0000000000000017 "WebAssemblyPtr")
                  DW_AT_data_member_location	(0x00)

While the source code for this is just an empty instance method in a C++ class (DerivedType), this DWARF is a transformed version of the original that a WASM runtime (wasmtime) performs to make it usable from the native debugger as-is. Moreover, this DWARF is from an unfinished change of mine where I strip instance-ness of methods by removing DW_AT_object_pointer because the aforementioned transform changes all T*s into WebAssemblyPtrWrapper<T>s, making this unusable.

From above, you can see there is a bug: the declaration and the definition do not match. In LLDB this manifested as my now-not-instance method still being shown as instance in type lookup DerivedType. To investigate, I've opened LLDB in the debugger to see how precisely is the instance-ness determined.

That led me to this code in DWARFASTParserClang::ParseChildParameters, parsing the declaration (specification). What happens is that we enter if (skip_artificial && is_artificial), resolve this_type to be WebAssemblyPtrWrapper<DerivedType>, get 0x2 for encoding_mask as the type's EncodingDataType is eEncodingIsUID, and so encoding_mask & Type::eEncodingIsPointerUID gets calculated as 0x2 & 0x6 => 0x2 => true => is_static is set to false.

What filtering are you referring to?

I would expect the code's intent to have been to filter out non-pointer types from being this, i. e.:

- if (encoding_mask & Type::eEncodingIsPointerUID) { 
+ if (encoding_mask & (1u << Type::eEncodingIsPointerUID)) { 

[Michael137]

The issue can reproduced without the WASM transformation you're talking about using deducing this:

struct Bar {                          
    template <typename Self>          
    void method(this Self self, int) {
    }                                 
                                      
    void method(float) {}             
};                                    
                                      
int main() {                          
    Bar{}.method(5);                  
    Bar{}.method(5.0);                
    return 0;                         
}                                     

(lldb) type lookup Bar
struct Bar {
    void method(float);
}

So yes, this is definitely a bug that can occur with valid C++/DWARF.

• if (encoding_mask & Type::eEncodingIsPointerUID) {

• if (encoding_mask & (1u << Type::eEncodingIsPointerUID)) {

But the suggested patch wouldn't be the solution here. The EncodingDataType::eEncodingIsConstUID and EncodingDataType::eEncodingIsVolatileUID are special in that their values can (and are) used as bitmasks. But Type::eEncodingIsPointerUID shouldn't be used as a bitmask. The problem here is that we're assuming the this parameter is always a pointer, which isn't true anymore in newer C++ versions. It need not even be called this anymore but could be any name. So we'll have to come up with a new heuristic for detecting whether a method is static or not

[Michael137]

I raised following two issues:

• DW_AT_object_pointer not attached to method declarations #120973
• DW_AT_object_pointer not emitted for explicit object parameter #120974

IMO this would be the best way to resolve the issue you're seeing (of course we would still need to make LLDB actually use the DW_AT_object_pointer attribute to distinguish static vs. non-static methods). Though you said something about stripping DW_AT_object_pointer attributes from your DWARF. Could you elaborate on what you're doing/trying to achieve?

[SingleAccretion]

IMO this would be the best way to resolve the issue you're seeing.

Indeed. Of course, we (as in wasmtime) cannot rely on seeing 'new' output if/when it gets fixed, so we'll need to maintain a version of the current LLDB heuristic as well.

Just to be clear though, this issue is originally about LLDB's logic not behaving as it was seemingly intended to. You write in #120973:

So LLDB currently relies on the existence of a DW_TAG_formal_parameter called this and that it is a pointer type.

The "...and that it is a pointer type..." part is not how the code currently behaves - struct/class types will also get recognized as this.

Could you elaborate on what you're doing/trying to achieve?

As I alluded to in the previous comment, wasmtime transforms DWARF<WASM> to DWARF<native>. For .debug_info, it mostly just copies things verbatim from DWARF<WASM>. However, since pointers in WASM are relative to the start of linear memory, the transform also replaces T*s with structs types that have overloaded operator ->/operator *, which essentially add that linear memory base. It allows you to interact with pointers in the debugger 'naturally':

p something_that_lives_in_linear_memory->Member

This doesn't work for this because this is special. LLDB expects it to be a proper pointer type in the expression evaluator (currently, at least). And you can't circumvent this with p this->Member. Hence, the 'stripping' idea - if we replace this with __this, it becomes possible to do p __this->Member. Not as good as p Member, but that's the limitation of the current approach.

[Michael137]

IMO this would be the best way to resolve the issue you're seeing.

Indeed. Of course, we (as in wasmtime) cannot rely on seeing 'new' output if/when it gets fixed, so we'll need to maintain a version of the current LLDB heuristic as well.

Just to be clear though, this issue is originally about LLDB's logic not behaving as it was seemingly intended to. You write in #120973:

So LLDB currently relies on the existence of a DW_TAG_formal_parameter called this and that it is a pointer type.

The "...and that it is a pointer type..." part is not how the code currently behaves - struct/class types will also get recognized as this.

Ah I see, I misunderstood our use of GetEncodingMask here. Thanks for the correction. Indeed, that does also seem like a bug, and the fix as you propose (to use 1 << Type::eEncodingIsPointerUID) is correct. We don't want to use Type::eEncodingIsPointerUID as the mask itself.