This revision was automatically updated to reflect the committed changes.
Closed by commit rL328658: Use the DWARF linkage name when importing C++
methods. (authored by davide, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D40283?vs=1
davide added a comment.
As there are no strong objections, I'm going to check this in tomorrow PST and
see how it goes.
Thanks for your contribution.
https://reviews.llvm.org/D40283
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://
clayborg added a comment.
In https://reviews.llvm.org/D40283#983997, @aprantl wrote:
> As Greg pointed out C++ (and Swift) mangled names can be enormous, so it
> would definitely have an impact on the memory footprint (unless we are only
> passing `StringRef`s around. Are we?)
Most strings se
aprantl added a comment.
As Greg pointed out C++ (and Swift) mangled names can be enormous, so it would
definitely have an impact on the memory footprint (unless we are only passing
`StringRef`s around. Are we?)
https://reviews.llvm.org/D40283
___
nelhage added a comment.
> Thanks Greg. I'll wait another day and check this one in. Nelson, does the
> plan make sense to you?
Yep, sounds great. Thanks so much.
I'd prefer to not gate this on !Darwin, since the added complexity seems
unfortunate, and this isn't, strictly-speaking, only about
davide added a comment.
In https://reviews.llvm.org/D40283#983946, @aprantl wrote:
> In https://reviews.llvm.org/D40283#983908, @clayborg wrote:
>
> > I am fine with checking this. The only issue on my end is the extra memory
> > that will be needed to store these often huge mangled names in eve
aprantl added a comment.
In https://reviews.llvm.org/D40283#983908, @clayborg wrote:
> I am fine with checking this. The only issue on my end is the extra memory
> that will be needed to store these often huge mangled names in every AST
> context for the 0.0001% of the time it is actually neede
davide accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.
Thanks Greg. I'll wait another day and check this one in. Nelson, does the plan
make sense to you?
https://reviews.llvm.org/D40283
___
lldb-comm
clayborg added a comment.
I am fine with checking this. The only issue on my end is the extra memory that
will be needed to store these often huge mangled names in every AST context for
the 0.0001% of the time it is actually needed.
https://reviews.llvm.org/D40283
__
nelhage added a comment.
Yes, I will need someone else to commit on my behalf -- I don't have commit
access.
https://reviews.llvm.org/D40283
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/ll
davide added a comment.
In https://reviews.llvm.org/D40283#983057, @nelhage wrote:
> I did a little bit of looking into performance implications today. It looks
> like `DWARFASTParserClang::ParseTypeFromDWARF` is only called lazily as
> symbols are needed, which alleviates some of my concerns,
nelhage added a comment.
I did a little bit of looking into performance implications today. It looks
like `DWARFASTParserClang::ParseTypeFromDWARF` is only called lazily as symbols
are needed, which alleviates some of my concerns, and also makes it a bit
trickier to construct a convenient bench
I can try to take some time to benchmark, hopefully this weekend or so.
I do want to reiterate that this without this change debugging binaries
linked against libstdc++ is badly degraded, since recent libstdc++ uses
ABI tags on ~any methods that touch a std::string.
- Nelson
On Thu, Jan 11, 201
On Thu, Jan 11, 2018 at 2:01 AM, Pavel Labath via lldb-commits
wrote:
> On 10 January 2018 at 22:51, Greg Clayton wrote:
>> The right solution seems to be adding some sort of custom GNU ABI tag to the
>> DWARF. I know that won't help with existing binaries, but it sounds too
>> expensive to set
On 10 January 2018 at 22:51, Greg Clayton wrote:
> The right solution seems to be adding some sort of custom GNU ABI tag to the
> DWARF. I know that won't help with existing binaries, but it sounds too
> expensive to set the ASM name for everything.
>
What makes you think it will be expensive?
The right solution seems to be adding some sort of custom GNU ABI tag to the
DWARF. I know that won't help with existing binaries, but it sounds too
expensive to set the ASM name for everything.
> On Jan 10, 2018, at 2:23 PM, Nelson Elhage via Phabricator
> wrote:
>
> nelhage added a comment
nelhage added a comment.
Further evidence that the ABI tag is never stored separately:
$ strings -a -n4 test_abi_tag | grep cxx11
helloWorld_cxx11
_ZN1A16helloWorld_cxx11B5cxx11Ev
_ZN1A16helloWorld_cxx11B5cxx11Ev
Those three instances are, I presume, the `DW_AT_name`, and the
`DW_AT_lin
nelhage added a comment.
Compiling my test program with g++ and looking at `llvm-dwarfdump -all`, I can
see no reference to the ABI tag other than in the `DW_AT_linkage_name`.
Skimming the gdb source, I see references to it knowing how to *demangle* ABI
tags, but no references in the DWARF code
aprantl added a comment.
In https://reviews.llvm.org/D40283#972622, @clayborg wrote:
> Added Adrian Prantl as a reviewer in case he has any input. Adrian: is there
> any way that a DIE is marked up with an extra attribute when the asm name is
> explicitly set? It would be great to know this fro
clayborg added a comment.
And I do agree with Jim that we don't want to have to mangle the typename to
see if it matches, that is too much work.
https://reviews.llvm.org/D40283
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.
clayborg added a comment.
Added Adrian Prantl as a reviewer in case he has any input. Adrian: is there
any way that a DIE is marked up with an extra attribute when the asm name is
explicitly set? It would be great to know this from the DWARF so we don't have
to end up setting the ASM name for e
clayborg added a comment.
Sounds like finding a clang expert to clarify what Jim last asked for is the
way forward. Do a source control "blame" command and see who worked on the code
in the area of clang and maybe add them as reviewers so they can comment? I
agree with Jim that this sounds like
> On Jan 9, 2018, at 9:31 PM, Nelson Elhage via Phabricator
> wrote:
>
> nelhage added a comment.
>
> Hey -- Is there anything I can do to move this patch forward? Would it help
> to do something like only setting the attribute if the mangled name that
> *would* be generated doesn't match t
nelhage added a comment.
Hey -- Is there anything I can do to move this patch forward? Would it help to
do something like only setting the attribute if the mangled name that *would*
be generated doesn't match the one from the DWARF? Any such symbol is already
broken for lldb, so such a patch fe
jingham added a comment.
This patch is actually pretty broad, since it will make all C++ declarations
coming from DWARF have overridden names so far as clang is concerned. So we
are relying on the fact that as far as clang is concerned a method with an asm
assigned name is equivalent to method
clayborg added a comment.
ok, sounds like the abi_tag stuff is meant to just affect the mangled named
with no code gen implications. Jim should ok this, but I am ok if he is.
https://reviews.llvm.org/D40283
___
lldb-commits mailing list
lldb-commit
nelhage added a comment.
In https://reviews.llvm.org/D40283#936088, @clayborg wrote:
> Jim will need to ok this. A few concerns follow. Most of the time when we
> don't get the DWARF -> AST conversion right, it can mean that we might code
> gen incorrect code. Is there not enough information fo
clayborg added a comment.
Jim will need to ok this. A few concerns follow. Most of the time when we don't
get the DWARF -> AST conversion right, it can mean that we might code gen
incorrect code. Is there not enough information for the GNU abi_tag in the
DWARF to actually re-create these classe
nelhage updated this revision to Diff 124019.
nelhage added a comment.
Remove accidentally-added flycheck file.
https://reviews.llvm.org/D40283
Files:
include/lldb/Symbol/ClangASTContext.h
packages/Python/lldbsuite/test/expression_command/issue_35310/
packages/Python/lldbsuite/test/expres
nelhage updated this revision to Diff 124018.
nelhage added a comment.
Add a test case.
Verified that it passes on my machine and fails with this patch
reverted.
https://reviews.llvm.org/D40283
Files:
include/lldb/Symbol/ClangASTContext.h
packages/Python/lldbsuite/test/expression_command/i
labath added reviewers: clayborg, jingham.
labath added a comment.
This seems like a good idea to me (but Greg or Jim should review this).
Could you please also add a test case for this. It looks like you already have
a simple program in the bug you linked, so it should be just a matter of
turn
nelhage created this revision.
Herald added subscribers: JDevlieghere, aprantl.
When importing C++ methods into clang AST nodes from the DWARF symbol table,
preserve the `DW_AT_linkage_name` and use it as the linker ("asm") name for the
symbol.
Concretely, this enables `expression` to call into
32 matches
Mail list logo
