[Lldb-commits] [PATCH] D131335: [lldb] abi_tag support 3/3 - Use mangle tree API to determine approximate mangled matches

Thu Aug 18 11:26:27 PDT 2022

labath added a comment.

In D131335#3726204 <https://reviews.llvm.org/D131335#3726204>, @Michael137 wrote:

> Thanks for taking the time to look at this
>
>> I would like to understand what (if any) are the issues with extending the approach #2 to cover non-member functions as well. For what it's worth, I don't think #2 is a hack. I might actually say that's "using DWARF as it was meant to be used" -- the mangled name is there for us to use (*). Before LLDB came around, I doubt anybody on the DWARF committee imagined people would try round-tripping the DWARF information back into a "regular" C++ compiler (and expecting it to recreate "perfect" mangled names for templates and all).
>
> There are no issues that I know of with this. My hesitation with this approach was just that if we eventually fix the way we construct template decls, we will need to make sure we continue attaching the labels correctly. Maybe this isn't a big deal if we note it in a comment

Yeah, if you end up working on this, and find that you need to have clang to something in this area, I can try to loop in some clang people to help.

>> I don't mean to offend, but if anything, I would say that #1 is a hack. What it essentially does is pretend that abi tags don't exist. The way I understand it, the main reason for the introduction of abi tags was to enable one to have two versions of the same function (class, etc.) co-exist in the same binary/program. This essentially defeats that. I haven't looked at the implementation too closely, but I don't see how one could properly disambiguate two differently-tagged versions of the same functions using this approach. It may be a hack that we have to live with, but I am not convinced of that (yet).
>
> The way this patch allows distinguishing differently-tagged versions is by checking the `Attrs` Node of the Itanium mangle tree. But you're right in that this change does rely on a follow-up patch that attaches `AbiTagAttr` nodes to the `FunctionDecl`s we generate, which we were planning on doing. But I guess if we are going to attach labels to FunctionDecls anyway, we could just attach the `AsmLabel` instead and thus cover all types of attributes trivially.

FWIW, I would be all for attaching the abi tags to the clang declarations if they would be in some easily accessible form (e.g. a DWARF attribute). Parsing them out of the mangled name is somewhat dubious, but I am not entirely against that, if it is necessary for some use case. However, even if we did that, I'd still say that attaching the asm attribute is a good idea.

>> Regarding #3, yes, there are definite problems regarding the way we represent templates. The root cause is the fundamental mismatch between what clang wants (a completely accurate description of all objects "as if written in source") versus what DWARF provides (a description of the most important properties of entities that are present in the binary). Where the problem lies depends on your point of view. However, even if we come up with a better way to represent the debug information in AST, it's not clear to me why that would be incompatible with #2. Since we don't have the actual source code for the template function, we will never be able to provide a fully generic definition of it -- we will always need to deal with specific instantiations of that template. So what we just need is the attach a (asm) label to a specific template instantiation. I would hope that wouldn't be too much to ask of clang to support.
>
> This mismatch currently causes problems with unqualified lookups, especially with import-std-module enabled, so it would be nice to fix eventually. We don't need a fully generic definition, just the generic prototype to get the mangled name correctly. But as you say, this can be done with an asm() label with less trouble.
>
> I can open a separate diff with approach #2 and see from there.

Cool. Thanks for doing that.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131335/new/

https://reviews.llvm.org/D131335