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

Tue Aug 16 06:58:45 PDT 2022

labath added a comment.

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

> As @labath mentioned, we do force clang to preserve the linkage name via `asm()`, but only for class member functions. This was added in `675767a5910d2ec77ef8b51c78fe312cf9022896` (https://reviews.llvm.org/D40283) to also support `abi_tag`!

Yes, that's the patch I was alluding to. I'm sorry I was too lazy to look it up.

> But that didn't cover templates functions:
>
>   Use the DWARF linkage name when importing C++ methods.
>   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 names that use the
>   GNU `abi_tag` extension
>
> I tried adding an `AsmLabelAttr` to the `FunctionDecl`s we create when parsing DWARF and it does fix the ABI-tag problem on my small test case.

Which test case are you referring to. The one you made inline in https://reviews.llvm.org/D131335#3714567. Or the test attached to this patch? If it's the former, what part of the big test case is missing (and can that be fixed)?

> But this only works because the way we create `FunctionTemplateDecl`s is incorrect (as I've described in my previous comments).
>
> So the options are any combination of the following:
>
> 1. carry this patch forward (and possibly remove the `asm()` hack for C++ member functions)
> 2. Add the `asm()` attribute hack to all function declarations (or just when we are dealing with template functions)
> 3. Fix the way we generate `FunctionTemplateDecl`s when parsing DWARF (this likely needs a change to DWARF generation)
>
> @aprantl @labath Any preference? To me it seems 1 and 3 are the more "proper" way to fix this issue. And once we fix 3 (which we should do anyway) #2 may break.

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).

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).

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.

The idea of encoding this information into dwarf (via `DW_TAG_template_type_parameter`) sounds interesting but it's not clear to me how one would extend it to more complicated scenarios. E.g., I'm not sure how I'd describe `template<typename T> void f(std::enable_if_t<std::is_whatever_v<T>, T>)` in this way, but the result would likely be huge. One could also consider "fishing" this information out of the mangled name (at least the itanium scheme should have all of this in there, and in a more compact scheme), but that does seem rather hacky.

> The good thing about #2 is that we avoid searching object files, improving performance.

Speaking of performance, how does this searching impact the evaluation time of expressions?

(*) The use of mangled names is not particularly important here. What we really need is just some way to associate the entities in the clang AST with the symbols in llvm IR. Mangled names, due to their almost-guaranteed uniqueness, happen to be very good for that. However, one can imagine an implementation where we tag each symbol with an `asm("__lldb_deadbeef")` label, where `0xdeadbeef` is the address of that particular symbol in a binary, and then we translate that symbol to an actual address in llvm IR. That would arguably be a hack, but only because it works around an lldb/clang limitation. Functionality-wise, I think it would be even more correct than using mangled names.

================
Comment at: lldb/test/API/lang/cpp/abi_tag_lookup/TestAbiTagLookup.py:14
+    @skipIfWindows
+    @expectedFailureAll(debug_info=["dwarf", "gmodules", "dwo"])
+    def test_abi_tag_lookup(self):
----------------
This means the test is only expected to pass with the `dsym` debug info flavour. Is that really what you wanted to say?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131335