[LLDB][NativePDB] Fix parameter size for member functions LF_MFUNCTION

[zequanwu]

Summary

Fix the problem that it was treating member functions as non-member functions
when trying to get the parameter size. This causes some non-parameter variables
showing up in function signature. Suprisingly,
cantFail(TypeDeserializer::deserializeAs<ProcedureRecord>(...)) just sliently
parse it without error and gave the wrong result.

It's hard to test it. This only causes problem when params_remaining
is larger than the real parameter size. If it's smaller, we also check
individual local variable's attribute to see it's a parameter. When I trying to
come up with a test, the parameter size is always 0 if we parse LF_MFUNCTION as
LF_PROCEDURE.

[rnk]

Right, so cantFail must be an assertions-only check. That's not great. These deserialization APIs have really, really bad ergonomics and could use a lot of improvement if you want to look into that.

It would be really nice if, for example, this worked more like dyn_cast. Consider this kind of code:

CVType signature;
NewTypeMatcher d(signature);
if (auto *sig = d.getAs<ProcedureRecord>()) {
   ... handle ProcedureRecord
} else if (auto *sig = d.getAs<MemberFunctionRecord>()) {
  ... handle MemberFunctionRecord
}

[labath]

Given what Reid said, would it be possible to make a test case that fails in an asserts build? Or was it maybe failing already there?

[zequanwu]

In D136209#3866933, @rnk wrote:

Right, so cantFail must be an assertions-only check. That's not great.

With assertion enabled, lldb still silently deserialize member functions as non-member functions. My guess is that TypeDeserializer::deserializeAs doesn't return error in this case since sizeof(MemberFunctionRecord) > sizeof(ProcedureRecord). We may need additional checking in TypeDeserializer::deserializeAs to verify that we are deserializing the correct type.

[labath]

So if I understand correctly, when we deserialize incorrectly, this causes us to misclassify some function parameters (arguments) as local variables. Is that correct? If yes, then that is something that can be checked. Even if the misclassified variables behave perfectly, their kind is still visible, e.g. in the output of "frame variable --scope".

[zequanwu]

In D136209#3878821, @labath wrote:

So if I understand correctly, when we deserialize incorrectly, this causes us to misclassify some function parameters (arguments) as local variables. Is that correct? If yes, then that is something that can be checked. Even if the misclassified variables behave perfectly, their kind is still visible, e.g. in the output of "frame variable --scope".

It's the opposite. Some non parameters local variables are misclassified as parameters, since deserialized parameter count is incorrect/larger. Those misclassified local variables will have the parameter bit set. See the inline comment for detail.

Or maybe we should just rely on individual variable's debug info to determine if a local var is parameter or not, not the parameter count found in function debug info.

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

1825

Because we also check individual variable's debug info kind (parameter or not), if the deserialized parameter count number is smaller than actual number, this would fix it. So, it only causes problem when the deserialized result is larger which causes non-parameter local variables count as parameters.

The reason for this check, I vaguely remember, is that some function debug info has missing parameter count (e.g. 0), but the individual variable debug info says it's a parameter.

[labath]

In D136209#3880121, @zequanwu wrote:

In D136209#3878821, @labath wrote:

So if I understand correctly, when we deserialize incorrectly, this causes us to misclassify some function parameters (arguments) as local variables. Is that correct? If yes, then that is something that can be checked. Even if the misclassified variables behave perfectly, their kind is still visible, e.g. in the output of "frame variable --scope".

It's the opposite. Some non parameters local variables are misclassified as parameters, since deserialized parameter count is incorrect/larger. Those misclassified local variables will have the parameter bit set. See the inline comment for detail.

Ugh, yeah, carry on as you were.

Or maybe we should just rely on individual variable's debug info to determine if a local var is parameter or not, not the parameter count found in function debug info.

If it's sufficient, then yeah, definitely.

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

VariableSP SymbolFileNativePDB::CreateLocalVariable(PdbCompilandSymId scope_id,
   TypeSP type_sp = GetOrCreateType(var_info.type);
   if (!type_sp)
     return nullptr;
   std::string name = var_info.name.str();
   Declaration decl;
   SymbolFileTypeSP sftype =
       std::make_shared<SymbolFileType>(*this, type_sp->GetID());
 
   is_param |= var_info.is_param;
   ValueType var_scope =
       is_param ? eValueTypeVariableArgument : eValueTypeVariableLocal;
   bool external = false;
   bool artificial = false;
   bool location_is_constant_data = false;
   bool static_member = false;
   Variable::RangeList scope_ranges;
   VariableSP var_sp = std::make_shared<Variable>(
size_t SymbolFileNativePDB::ParseVariablesForBlock(PdbCompilandSymId block_id) {
   CVSymbol sym = cii->m_debug_stream.readSymbolAtOffset(block_id.offset);
   uint32_t params_remaining = 0;
   switch (sym.kind()) {
   case S_GPROC32:
   case S_LPROC32: {
     ProcSym proc(static_cast<SymbolRecordKind>(sym.kind()));
     cantFail(SymbolDeserializer::deserializeAs<ProcSym>(sym, proc));
     CVType signature = m_index->tpi().getType(proc.FunctionType);
+    if (signature.kind() == LF_PROCEDURE) {
       ProcedureRecord sig;
-    cantFail(TypeDeserializer::deserializeAs<ProcedureRecord>(signature, sig));
+      if (llvm::Error e = TypeDeserializer::deserializeAs<ProcedureRecord>(
+              signature, sig)) {
+        llvm::consumeError(std::move(e));
+        return 0;
+      }
+      params_remaining = sig.getParameterCount();
+    } else if (signature.kind() == LF_MFUNCTION) {
+      MemberFunctionRecord sig;
+      if (llvm::Error e = TypeDeserializer::deserializeAs<MemberFunctionRecord>(
+              signature, sig)) {
+        llvm::consumeError(std::move(e));
+        return 0;
+      }
       params_remaining = sig.getParameterCount();
+    } else
+      return 0;
     break;
   }
   case S_BLOCK32:
     break;
   case S_INLINESITE:
     break;
   default:
     lldbassert(false && "Symbol is not a block!");