[lldb][TypeSystemClang][NFC] Clean up TypeSystemClang::GetBitSize #100674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch does following NFC changes to TypeSystemClang::GetBitSize: * Factor out the Objective-C logic into a helper function. * We had a redundant check for `GetCompleteType` in the `RecordType` case. We can remove that switch case entirely and rely on the `default` case. * Introduce a new case for `FunctionProto`. I don't see a good reason for special-casing this in the `default` case. * Introduce a new case for `IncompleteArray`. The motivation for this is that there are a few issues around VLAs (i.e., `Type::IncompleteArray`) whose fixes will be easier to reason about after this refactor.
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch performs following NFC changes to TypeSystemClang::GetBitSize:
The motivation for this is that there are a few issues around VLAs (i.e., Generally I don't think treating a type with 0 size as an error (at least not for array types). But that's not something I wanted to fold into this NFC change. Full diff: https://github.com/llvm/llvm-project/pull/100674.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index f70efe5ed57e4..3c16e86f12a3f 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -4725,67 +4725,71 @@ TypeSystemClang::GetFloatTypeSemantics(size_t byte_size) {
return llvm::APFloatBase::Bogus();
}
+std::optional<uint64_t>
+TypeSystemClang::GetObjCBitSize(QualType qual_type,
+ ExecutionContextScope *exe_scope) {
+ assert(qual_type->isObjCObjectOrInterfaceType());
+ ExecutionContext exe_ctx(exe_scope);
+ Process *process = exe_ctx.GetProcessPtr();
+ if (process) {
+ if (ObjCLanguageRuntime *objc_runtime =
+ ObjCLanguageRuntime::Get(*process)) {
+ if (std::optional<uint64_t> bit_size =
+ objc_runtime->GetTypeBitSize(GetType(qual_type)))
+ return *bit_size;
+ }
+ } else {
+ static bool g_printed = false;
+ if (!g_printed) {
+ StreamString s;
+ DumpTypeDescription(qual_type.getAsOpaquePtr(), s);
+
+ llvm::outs() << "warning: trying to determine the size of type ";
+ llvm::outs() << s.GetString() << "\n";
+ llvm::outs() << "without a valid ExecutionContext. this is not "
+ "reliable. please file a bug against LLDB.\n";
+ llvm::outs() << "backtrace:\n";
+ llvm::sys::PrintStackTrace(llvm::outs());
+ llvm::outs() << "\n";
+ g_printed = true;
+ }
+ }
+
+ return getASTContext().getTypeSize(qual_type) +
+ getASTContext().getTypeSize(getASTContext().ObjCBuiltinClassTy);
+}
+
std::optional<uint64_t>
TypeSystemClang::GetBitSize(lldb::opaque_compiler_type_t type,
ExecutionContextScope *exe_scope) {
- if (GetCompleteType(type)) {
- clang::QualType qual_type(GetCanonicalQualType(type));
- const clang::Type::TypeClass type_class = qual_type->getTypeClass();
- switch (type_class) {
- case clang::Type::Record:
- if (GetCompleteType(type))
- return getASTContext().getTypeSize(qual_type);
- else
- return std::nullopt;
- break;
+ if (!GetCompleteType(type))
+ return std::nullopt;
- case clang::Type::ObjCInterface:
- case clang::Type::ObjCObject: {
- ExecutionContext exe_ctx(exe_scope);
- Process *process = exe_ctx.GetProcessPtr();
- if (process) {
- if (ObjCLanguageRuntime *objc_runtime =
- ObjCLanguageRuntime::Get(*process)) {
- if (std::optional<uint64_t> bit_size =
- objc_runtime->GetTypeBitSize(GetType(qual_type)))
- return *bit_size;
- }
- } else {
- static bool g_printed = false;
- if (!g_printed) {
- StreamString s;
- DumpTypeDescription(type, s);
-
- llvm::outs() << "warning: trying to determine the size of type ";
- llvm::outs() << s.GetString() << "\n";
- llvm::outs() << "without a valid ExecutionContext. this is not "
- "reliable. please file a bug against LLDB.\n";
- llvm::outs() << "backtrace:\n";
- llvm::sys::PrintStackTrace(llvm::outs());
- llvm::outs() << "\n";
- g_printed = true;
- }
- }
- }
- [[fallthrough]];
- default:
- const uint32_t bit_size = getASTContext().getTypeSize(qual_type);
- if (bit_size == 0) {
- if (qual_type->isIncompleteArrayType())
- return getASTContext().getTypeSize(
- qual_type->getArrayElementTypeNoTypeQual()
- ->getCanonicalTypeUnqualified());
- }
- if (qual_type->isObjCObjectOrInterfaceType())
- return bit_size +
- getASTContext().getTypeSize(getASTContext().ObjCBuiltinClassTy);
- // Function types actually have a size of 0, that's not an error.
- if (qual_type->isFunctionProtoType())
- return bit_size;
- if (bit_size)
- return bit_size;
- }
+ clang::QualType qual_type(GetCanonicalQualType(type));
+ const clang::Type::TypeClass type_class = qual_type->getTypeClass();
+ switch (type_class) {
+ case clang::Type::FunctionProto:
+ case clang::Type::Record:
+ return getASTContext().getTypeSize(qual_type);
+ case clang::Type::ObjCInterface:
+ case clang::Type::ObjCObject:
+ return GetObjCBitSize(qual_type, exe_scope);
+ case clang::Type::IncompleteArray: {
+ const uint32_t bit_size = getASTContext().getTypeSize(qual_type);
+ if (bit_size == 0)
+ return getASTContext().getTypeSize(
+ qual_type->getArrayElementTypeNoTypeQual()
+ ->getCanonicalTypeUnqualified());
+
+ return bit_size;
+ }
+ default: {
+ const uint32_t bit_size = getASTContext().getTypeSize(qual_type);
+ if (bit_size)
+ return bit_size;
}
+ }
+
return std::nullopt;
}
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index d67b7a4c9fe72..70722eb375ab7 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -1172,6 +1172,9 @@ class TypeSystemClang : public TypeSystem {
/// on creation of a new instance.
void LogCreation() const;
+ std::optional<uint64_t> GetObjCBitSize(clang::QualType qual_type,
+ ExecutionContextScope *exe_scope);
+
// Classes that inherit from TypeSystemClang can see and modify these
std::string m_target_triple;
std::unique_ptr<clang::ASTContext> m_ast_up;
|
JDevlieghere
requested changes
Jul 26, 2024
| ExecutionContextScope *exe_scope) { | ||
| assert(qual_type->isObjCObjectOrInterfaceType()); | ||
| ExecutionContext exe_ctx(exe_scope); | ||
| Process *process = exe_ctx.GetProcessPtr(); |
There was a problem hiding this comment.
Nit: you could move this into the if right below.
Comment on lines
+4747
to
+4753
| llvm::outs() << "warning: trying to determine the size of type "; | ||
| llvm::outs() << s.GetString() << "\n"; | ||
| llvm::outs() << "without a valid ExecutionContext. this is not " | ||
| "reliable. please file a bug against LLDB.\n"; | ||
| llvm::outs() << "backtrace:\n"; | ||
| llvm::sys::PrintStackTrace(llvm::outs()); | ||
| llvm::outs() << "\n"; |
There was a problem hiding this comment.
Rather than printing to stdout, this should use the global diagnostics (Debugger::ReportWarning). The function takes an optional std::once_flag to do the same thing you're doing here.
There was a problem hiding this comment.
Yea I was really tempted to just outright remove this tbh
There was a problem hiding this comment.
Mind if I keep this as-is for now? To keep this change as NFC as possible? (I'm just moving this code around)
There was a problem hiding this comment.
That's fine as long as it gets either fixed or removed eventually.
JDevlieghere
approved these changes
Jul 26, 2024
|
Test failure is: Which is unrelated (and I've seen be flakey lately). Will merge now |
Michael137
added a commit
that referenced
this pull request
Jul 29, 2024
…rrayType (#100710) Depends on #100674 Currently, we treat VLAs declared as `int[]` and `int[0]` identically. I.e., we create them as `IncompleteArrayType`s. However, the `DW_AT_count` for `int[0]` *does* exist, and is retrievable without an execution context. This patch decouples the notion of "has 0 elements" from "has no known `DW_AT_count`". This aligns with how Clang represents `int[0]` in the AST (it treats it as a `ConstantArrayType` of 0 size). This issue was surfaced when adapting LLDB to #93069. There, the `__compressed_pair_padding` type has a `char[0]` member. If we previously got the `__compressed_pair_padding` out of a module (where clang represents `char[0]` as a `ConstantArrayType`), and try to merge the AST with one we got from DWARF (where LLDB used to represent `char[0]` as an `IncompleteArrayType`), the AST structural equivalence check fails, resulting in silent ASTImporter failures. This manifested in a failure in `TestNonModuleTypeSeparation.py`. **Implementation** 1. Adjust `ParseChildArrayInfo` to store the element counts of each VLA dimension as an `optional<uint64_t>`, instead of a regular `uint64_t`. So when we pass this down to `CreateArrayType`, we have a better heuristic for what is an `IncompleteArrayType`. 2. In `TypeSystemClang::GetBitSize`, if we encounter a `ConstantArrayType` simply return the size that it was created with. If we couldn't determine the authoritative bound from DWARF during parsing, we would've created an `IncompleteArrayType`. This ensures that `GetBitSize` on arrays with `DW_AT_count 0x0` returns `0` (whereas previously it would've returned a `std::nullopt`, causing that `FieldDecl` to just get dropped during printing)
banach-space
pushed a commit
to banach-space/llvm-project
that referenced
this pull request
Aug 7, 2024
…rrayType (llvm#100710) Depends on llvm#100674 Currently, we treat VLAs declared as `int[]` and `int[0]` identically. I.e., we create them as `IncompleteArrayType`s. However, the `DW_AT_count` for `int[0]` *does* exist, and is retrievable without an execution context. This patch decouples the notion of "has 0 elements" from "has no known `DW_AT_count`". This aligns with how Clang represents `int[0]` in the AST (it treats it as a `ConstantArrayType` of 0 size). This issue was surfaced when adapting LLDB to llvm#93069. There, the `__compressed_pair_padding` type has a `char[0]` member. If we previously got the `__compressed_pair_padding` out of a module (where clang represents `char[0]` as a `ConstantArrayType`), and try to merge the AST with one we got from DWARF (where LLDB used to represent `char[0]` as an `IncompleteArrayType`), the AST structural equivalence check fails, resulting in silent ASTImporter failures. This manifested in a failure in `TestNonModuleTypeSeparation.py`. **Implementation** 1. Adjust `ParseChildArrayInfo` to store the element counts of each VLA dimension as an `optional<uint64_t>`, instead of a regular `uint64_t`. So when we pass this down to `CreateArrayType`, we have a better heuristic for what is an `IncompleteArrayType`. 2. In `TypeSystemClang::GetBitSize`, if we encounter a `ConstantArrayType` simply return the size that it was created with. If we couldn't determine the authoritative bound from DWARF during parsing, we would've created an `IncompleteArrayType`. This ensures that `GetBitSize` on arrays with `DW_AT_count 0x0` returns `0` (whereas previously it would've returned a `std::nullopt`, causing that `FieldDecl` to just get dropped during printing)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch performs following NFC changes to TypeSystemClang::GetBitSize:
FunctionProto. I don't see a good reason for special-casing this in thedefaultcase.GetCompleteTypein theRecordTypecase. We used to allow zero-size types for records and function prototypes, so we can just fold them into the same case.IncompleteArray.The motivation for this is that there are a few issues around VLAs (i.e.,
Type::IncompleteArray) whose fixes will be easier to reason about after this refactor.Generally I don't think treating a type with 0 size as an error is the right thing to do (at least not for array types). But that's not something I wanted to fold into this NFC change.