Re: [lldb-dev] clang::VersionTuple

[Pavel Labath via lldb-dev]

Thank you all for the feedback. I'll get on with this when I find some
spare time. I will send a patch which will show the final look of the code,
before I start moving VersionTuple into llvm.

cheers,
pl
On Tue, 8 May 2018 at 19:46, Frédéric Riss via lldb-dev <
lldb-dev@lists.llvm.org> wrote:



> On May 8, 2018, at 11:37 AM, Greg Clayton  wrote:

> I was referring to the Swift and Apple internal branches. They tend to
lock down against older llvm and clang repositories so when we put changes
in llvm or clang that are required for LLDB, it makes merging a bit tougher
in those cases. Again, I am not affected by this, just trying to watch out
for you guys.


> I understand and I appreciate it, I was just worried that I’m missing
something obvious. We branch LLDB at the same time as LLVM so that’s not
actually an issue. Of course, it might cause merge conflicts or make it
harder to cherry-pick patches, but that's just living downstream.

> Fred

> Greg


> On May 8, 2018, at 11:35 AM, Greg Clayton  wrote:

> I'm good if Apple is good.

> On May 8, 2018, at 11:31 AM, Frédéric Riss  wrote:



> On May 8, 2018, at 10:04 AM, Greg Clayton via lldb-dev <
lldb-dev@lists.llvm.org> wrote:



> On May 8, 2018, at 9:47 AM, Zachary Turner  wrote:

> We don’t want the lowest levels of lldb to depend on clang. If this is
useful we should move it from clang to llvm and use llvm::VersionTuple


> I agree, though this move will cause merging issues for many that have
repositories that link against older llvm/clang. Doesn't affect me anymore,
but Apple will be affected.


> I’m not sure I understand what issues you’re referring to, we don’t link
new LLDBs to old clangs (and even if we did, it wouldn’t be something the
that drives community decisions).

> Fred

> Greg

> On Tue, May 8, 2018 at 9:26 AM Greg Clayton via lldb-dev <
lldb-dev@lists.llvm.org> wrote:

>> No issues from me.

>> > On May 8, 2018, at 9:11 AM, Pavel Labath via lldb-dev <
lldb-dev@lists.llvm.org> wrote:
>> >
>> > While moving Args around, I noticed that we have a bunch of
>> > functions/classes that pass/store version numbers as a triplet of
integers
>> > (e.g. Platform::GetOSVersion). I got halfway into creating a wrapper
class
>> > for that when I noticed clang::VersionTuple, which is pretty much what
I
>> > wanted out of the box.
>> >
>> > Now there are small differences between this class, and what we have
now:
>> > it has an extra fourth "build" field, and it uses only 31 bits to
represent
>> >  the values. None of these seem to matter (particularly as we are
>> > converting our representation into this struct in some places) that
much,
>> > but before I go through the trouble of pulling this class into llvm
>> > (although technically possible, it seems wrong to pull a clang
dependency
>> > at such a low level), I wanted to make sure we are able to use it.
>> >
>> > Do you see any reason why we could not replace our version triplets
with
>> > clang::VersionTuple ?
>> >
>> > cheers,
>> > pl
>> > ___
>> > lldb-dev mailing list
>> > lldb-dev@lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

>> ___
>> lldb-dev mailing list
>> lldb-dev@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev


> ___
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev




> ___
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] What should SymbolFile::FindFunctions(..., eFunctionNameTypeFull, ...) do ?

[Pavel Labath via lldb-dev]

On Tue, 8 May 2018 at 10:51, Pavel Labath  wrote:


> On Fri, 4 May 2018 at 17:31, Greg Clayton  wrote:
> > > On May 4, 2018, at 4:58 AM, Pavel Labath  wrote:
> > > Is it really OK? If our indexes will never contain the demangled
names,
> > > then the IRExecutionUnit lookups using the demangled names will always
> > > fail. (Right now they will only succeed for manually indexed dwarf,
but
> > > this will change if I stop putting these names in the full index)
> Shouldn't
> > > we fix the IRExecutionUnit to not attempt these lookups in the first
> place?

> > Yikes. I wasn't aware this was happening. What is the flow here? It
tries
> to lookup using a mangled name first and if that fails, then it tries to
> demangle the name and then look that up? Are there cases where we don't
> have mangled names in the debug info, yet we are able to construct a fully
> qualified name and look that up?

