[Lldb-commits] [RFC] Type lookup for template types is broken...
via lldb-commits
lldb-commits at lists.llvm.org
Tue May 8 10:27:09 PDT 2018
Clang can control the emission of <params> or not in the name, using the "debugger tuning" feature. It sounds like neither LLDB nor PS4 will mind losing the <params> in the name, and it makes the indexing simpler. If the tuning is set for GDB then we can still emit the <params> in the names.
It would make me uncomfortable to have the index and the actual DWARF be inconsistent, but given the way templates work in general (a variety of spellings all refer to the same type) it is probably better to use the un-decorated name in the index even if the DWARF itself used decorated names. This should probably be proposed as a revision for DWARF 6.
--paulr
From: Greg Clayton [mailto:clayborg at gmail.com]
Sent: Tuesday, May 08, 2018 12:25 PM
To: Robinson, Paul
Cc: friss at apple.com; lldb-commits at lists.llvm.org
Subject: Re: [Lldb-commits] [RFC] Type lookup for template types is broken...
I think for display purposes, the type name should include all of the <params>. If I have two variables, both using class C, but both have different template parameters, I want to see that in the class name and have that properly show up in the variable view. LLDB doesn't actually care about the name that is in the DWARF because we will show the type name by grabbing it from the lldb_private::CompilerType which will end up being correct. Other debuggers might just display "C" if that is what is in the DWARF. So I would caution against just putting the type basename without params in the DWARF itself.
Greg
On May 8, 2018, at 9:12 AM, <paul.robinson at sony.com<mailto:paul.robinson at sony.com>> <paul.robinson at sony.com<mailto:paul.robinson at sony.com>> wrote:
So…. if the name in the type entry did not include the <params> then the indexing would automatically do what you want; you would need to reconstruct the <params> from the template parameter children of the DIE. Would that help?
On the LLVM side we have debated the merits of <params> in the name back and forth. I would have to investigate the current state of the world; I know that for PS4 we prefer to have the children and not put <params> in the name, while others prefer it the other way around. If LLDB came out preferring not to have a decorated name, it should not be hard to accommodate that in Clang.
Not sure where gcc/gdb stand on this point.
--paulr
From: friss at apple.com<mailto:friss at apple.com> [mailto:friss at apple.com]
Sent: Tuesday, May 08, 2018 12:07 PM
To: Greg Clayton
Cc: Robinson, Paul; lldb-commits at lists.llvm.org<mailto:lldb-commits at lists.llvm.org>
Subject: Re: [Lldb-commits] [RFC] Type lookup for template types is broken...
On May 8, 2018, at 9:04 AM, Greg Clayton <clayborg at gmail.com<mailto:clayborg at gmail.com>> wrote:
The only way for us to find all classes whose type is "C" is to add the entry for all template classes named "C", so I would vote to add them as it is accurate. Do we currently add one for "C<12, 16>”?
Yes we do, not sure it is actually useful.
Fred
Greg
On May 8, 2018, at 8:58 AM, Frédéric Riss via lldb-commits <lldb-commits at lists.llvm.org<mailto:lldb-commits at lists.llvm.org>> wrote:
On May 8, 2018, at 8:30 AM, paul.robinson at sony.com<mailto:paul.robinson at sony.com> wrote:
-----Original Message-----
From: lldb-commits [mailto:lldb-commits-bounces at lists.llvm.org] On Behalf
Of Pavel Labath via lldb-commits
Sent: Tuesday, May 08, 2018 10:48 AM
To: friss at apple.com<mailto:friss at apple.com>
Cc: lldb-commits at lists.llvm.org<mailto:lldb-commits at lists.llvm.org>
Subject: Re: [Lldb-commits] [RFC] Type lookup for template types is
broken...
Well.. it encodes some assumptions about how a class name looks like,
which
are probably valid for C++, but they don't have to hold for any language
frontend LLVM supports. That said, I am not saying this is worth the
trouble of adding a special "these are the additional names you are to
insert into the index" channel that clang should use to communicate this
(I
wouldn't be surprised if we make even stronger assumptions elsewhere). I
was just curious about what your thoughts here were.
If you add an accelerator entry for "C" what does it point to? All the
instantiations of "C"? The DWARF does not describe the template, only
the concrete instances.
Yes, there would be a “C” entry for every instantiation of C.
Fred
--paulr
On Tue, 8 May 2018 at 15:29, Frédéric Riss <friss at apple.com<mailto:friss at apple.com>> wrote:
On May 8, 2018, at 2:23 AM, Pavel Labath <labath at google.com<mailto:labath at google.com>> wrote:
I am still building a picture for myself of how the accelerator tables
and
our name lookup works, but from what I managed to learn so far, adding
an
accelerator for "C" seems like a useful thing to do. However, this does
go
beyond what the DWARF 5 spec says we should do (we are only required to
add
the DW_AT_name string). We are still free to add any extra entries we
like,
but if we're going to be relying on this, we should try to get some of
this
into the next version of the spec.
On Mon, 7 May 2018 at 22:19, Frédéric Riss via lldb-commits <
lldb-commits at lists.llvm.org<mailto:lldb-commits at lists.llvm.org>> wrote:
(...At least when using accelerator tables)
If you apply the following patch, TestClassTemplateParameterPack.py will
start failing:
diff --git
a/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
pack/main.cpp
b/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
pack/main.cpp
index 90e63b40f..304872a15 100644
---
a/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
pack/main.cpp
+++
b/packages/Python/lldbsuite/test/lang/cpp/class-template-parameter-
pack/main.cpp
@@ -37,7 +37,7 @@ template <> struct D<int, int, bool> : D<int, int> {
int main (int argc, char const *argv[])
{
- C<int,16,32> myC;
+ C<int,16,32> myC; //% self.runCmd("settings set
target.experimental.inject-local-vars false")
C<int,16> myLesserC;
myC.member = 64;
(void)C<int,16,32>().isSixteenThirtyTwo();
The test does things like invoke methods on temporary template objects:
//% self.expect("expression -- C<int, 16>().isSixteenThirtyTwo()",
DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["false"])
The above expression currently works because there’s a local of type
C<int, 16>. With injected locals, the type is made readily available to
Clang. No type lookup is required for this to work in this setup.
If you stop injecting locals, the test fails. We don’t provide the
information to Clang to understand what C is. The reason is that when
Clang
parses “C<int , 16>”, it is going to ask about “C”, not the fully
templated
name. Our accelerator tables contain references to the full names, but
not
to C alone and we never find it. If I change Clang and dsymutil to add
an
accelerator for “C” each time an instance of C is seen then it nearly
works. I just need this additional lldb patch:
diff --git a/source/Symbol/TypeMap.cpp b/source/Symbol/TypeMap.cpp
index 2838039ad..d2f2026bf 100644
--- a/source/Symbol/TypeMap.cpp
+++ b/source/Symbol/TypeMap.cpp
@@ -227,8 +227,11 @@ void TypeMap::RemoveMismatchedTypes(const
std::string &type_scope,
} else {
// The type we are currently looking at doesn't exists in a
namespace
// or class, so it only matches if there is no type scope...
- keep_match =
- type_scope.empty() &&
type_basename.compare(match_type_name)
== 0;
+ if (type_scope.empty()) {
+ keep_match = type_basename.compare(match_type_name) == 0 ||
+ (strlen(match_type_name) > type_basename.size() &&
+ match_type_name[type_basename.size()] == '<');
+ }
}
}
I didn’t post this as a Phabricator review as it requires changes in
llvm
before doing anything in LLDB and I wanted to make sure we agree this is
the right thing to do. I’m also not sure if this works out of the box on
platforms without accelerator tables.
It won't work "out of the box", but it should be fairly simple to change
our indexing code to add the extra entries, so that a lookup for "C"
works
the same way in both cases. BTW, how were you planning to compute the
untemplated string ("C"). Will you just strip everything after the first
'<' character, or were you thinking of something more fancy?
AFAIK, there are no fully qualified names in the debug info we generate
so taking what’s before the first ‘<‘ should always return the class name.
Does this logic seem flawed?
Fred
_______________________________________________
lldb-commits mailing list
lldb-commits at lists.llvm.org<mailto:lldb-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
_______________________________________________
lldb-commits mailing list
lldb-commits at lists.llvm.org<mailto:lldb-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180508/71d860af/attachment-0001.html>
More information about the lldb-commits
mailing list