[Lldb-commits] [PATCH] D72953: Fix the handling of unnamed bit-fields when parsing DWARF

Mon Jan 20 03:58:19 PST 2020

teemperor added a comment.

(Jim pointed out that we land this without expect_expr to make back porting easier but somehow Phabricator didn't add his comment to the review here. `expect_expr` is not yet in the downstream Github branch but it is in the 10 release branch, so that makes sense to me).

Anyway, some more points from my side:

1. I actually thought this would fix the crashes in `CGRecordLowering::lower()` that we kept seeing, but I've been dogfooding this patch today and I'm still getting the crashes when dumping arbitrary clang Decls from LLDB:

  3  libsystem_platform.dylib 0x0000000102881000 _sigtramp + 18446603342990035968
  4  liblldb.11.0.0git.dylib  0x0000000109d8eb3b (anonymous namespace)::CGRecordLowering::lower(bool) + 14555
  5  liblldb.11.0.0git.dylib  0x0000000109d89aa9 clang::CodeGen::CodeGenTypes::ComputeRecordLayout(clang::RecordDecl const*, llvm::StructType*) + 249
  6  liblldb.11.0.0git.dylib  0x0000000109e948b1 clang::CodeGen::CodeGenTypes::ConvertRecordDeclType(clang::RecordDecl const*) + 785
  7  liblldb.11.0.0git.dylib  0x0000000109e91800 clang::CodeGen::CodeGenTypes::ConvertType(clang::QualType) + 2048
  8  liblldb.11.0.0git.dylib  0x0000000109d8f269 (anonymous namespace)::CGRecordLowering::getStorageType(clang::FieldDecl const*) + 41
  9  liblldb.11.0.0git.dylib  0x0000000109d8c9f3 (anonymous namespace)::CGRecordLowering::lower(bool) + 6035
  10 liblldb.11.0.0git.dylib  0x0000000109d89aa9 clang::CodeGen::CodeGenTypes::ComputeRecordLayout(clang::RecordDecl const*, llvm::StructType*) + 249
  11 liblldb.11.0.0git.dylib  0x0000000109e948b1 clang::CodeGen::CodeGenTypes::ConvertRecordDeclType(clang::RecordDecl const*) + 785
  12 liblldb.11.0.0git.dylib  0x0000000109e91313 clang::CodeGen::CodeGenTypes::ConvertType(clang::QualType) + 787
  13 liblldb.11.0.0git.dylib  0x0000000109d8f269 (anonymous namespace)::CGRecordLowering::getStorageType(clang::FieldDecl const*) + 41
  14 liblldb.11.0.0git.dylib  0x0000000109d8c9f3 (anonymous namespace)::CGRecordLowering::lower(bool) + 6035
  15 liblldb.11.0.0git.dylib  0x0000000109d89aa9 clang::CodeGen::CodeGenTypes::ComputeRecordLayout(clang::RecordDecl const*, llvm::StructType*) + 249
  16 liblldb.11.0.0git.dylib  0x0000000109e948b1 clang::CodeGen::CodeGenTypes::ConvertRecordDeclType(clang::RecordDecl const*) + 785
  17 liblldb.11.0.0git.dylib  0x0000000109e91800 clang::CodeGen::CodeGenTypes::ConvertType(clang::QualType) + 2048
  18 liblldb.11.0.0git.dylib  0x0000000109f0a22a (anonymous namespace)::X86_64ABIInfo::classifyArgumentType(clang::QualType, unsigned int, unsigned int&, unsigned int&, bool) const + 634
  19 liblldb.11.0.0git.dylib  0x0000000109f07515 (anonymous namespace)::X86_64ABIInfo::computeInfo(clang::CodeGen::CGFunctionInfo&) const + 2117
  20 liblldb.11.0.0git.dylib  0x0000000109b513f6 clang::CodeGen::CodeGenTypes::arrangeLLVMFunctionInfo(clang::CanQual<clang::Type>, bool, bool, llvm::ArrayRef<clang::CanQual<clang::Type> >, clang::FunctionType::ExtInfo, llvm::ArrayRef<clang::FunctionType::ExtParameterInfo>, clang::CodeGen::RequiredArgs) + 2822
  21 liblldb.11.0.0git.dylib  0x0000000109b51aec arrangeLLVMFunctionInfo(clang::CodeGen::CodeGenTypes&, bool, llvm::SmallVectorImpl<clang::CanQual<clang::Type> >&, clang::CanQual<clang::FunctionProtoType>) + 700
  22 liblldb.11.0.0git.dylib  0x0000000109b51ba9 clang::CodeGen::CodeGenTypes::arrangeCXXMethodType(clang::CXXRecordDecl const*, clang::FunctionProtoType const*, clang::CXXMethodDecl const*) + 153
  23 liblldb.11.0.0git.dylib  0x0000000109b51e0d clang::CodeGen::CodeGenTypes::arrangeCXXMethodDeclaration(clang::CXXMethodDecl const*) + 525
  24 liblldb.11.0.0git.dylib  0x0000000109e15831 clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, llvm::GlobalValue*) + 177
  25 liblldb.11.0.0git.dylib  0x0000000109e0b48b clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) + 2235
  26 liblldb.11.0.0git.dylib  0x0000000109e19996 clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) + 150

So either there is another corner case not handled in this patch, we have another bug that screws up our layout or my local build is broken (I already dropped every patch beside this one and rebuild, so the last one is unlikely). Or I was mistaken what actually was causing this crash.

2. I'm still kinda confused by some parts of the code but the undocumented in/out last_field_info thing we did before was also confusing, so if this gets bitfields working then I'm fine with getting this in. However I wish we didn't add another in/out parameter by now having a normal field info and a field info for bitfields. If we could model this as one FieldInfo with a `is_bitfield` flag or so that would be preferred.

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2393
     lldb_private::ClangASTImporter::LayoutInfo &layout_info,
-    BitfieldInfo &last_field_info) {
+    FieldInfo &last_bitfield_info, FieldInfo &last_field_info) {
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
----------------
IIUC then those two bitfield info variables are mutually exclusive? I.e., either the last field was bitfield or a normal field? If yes, would it make sense to model this as one FieldInfo variable and have a bool value for differentiating between them? That way we don't have the possibility that we forget to clear one when we set the other.

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2838
           m_ast.SetMetadataAsUserID(field_decl, die.GetID());
-
           layout_info.field_offsets.insert(
----------------
unrelated?

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2842
+  last_field_info.bit_offset = 0;
+  last_field_info.bit_size = 0;

----------------
This code looks like we forgot to initialise the `last_bitfield_info` fields and would IMHO also be more readable if we had a bool value like `is_bitfield` etc.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72953/new/

https://reviews.llvm.org/D72953