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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
