Page MenuHomePhabricator

[lldb] Remove template parameters from FunctionTemplateDecl names
ClosedPublic

Authored by shafik on Mar 6 2020, 11:05 AM.

Details

Summary

Fix to get the AST we generate for function templates closer to what clang generates and expects. We fix which FuntionDecl we are passing to CreateFunctionTemplateSpecializationInfo and we strip template parameters from the name when creating the FunctionDecl and FunctionTemplateDecl.

These two fixes together fix asserts and ambiguous lookup issues for several cases which are added to the already existing small function template test. This fixes issues with overloads, overloads and ADL, variadic function templates and templated operator overloads.

The recent fix D75545 came out of this patch and there will be at least one more patch to deal with and edge case with templated operator< when it is in a namespace. We do C++ name parsing in CPlusPlusNameParser and we don’t deal with cases like A::operator<<A::B> properly.

Currently the fix applies mostly to dsym cases and I will be working on a fix that will apply to all cases but it may be a separate PR.

Diff Detail

Event Timeline

shafik created this revision.Mar 6 2020, 11:05 AM

The general mechanics look good, I have a few comments about the implementation.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
823

Nit: llvm::Optional

However, usually just a plain StringRef is enough, since it can be empty().

825

Nit: This would be nicer as the doxygen comment of the function.

831

Should we scan for the complete "operator<" "operator<=>" instead, to be clearer and more future-proof?
Should we assert(Name.count("<")), too?

841

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?

852

This function would probably benefit from a unit test even if that means it can't be static any more. I just can't prove to myself that there isn't an off-by-one error lurking in it somewhere.

1172

Can you add a comment here explaining why the template parameters are being stripped? I don't think that's obvious to the casual reader.

lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py
63

Are all of these strings exercising the stripping function? Perhaps that's just as good as a unit test then? But the unit test would still be better to document the expectations of the function.

labath added a subscriber: labath.Mar 9 2020, 1:50 AM

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

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

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
841

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

lldb/test/API/lang/cpp/template-function/main.cpp
1

Do we have a test for the spaceship operator?

teemperor requested changes to this revision.Mar 9 2020, 3:33 PM

Small nit pick: Use expect_expr( over expect("expr ... if you do a Python test.

This revision now requires changes to proceed.Mar 9 2020, 3:33 PM
shafik updated this revision to Diff 249247.Mar 9 2020, 5:02 PM
shafik marked 11 inline comments as done.

Moving to using ItaniumPartialDemangler for now.

shafik added a comment.Mar 9 2020, 5:02 PM

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

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.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
831

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.

841

You correct, I was moving toward a state machine before you pointed out ItaniumPartialDemangler

lldb/test/API/lang/cpp/template-function/main.cpp
1

We currently don't support C++2a but when we do we should add a test.

(This looks fine to me.)

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.

This is a issue that has come up multiple times now. The presence of template parameters makes it very hard to use accelerator tables. I think it would be great if we could get rid of them.

lldb/test/API/lang/cpp/template-function/main.cpp
1

How is this lack of support "implemented"? Would it make sense to test that we do something reasonable (e.g. ignore it) if we do happen to run into it?

Given that the new version of this patch doesn't treat the spaceship operator specially, I don't think it needs to/should be a part of this patch, but it is something to think about...

shafik updated this revision to Diff 249430.Mar 10 2020, 9:54 AM

Move to using expect_expr in the test.

teemperor retitled this revision from Fix to get the AST we generate for function templates to be closer to what clang generates and expects to [lldb] Fix to get the AST we generate for function templates to be closer to what clang generates and expects.Mar 11 2020, 1:40 AM
teemperor set the repository for this revision to rLLDB LLDB.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2020, 1:40 AM
teemperor accepted this revision.Mar 11 2020, 4:53 AM

I only have some minor comments but otherwise I think this can land. The title could use some updating as "Fix to get the AST we generate for function templates to be closer to what clang generates and expects" seems very abstract. What about "[lldb] Remove template parameters from FunctionTemplateDecl names" or something like that?

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1182

You can just pass a nullptr instead of &Size if you don't use the value.

lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py
16–18

This line can just be lldbutil.run_to_source_breakpoint(self, '// break here', lldb.SBFileSpec("main.cpp")), no need to assign self.anything. The SBFileSpec constructor is curious as it seems the constructor without the bool is deprecated since 2010 (?) but we're using it in many other places (as the bool parameter doesn't appear to be documented.....)

lldb/test/API/lang/cpp/template-function/main.cpp
1

Well the expression evaluator doesn't activate C++20 so we would just not parse the <=> as the space ship operator in an expression (and therefore not call it). We could still get it into our AST in which case we would probably handle it as if C++20 was activated (at least on the Clang side, not sure about the other parts of LLDB).

I don't think this review is blocked by this though. We first need to add proper C++20 support and then we can actually test this. But I'm also not opposed to having a test that tries calling <=> and just makes sure that Clang handles it and then errors out without crashing anything.

15

I wish this function had a more descriptive name. Maybe something like FuncWithOverload and g would be FuncWithoutOverload. Or a comment that the f here is intended to have the same name as A::f or something like that. Same for the other single-letter function names in here (as they all seem to serve a special purpose and are not just generic functions).

42

This isn't related to A::C from what I understand, so could this have another name than C?

73

Can you move the comments what those expressions are testing to TestTemplateFunctions.py? If the expression fails it would be good to have the respective comment next to the expression instead of the copy in the .cpp file.

This revision is now accepted and ready to land.Mar 11 2020, 4:53 AM

FWIW, I don't think the error cases should use expect_expr as it currently doesn't have a reasonable way to handle errors (the error_msg thing is really too strict and didn't land on purpose. That was more of a side effect of all the refactoring that went into the expect_expr patch).

shafik updated this revision to Diff 250327.Mar 13 2020, 5:04 PM
shafik marked 8 inline comments as done.

Addressing comments

teemperor accepted this revision.Mar 16 2020, 2:00 AM

LGTM

lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py
37

ultra nit pick: technically there should be a space after the "," in the expression.

lldb/test/API/lang/cpp/template-function/main.cpp
6

hae -> have

(Also my nit pick about the revision title is still stands)

shafik retitled this revision from [lldb] Fix to get the AST we generate for function templates to be closer to what clang generates and expects to [lldb] Remove template parameters from FunctionTemplateDecl names.Mar 16 2020, 3:50 PM
shafik updated this revision to Diff 250644.Mar 16 2020, 3:54 PM
shafik marked 2 inline comments as done.

Addressing minor comments

This revision was automatically updated to reflect the committed changes.