[Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
This revision was automatically updated to reflect the committed changes.
Closed by commit rL356048: Fix/unify SBType comparison (authored by labath,
committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59217/new/
https://reviews.llvm.org/D59217
Files:
lldb/trunk/include/lldb/Symbol/Type.h
lldb/trunk/scripts/Python/modify-python-lldb.py
lldb/trunk/scripts/interface/SBType.i
Index: lldb/trunk/scripts/Python/modify-python-lldb.py
===
--- lldb/trunk/scripts/Python/modify-python-lldb.py
+++ lldb/trunk/scripts/Python/modify-python-lldb.py
@@ -143,7 +143,6 @@
'SBWatchpoint': ['GetID'],
'SBFileSpec': ['GetFilename', 'GetDirectory'],
'SBModule': ['GetFileSpec', 'GetUUIDString'],
- 'SBType': ['GetByteSize', 'GetName']
}
Index: lldb/trunk/scripts/interface/SBType.i
===
--- lldb/trunk/scripts/interface/SBType.i
+++ lldb/trunk/scripts/interface/SBType.i
@@ -322,6 +322,10 @@
uint32_t
GetTypeFlags ();
+bool operator==(lldb::SBType );
+
+bool operator!=(lldb::SBType );
+
%pythoncode %{
def template_arg_array(self):
num_args = self.num_template_args
Index: lldb/trunk/include/lldb/Symbol/Type.h
===
--- lldb/trunk/include/lldb/Symbol/Type.h
+++ lldb/trunk/include/lldb/Symbol/Type.h
@@ -256,14 +256,10 @@
explicit operator bool() const { return IsValid(); }
bool operator==(const TypePair ) const {
-return compiler_type == rhs.compiler_type &&
- type_sp.get() == rhs.type_sp.get();
+return compiler_type == rhs.compiler_type;
}
- bool operator!=(const TypePair ) const {
-return compiler_type != rhs.compiler_type ||
- type_sp.get() != rhs.type_sp.get();
- }
+ bool operator!=(const TypePair ) const { return !(*this == rhs); }
void Clear() {
compiler_type.Clear();
Index: lldb/trunk/scripts/Python/modify-python-lldb.py
===
--- lldb/trunk/scripts/Python/modify-python-lldb.py
+++ lldb/trunk/scripts/Python/modify-python-lldb.py
@@ -143,7 +143,6 @@
'SBWatchpoint': ['GetID'],
'SBFileSpec': ['GetFilename', 'GetDirectory'],
'SBModule': ['GetFileSpec', 'GetUUIDString'],
- 'SBType': ['GetByteSize', 'GetName']
}
Index: lldb/trunk/scripts/interface/SBType.i
===
--- lldb/trunk/scripts/interface/SBType.i
+++ lldb/trunk/scripts/interface/SBType.i
@@ -322,6 +322,10 @@
uint32_t
GetTypeFlags ();
+bool operator==(lldb::SBType );
+
+bool operator!=(lldb::SBType );
+
%pythoncode %{
def template_arg_array(self):
num_args = self.num_template_args
Index: lldb/trunk/include/lldb/Symbol/Type.h
===
--- lldb/trunk/include/lldb/Symbol/Type.h
+++ lldb/trunk/include/lldb/Symbol/Type.h
@@ -256,14 +256,10 @@
explicit operator bool() const { return IsValid(); }
bool operator==(const TypePair ) const {
-return compiler_type == rhs.compiler_type &&
- type_sp.get() == rhs.type_sp.get();
+return compiler_type == rhs.compiler_type;
}
- bool operator!=(const TypePair ) const {
-return compiler_type != rhs.compiler_type ||
- type_sp.get() != rhs.type_sp.get();
- }
+ bool operator!=(const TypePair ) const { return !(*this == rhs); }
void Clear() {
compiler_type.Clear();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
labath added a comment. Thanks. I'm going to commit this, and then see whether it is possible to remove the type_sp member. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59217/new/ https://reviews.llvm.org/D59217 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
The main reason for this that I can see is if we ever want to provide a lldb::user_id_t for a given SBType. We will lose that ability if we remove, but I am ok with this because we can force the TypeSystem to be able to remember this in metadata if we ever do need it. Why? In DWARF that most compilers produce, they produce many copies of a type, up to one in each compile unit within an symbol file. If you somehow get ahold of a type via the lldb::SB API right now, there is no way to figure out which one it picked from those many copies. So not a big deal if we remove this as we have not exposed "lldb::user_id_t lldb::SBType::GetID()" in the API yet, nor do we seem to have a need for it. You might keep the constructor that takes a type_sp just to keep the diffs down though, that ctor will extract the compiler type form the type_sp and not store it. Greg > On Mar 11, 2019, at 4:32 PM, Jim Ingham via lldb-commits > wrote: > > Ah, I see. This doesn't seem terribly confusing, TypePair manages the > TypeSP & CompilerType and keeps them in sync, so it just allows you to > provide a higher quality representation if you have it. A lot of SBValues > come from debug information so they will have TypeSP's around when they get > made. > > But if you want to try simplifying things, that's also good. Since this is > caching the TypeSP we need to include "doesn't slow down debugging" to the > things that don't break, but we don't have a good way to measure that right > now. > > Jim > > >> On Mar 11, 2019, at 4:17 PM, Zachary Turner wrote: >> >> >> >> On Mon, Mar 11, 2019 at 4:15 PM Jim Ingham via lldb-commits >> wrote: >> Just to be precise: TypeImpl stores a TypePair for the static type and a >> CompilerType for the dynamic type. These two have different meanings. >> There's no assumption about the relationship between the static and dynamic >> type. In ObjC, the dynamic type need not even be in the same class >> hierarchy as the static type. That's there so that if an SBValue hands out >> a type, it can represent both the static and dynamic types of the value it >> comes from. >> >> I'm not sure why the static type is a TypePair and the dynamic type is a >> CompilerType, however. >> >> The TypePair stores a TypeSP and a CompilerType that are supposed to be the >> same type. It doesn't look like there is any way for those two to get out >> of sync, but I'm not entirely sure why it helps to have both in the same >> object. Presumably it's caching? >> >> This was what i meant. It seems that in the case of SBType, nothing depends >> on the type_sp member of the pair, only the CompilerType. >> >> I think the reason why both are in the same object is so that if you >> initialize it with a TypeSP you have a superset of functionality available >> than if you initialize it from a CompilerType. But, if nobody actually >> requires this, then it simplifies the interface and makes it easier to >> reason about to just store a CompilerType for the static type. >> >> Like Greg said though, it should be easy to see if it breaks anything by >> just changing the static type from TypePair to CompilerType and then fixing >> up the code and seeing if anything breaks. > > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
Ah, I see. This doesn't seem terribly confusing, TypePair manages the TypeSP & CompilerType and keeps them in sync, so it just allows you to provide a higher quality representation if you have it. A lot of SBValues come from debug information so they will have TypeSP's around when they get made. But if you want to try simplifying things, that's also good. Since this is caching the TypeSP we need to include "doesn't slow down debugging" to the things that don't break, but we don't have a good way to measure that right now. Jim > On Mar 11, 2019, at 4:17 PM, Zachary Turner wrote: > > > > On Mon, Mar 11, 2019 at 4:15 PM Jim Ingham via lldb-commits > wrote: > Just to be precise: TypeImpl stores a TypePair for the static type and a > CompilerType for the dynamic type. These two have different meanings. > There's no assumption about the relationship between the static and dynamic > type. In ObjC, the dynamic type need not even be in the same class hierarchy > as the static type. That's there so that if an SBValue hands out a type, it > can represent both the static and dynamic types of the value it comes from. > > I'm not sure why the static type is a TypePair and the dynamic type is a > CompilerType, however. > > The TypePair stores a TypeSP and a CompilerType that are supposed to be the > same type. It doesn't look like there is any way for those two to get out of > sync, but I'm not entirely sure why it helps to have both in the same object. > Presumably it's caching? > > This was what i meant. It seems that in the case of SBType, nothing depends > on the type_sp member of the pair, only the CompilerType. > > I think the reason why both are in the same object is so that if you > initialize it with a TypeSP you have a superset of functionality available > than if you initialize it from a CompilerType. But, if nobody actually > requires this, then it simplifies the interface and makes it easier to reason > about to just store a CompilerType for the static type. > > Like Greg said though, it should be easy to see if it breaks anything by just > changing the static type from TypePair to CompilerType and then fixing up the > code and seeing if anything breaks. ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
On Mon, Mar 11, 2019 at 4:15 PM Jim Ingham via lldb-commits < lldb-commits@lists.llvm.org> wrote: > Just to be precise: TypeImpl stores a TypePair for the static type and a > CompilerType for the dynamic type. These two have different meanings. > There's no assumption about the relationship between the static and dynamic > type. In ObjC, the dynamic type need not even be in the same class > hierarchy as the static type. That's there so that if an SBValue hands out > a type, it can represent both the static and dynamic types of the value it > comes from. > > I'm not sure why the static type is a TypePair and the dynamic type is a > CompilerType, however. > > The TypePair stores a TypeSP and a CompilerType that are supposed to be > the same type. It doesn't look like there is any way for those two to get > out of sync, but I'm not entirely sure why it helps to have both in the > same object. Presumably it's caching? > This was what i meant. It seems that in the case of SBType, nothing depends on the type_sp member of the pair, only the CompilerType. I think the reason why both are in the same object is so that if you initialize it with a TypeSP you have a superset of functionality available than if you initialize it from a CompilerType. But, if nobody actually requires this, then it simplifies the interface and makes it easier to reason about to just store a CompilerType for the static type. Like Greg said though, it should be easy to see if it breaks anything by just changing the static type from TypePair to CompilerType and then fixing up the code and seeing if anything breaks. ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
clayborg added a comment. If no one is depending on type_sp, then ok to delete. Should be easy to test by removing it and seeing what breaks (if anything). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59217/new/ https://reviews.llvm.org/D59217 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
Just to be precise: TypeImpl stores a TypePair for the static type and a CompilerType for the dynamic type. These two have different meanings. There's no assumption about the relationship between the static and dynamic type. In ObjC, the dynamic type need not even be in the same class hierarchy as the static type. That's there so that if an SBValue hands out a type, it can represent both the static and dynamic types of the value it comes from. I'm not sure why the static type is a TypePair and the dynamic type is a CompilerType, however. The TypePair stores a TypeSP and a CompilerType that are supposed to be the same type. It doesn't look like there is any way for those two to get out of sync, but I'm not entirely sure why it helps to have both in the same object. Presumably it's caching? Jim > On Mar 11, 2019, at 10:32 AM, Zachary Turner via Phabricator via lldb-commits > wrote: > > zturner added a comment. > > SBType storing a pair of CompilerType and TypeSP seems like a very confusing > interface and like it will be very easy to misuse or violate assumptions > (perhaps even assumptions that nobody knows exists). Why exactly is this > necessary? > > As far as I can tell, `SBType` uses `TypeImpl`, which supports having a > `CompilerType`, a `TypeSP`, or both, but I cannot find any client of > `TypeImpl` which actually depends on the `TypeSP` being set. Perhaps we can > just delete support for `TypeSP` from `TypeImpl` entirely. > > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D59217/new/ > > https://reviews.llvm.org/D59217 > > > > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
zturner added a comment. SBType storing a pair of CompilerType and TypeSP seems like a very confusing interface and like it will be very easy to misuse or violate assumptions (perhaps even assumptions that nobody knows exists). Why exactly is this necessary? As far as I can tell, `SBType` uses `TypeImpl`, which supports having a `CompilerType`, a `TypeSP`, or both, but I cannot find any client of `TypeImpl` which actually depends on the `TypeSP` being set. Perhaps we can just delete support for `TypeSP` from `TypeImpl` entirely. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59217/new/ https://reviews.llvm.org/D59217 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
aprantl added a comment. Interesting find. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59217/new/ https://reviews.llvm.org/D59217 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D59217: Fix/unify SBType comparison
labath created this revision.
labath added reviewers: clayborg, aprantl.
Herald added a reviewer: serge-sans-paille.
Herald added a subscriber: jdoerfert.
In my next step at cleaning up modify-python-lldb.py, I started focusing
on equality comparison. To my surprise, I found out that both python and
c++ versions of the SBType class implement equality comparison, but each
one does it differently. While the python version was implemented in
terms of type name equality, the C++ one used a deep comparison on the
underlying objects.
Removing the python version caused one test to fail (TestTypeList). This
happened because the c++ version of operator== boiled down to
TypePair::operator==, which contains two items: the compiler_type and
type_sp. In this case, the compiler_type was identical, but one of the
objects had the type_sp field unset.
I tried fixing the code so that both objects keep their type_sp member,
but it wasn't easy, because there are so many operations which just work
with the CompilerType types, and so any operation on the SBType (the
test in question was doing GetPointeeType on the type of one variable
and expecting it to match the type of another variable), cause that
second member to be lost.
So instead, I tried to relax the equality comparison on the TypePair
class. Now, this class ignores the type_sp for the purposes of
comparison, and uses the CompilerType only. This seems reasonable, as
each TypeSP is able to convert itself to a CompilerType, though I don't
understand the full implications of this (e.g., is there any case where
two different TypeSPs might end up returning different CompilerTypes).
https://reviews.llvm.org/D59217
Files:
include/lldb/Symbol/Type.h
scripts/Python/modify-python-lldb.py
scripts/interface/SBType.i
Index: scripts/interface/SBType.i
===
--- scripts/interface/SBType.i
+++ scripts/interface/SBType.i
@@ -322,6 +322,10 @@
uint32_t
GetTypeFlags ();
+bool operator==(lldb::SBType );
+
+bool operator!=(lldb::SBType );
+
%pythoncode %{
def template_arg_array(self):
num_args = self.num_template_args
Index: scripts/Python/modify-python-lldb.py
===
--- scripts/Python/modify-python-lldb.py
+++ scripts/Python/modify-python-lldb.py
@@ -143,7 +143,6 @@
'SBWatchpoint': ['GetID'],
'SBFileSpec': ['GetFilename', 'GetDirectory'],
'SBModule': ['GetFileSpec', 'GetUUIDString'],
- 'SBType': ['GetByteSize', 'GetName']
}
Index: include/lldb/Symbol/Type.h
===
--- include/lldb/Symbol/Type.h
+++ include/lldb/Symbol/Type.h
@@ -256,14 +256,10 @@
explicit operator bool() const { return IsValid(); }
bool operator==(const TypePair ) const {
-return compiler_type == rhs.compiler_type &&
- type_sp.get() == rhs.type_sp.get();
+return compiler_type == rhs.compiler_type;
}
- bool operator!=(const TypePair ) const {
-return compiler_type != rhs.compiler_type ||
- type_sp.get() != rhs.type_sp.get();
- }
+ bool operator!=(const TypePair ) const { return !(*this == rhs); }
void Clear() {
compiler_type.Clear();
Index: scripts/interface/SBType.i
===
--- scripts/interface/SBType.i
+++ scripts/interface/SBType.i
@@ -322,6 +322,10 @@
uint32_t
GetTypeFlags ();
+bool operator==(lldb::SBType );
+
+bool operator!=(lldb::SBType );
+
%pythoncode %{
def template_arg_array(self):
num_args = self.num_template_args
Index: scripts/Python/modify-python-lldb.py
===
--- scripts/Python/modify-python-lldb.py
+++ scripts/Python/modify-python-lldb.py
@@ -143,7 +143,6 @@
'SBWatchpoint': ['GetID'],
'SBFileSpec': ['GetFilename', 'GetDirectory'],
'SBModule': ['GetFileSpec', 'GetUUIDString'],
- 'SBType': ['GetByteSize', 'GetName']
}
Index: include/lldb/Symbol/Type.h
===
--- include/lldb/Symbol/Type.h
+++ include/lldb/Symbol/Type.h
@@ -256,14 +256,10 @@
explicit operator bool() const { return IsValid(); }
bool operator==(const TypePair ) const {
-return compiler_type == rhs.compiler_type &&
- type_sp.get() == rhs.type_sp.get();
+return compiler_type == rhs.compiler_type;
}
- bool operator!=(const TypePair ) const {
-return compiler_type != rhs.compiler_type ||
- type_sp.get() != rhs.type_sp.get();
- }
+ bool operator!=(const TypePair ) const { return !(*this == rhs); }
void Clear() {
compiler_type.Clear();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