> > It would be great if we don't need these fully qualified name lookups,
> but if we do, we could fix this by looking up using the basename, then
> filtering the results of the lookup to only those that match in the IR
code.

> What happens here is that IRExecutionUnit gets a mangled name, and then
for
> some reason decides to demangle it and try a lookup that way (see
> IRExecutionUnit::CollectCandidateCPlusPlusNames). The history is not
really
> clear about why this is done -- this code has been there since the
function
> was introduced in r260734. After I remove the indexing of demangled names
> in the manual index, this code should become effectively dead. I'll try
> removing it after that.


So, I tried removing the demangled lookup in IRExecutionUnit, thinking that
would be a no-op. Turns out that was only almost true. The reason it was
not a no-op is because we have another "full name" index which contains
demangled names inside the Symtab class (Symtab.cpp:341). That index also
contains the mangled names, so normally the demangled names should not
matter.
However, they do matter for one particular case: constructors. Clang on
linux likes to omit the the symbol for the full object constructor if it
turns out to be identical to the base object constructor. Since these
demangle to the same name, we manage to find the base constructor when
looking for the full one. This is what makes calling constructors in
expressions "work" most of the time.

So, my question is: Should we change the Symtab index to not include the
demangled names as well? The constructor issue seems to be the only thing
it is useful, but that can be handled in a different way as well.
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] What should SymbolFile::FindFunctions(..., eFunctionNameTypeFull, ...) do ?

[Greg Clayton via lldb-dev]

> On May 9, 2018, at 4:53 AM, Pavel Labath  wrote:
> 
> On Tue, 8 May 2018 at 10:51, Pavel Labath  > wrote:
> 
> 
>> On Fri, 4 May 2018 at 17:31, Greg Clayton  wrote:
 On May 4, 2018, at 4:58 AM, Pavel Labath  wrote:
 Is it really OK? If our indexes will never contain the demangled
> names,
 then the IRExecutionUnit lookups using the demangled names will always
 fail. (Right now they will only succeed for manually indexed dwarf,
> but
 this will change if I stop putting these names in the full index)
>> Shouldn't
 we fix the IRExecutionUnit to not attempt these lookups in the first
>> place?
> 
>>> Yikes. I wasn't aware this was happening. What is the flow here? It
> tries
>> to lookup using a mangled name first and if that fails, then it tries to
>> demangle the name and then look that up? Are there cases where we don't
>> have mangled names in the debug info, yet we are able to construct a fully
>> qualified name and look that up?
> 
>>> It would be great if we don't need these fully qualified name lookups,
>> but if we do, we could fix this by looking up using the basename, then
>> filtering the results of the lookup to only those that match in the IR
> code.
> 
>> What happens here is that IRExecutionUnit gets a mangled name, and then
> for
>> some reason decides to demangle it and try a lookup that way (see
>> IRExecutionUnit::CollectCandidateCPlusPlusNames). The history is not
> really
>> clear about why this is done -- this code has been there since the
> function
>> was introduced in r260734. After I remove the indexing of demangled names
>> in the manual index, this code should become effectively dead. I'll try
>> removing it after that.
> 
> 
> So, I tried removing the demangled lookup in IRExecutionUnit, thinking that
> would be a no-op. Turns out that was only almost true. The reason it was
> not a no-op is because we have another "full name" index which contains
> demangled names inside the Symtab class (Symtab.cpp:341). That index also
> contains the mangled names, so normally the demangled names should not
> matter.
> However, they do matter for one particular case: constructors. Clang on
> linux likes to omit the the symbol for the full object constructor if it
> turns out to be identical to the base object constructor. Since these
> demangle to the same name, we manage to find the base constructor when
> looking for the full one. This is what makes calling constructors in
> expressions "work" most of the time.
> 
> So, my question is: Should we change the Symtab index to not include the
> demangled names as well? The constructor issue seems to be the only thing
> it is useful, but that can be handled in a different way as well.

I am all for that. It would be great to not have to store the fully demangled 
name in indexes, it would save memory and time. As long as we can still find 
the constructors, assignment operators and other default created C++ functions 
in another way.

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev