[Lldb-commits] [PATCH] D75761: Fix to get the AST we generate for function templates to be closer to what clang generates and expects

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

In D75761#1912038 <https://reviews.llvm.org/D75761#1912038>, @labath wrote:

> It's a pity that the clang's DW_AT_name value is so ambiguous. For example, gcc would output the name in the commit message as `operator< <A::B>`, which is a lot more parsable (for computers and people). Have you looked at changing clang's output to make it more like gcc's ? I know it would only help new compilers, but maybe for such a special case, this does not matter?
>
> Or, even if we do end up adding some compat parsing code, that would reduce the need for it to be super exact. I'm pretty sure that the current code does not handle all cases correctly. E.g., I believe it will break on something like `operator<<<&operator>> >` (mangled name `_ZlsIXadL_Zrs1AS0_EEEvS0_S0_`, godbolt link <https://godbolt.org/z/-c68Dz>).
>
> As an alternative, we could try extracting the same information from the mangled name (via llvm::ItaniumPartialDemangler::getFunctionBaseName), at least when DW_AT_linkage_name is present (not all compilers emit that attribute but I am not sure if anyone has tested if lldb actually works without it).


I was planning on looking into removing the template parameters from the names altogether for lldb but we would still need to do this on the lldb side to support older compilers. I have to try this and see how much it breaks.

I am going to move to use `llvm::ItaniumPartialDemangler::getFunctionBaseName)` and I am going to see if I can test subroutines w/o `DW_AT_linkage_name` and see if it works at all. If it does not work then maybe we don't need to support the case where we don't have it.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:829
+  // have something like operator>>. We check for the operator<=> case.
+  if (!Name.endswith(">") || Name.count("<") == 0 || Name.endswith("<=>"))
+    return {};
----------------
aprantl wrote:
> Should we scan for the complete "operator<" "operator<=>" instead, to be clearer and more future-proof?
> Should we assert(Name.count("<")), too?
I don't believe looking for the full name buys us anything correctness wise, we would have to be getting malformed names AFAIK but perhaps I am missing some odd case.

I don't think the assert makes sense b/c we may end up sharing this code w/ dsymutil where we don't know before hand if we have template params in the name. Also perhaps in the future we may change clang to not generate DW_AT_names like this but still support older compilers that do.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:839
+  size_t RightAngleCount = Name.count('>');
+  size_t LeftAngleCount = Name.count('<');
+
----------------
labath wrote:
> aprantl wrote:
> > We are scanning the entire string multiple times here and that seems unnecessarily expensive since C++ template function names get looong. I think we could do this as a single for-loop with a state machine instead, without being too difficult to read?
> Would it be sufficient to always strip one `<` from the first sequence of `<`'s that you find? You can't have two template `<` one after the other, so in a valid name, the last `<` will be the template angle bracket, while everything else must be a part of the operator name.
> 
> So, something like `return {Name.data(), Name.find_first_not_of('<', Name.find_first_of('<')) -1}` (with some error checks and an explicit check for the spaceship operator).
You correct, I was moving toward a state machine before you pointed out `ItaniumPartialDemangler`


================
Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:1
 template<typename T>
 int foo(T t1) {
----------------
labath wrote:
> Do we have a test for the spaceship operator?
We currently don't support C++2a but when we do we should add a test.


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

https://reviews.llvm.org/D75761