[lldb] Make LanguageRuntime::GetTypeBitSize return an optional (NFC) #96013
Conversation
This file contains hidden or 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
Make LanguageRuntime::GetTypeBitSize return an optional. This should be NFC, though the ObjCLanguageRuntime implementation is (possibly) more defensive against returning 0. I'm not sure if it's possible for both `m_ivar.size` and `m_ivar.offset` to be zero. Previously, we'd return 0 and cache it, only to discard it the next time when finding it in the cache, and recomputing it again. The new code will avoid putting it in the cache in the first place.
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesMake LanguageRuntime::GetTypeBitSize return an optional. This should be NFC, though the ObjCLanguageRuntime implementation is (possibly) more defensive against returning 0. I'm not sure if it's possible for both Full diff: https://github.com/llvm/llvm-project/pull/96013.diff 4 Files Affected:
diff --git a/lldb/include/lldb/Target/LanguageRuntime.h b/lldb/include/lldb/Target/LanguageRuntime.h
index a2a9c0163f082..58874c4188034 100644
--- a/lldb/include/lldb/Target/LanguageRuntime.h
+++ b/lldb/include/lldb/Target/LanguageRuntime.h
@@ -169,9 +169,9 @@ class LanguageRuntime : public Runtime, public PluginInterface {
return m_process->GetTarget().GetSearchFilterForModule(nullptr);
}
- virtual bool GetTypeBitSize(const CompilerType &compiler_type,
- uint64_t &size) {
- return false;
+ virtual std::optional<uint64_t>
+ GetTypeBitSize(const CompilerType &compiler_type) {
+ return {};
}
virtual void SymbolsDidLoad(const ModuleList &module_list) {}
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.cpp
index ba52444f0c2fc..a812ffba7a648 100644
--- a/lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.cpp
@@ -350,18 +350,17 @@ ObjCLanguageRuntime::EncodingToTypeSP ObjCLanguageRuntime::GetEncodingToType() {
return nullptr;
}
-bool ObjCLanguageRuntime::GetTypeBitSize(const CompilerType &compiler_type,
- uint64_t &size) {
+std::optional<uint64_t>
+ObjCLanguageRuntime::GetTypeBitSize(const CompilerType &compiler_type) {
void *opaque_ptr = compiler_type.GetOpaqueQualType();
- size = m_type_size_cache.Lookup(opaque_ptr);
- // an ObjC object will at least have an ISA, so 0 is definitely not OK
- if (size > 0)
- return true;
+ uint64_t cached_size = m_type_size_cache.Lookup(opaque_ptr);
+ if (cached_size > 0)
+ return cached_size;
ClassDescriptorSP class_descriptor_sp =
GetClassDescriptorFromClassName(compiler_type.GetTypeName());
if (!class_descriptor_sp)
- return false;
+ return {};
int32_t max_offset = INT32_MIN;
uint64_t sizeof_max = 0;
@@ -377,11 +376,13 @@ bool ObjCLanguageRuntime::GetTypeBitSize(const CompilerType &compiler_type,
}
}
- size = 8 * (max_offset + sizeof_max);
- if (found)
+ uint64_t size = 8 * (max_offset + sizeof_max);
+ if (found && size > 0) {
m_type_size_cache.Insert(opaque_ptr, size);
+ return size;
+ }
- return found;
+ return {};
}
lldb::BreakpointPreconditionSP
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h b/lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h
index 0a8b6e8d56ed0..ffe9725fa6826 100644
--- a/lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h
+++ b/lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h
@@ -107,7 +107,7 @@ class ObjCLanguageRuntime : public LanguageRuntime {
int64_t *value_bits = nullptr,
uint64_t *payload = nullptr) = 0;
/// @}
-
+
virtual uint64_t GetInstanceSize() = 0;
// use to implement version-specific additional constraints on pointers
@@ -321,8 +321,8 @@ class ObjCLanguageRuntime : public LanguageRuntime {
m_negative_complete_class_cache.clear();
}
- bool GetTypeBitSize(const CompilerType &compiler_type,
- uint64_t &size) override;
+ std::optional<uint64_t>
+ GetTypeBitSize(const CompilerType &compiler_type) override;
/// Check whether the name is "self" or "_cmd" and should show up in
/// "frame variable".
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index dbe6238d4fe5a..f9cdf357acdde 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -4744,11 +4744,12 @@ TypeSystemClang::GetBitSize(lldb::opaque_compiler_type_t type,
ExecutionContext exe_ctx(exe_scope);
Process *process = exe_ctx.GetProcessPtr();
if (process) {
- ObjCLanguageRuntime *objc_runtime = ObjCLanguageRuntime::Get(*process);
- if (objc_runtime) {
- uint64_t bit_size = 0;
- if (objc_runtime->GetTypeBitSize(GetType(qual_type), bit_size))
- return bit_size;
+ if (ObjCLanguageRuntime *objc_runtime =
+ ObjCLanguageRuntime::Get(*process)) {
+ std::optional<uint64_t> bit_size =
+ objc_runtime->GetTypeBitSize(GetType(qual_type));
+ if (bit_size)
+ return *bit_size;
}
} else {
static bool g_printed = false;
|
Michael137
approved these changes
Jun 19, 2024
Comment on lines
4749
to
4752
| std::optional<uint64_t> bit_size = | ||
| objc_runtime->GetTypeBitSize(GetType(qual_type)); | ||
| if (bit_size) | ||
| return *bit_size; |
There was a problem hiding this comment.
Couldn't this just be:
Suggested change
| std::optional<uint64_t> bit_size = | |
| objc_runtime->GetTypeBitSize(GetType(qual_type)); | |
| if (bit_size) | |
| return *bit_size; | |
| return objc_runtime->GetTypeBitSize(GetType(qual_type)); |
?
There was a problem hiding this comment.
That might be the right thing to do, but would be a change in behavior, as now, if it fails, we'll fall through to the default case first.
There was a problem hiding this comment.
Aah you're right, missed the fallthrough here. Could fold the GetTypeBitSize call into the if-statement, like you did for ObjCLanguageRuntime::Get. But either way, LGTM
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
…lvm#96013) Make LanguageRuntime::GetTypeBitSize return an optional. This should be NFC, though the ObjCLanguageRuntime implementation is (possibly) more defensive against returning 0. I'm not sure if it's possible for both `m_ivar.size` and `m_ivar.offset` to be zero. Previously, we'd return 0 and cache it, only to discard it the next time when finding it in the cache, and recomputing it again. The new code will avoid putting it in the cache in the first place.
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.
Make LanguageRuntime::GetTypeBitSize return an optional. This should be NFC, though the ObjCLanguageRuntime implementation is (possibly) more defensive against returning 0.
I'm not sure if it's possible for both
m_ivar.sizeandm_ivar.offsetto be zero. Previously, we'd return 0 and cache it, only to discard it the next time when finding it in the cache, and recomputing it again. The new code will avoid putting it in the cache in the first place.