[lldb][CPlusPlus] Implement CPlusPlusLanguage::GetFunctionDisplayName

[Michael137]

Summary

This patch implements the GetFunctionDisplayName API which gets
used by the frame-formatting code to decide how to print a
function name.

Currently this API trivially returns false, so we try to parse
the demangled function base-name by hand. We try find the closing
parenthesis by doing a forward scan through the demangled name. However,
for arguments that contain parenthesis (e.g., function pointers)
this would leave garbage in the frame function name.

By re-using the CPlusPlusLanguage parser for this we offload the
need to parse function names to a component that knows how to do this
already.

We leave the existing parsing code in FormatEntity since it's used
in cases where a language-plugin is not available (and is not
necessarily C++ specific).

Example

For following function:

int foo(std::function<int(void)> const& func) { return 1; }

Before patch:

frame #0: 0x000000010000151c a.out`foo(func= Function = bar() )> const&) at sample.cpp:11:49

After patch:

frame #0: 0x000000010000151c a.out`foo(func= Function = bar() ) at sample.cpp:11:49

Testing

• Added shell test

[Michael137]

• Add more test cases

[labath]

Wow, another name parser I knew nothing about. :/

I'm probably being naive, but I don't suppose there's an easy a way to reuse/repurpose the parser in the C++ language plugin for this (?) This is the first time I see this code, so it's hard to construct counter-examples, but I'd be surprised if this is correct.

lldb/source/Core/FormatEntity.cpp

1673

What if there are multiple function arguments? Won't this find the end of the last one?

[Michael137]

In D136761#3885529, @labath wrote:

Wow, another name parser I knew nothing about. :/

I'm probably being naive, but I don't suppose there's an easy a way to reuse/repurpose the parser in the C++ language plugin for this (?) This is the first time I see this code, so it's hard to construct counter-examples, but I'd be surprised if this is correct.

Hehe I agree this function could use some clean-up
I tried to avoid going down that rabbit hole at the moment but I'll double check :)
I wouldn't be shocked if there were edge-cases that don't work.

The algorithm iiuc works as follows:

1. Find the opening function parenthesis (open_paren) and print out the string up to that point (effectively the function name)
2. For each variable in scope print out: <var_name>=<var_representation>
3. Find closing parenthesis
4. Print everything from closing parenthesis onward (I imagine this is to preserve function qualifiers)

Where the reverse scan might go wrong is with noexecept(...) specifications. Will check...

An alternative is to actually implement the Language::GetFunctionDisplayName(Language::FunctionNameRepresentation::eNameWithArgs) API for CPlusPlusLanguage. Currently it's a no-op (which is why we end up in this hand-rolled parser)

[Michael137]

lldb/source/Core/FormatEntity.cpp

1673

Had a local test-case that worked iirc. A well-formed demangled function name always has a final closing parenthesis after all the function arguments. So unless I missed something it should find the last parenthesis correctly

I will add a test-case for this to make sure

[labath]

In D136761#3885557, @Michael137 wrote:

In D136761#3885529, @labath wrote:

Wow, another name parser I knew nothing about. :/

I'm probably being naive, but I don't suppose there's an easy a way to reuse/repurpose the parser in the C++ language plugin for this (?) This is the first time I see this code, so it's hard to construct counter-examples, but I'd be surprised if this is correct.

Hehe I agree this function could use some clean-up
I tried to avoid going down that rabbit hole at the moment but I'll double check :)
I wouldn't be shocked if there were edge-cases that don't work.

The algorithm iiuc works as follows:

1. Find the opening function parenthesis (open_paren) and print out the string up to that point (effectively the function name)
2. For each variable in scope print out: <var_name>=<var_representation>
3. Find closing parenthesis
4. Print everything from closing parenthesis onward (I imagine this is to preserve function qualifiers)

Ok, so it basically takes the function base name, and then manually prints the list of function arguments inside parenthesis. That sounds like it could be easy to do with the other parser. Keep the argument printing part, and replace the basename parsing with a call to the language parser ?

I wouldn't say its mandatory but, unlike removing the language parser, this could actually be achievable.

lldb/source/Core/FormatEntity.cpp

1673

Ok, I see. Given your description above, I believe this will work OK. However, I suspect it will go wrong in the part where it tries skip over the template arguments (and gets confused by things like operator<< and foo<(2 > 1)>).

[Michael137]

lldb/source/Core/FormatEntity.cpp

1673

Good catch with the operator<<. Indeed this solution wouldn't quite work. Added a test for this

[Michael137]

Keep the argument printing part, and replace the basename parsing with a call to the language parser ?

The two main caveats here are:

1. the name parser drops the return type when parsing a basename
2. this code-path is also used for cases where a language plugin isn't available (not sure how often/whether that happens in practice). So I don't think we can "replace"

I think we want to:

1. Add something like CPlusPlusNameParser::MethodName::GetReturnType
2. Implement CPlusPlusLanguage::GetFunctionDisplayName where we could re-use the argument parsing code from FormatEntity but get the basename from the language plugin

Shouldn't be too bad

[labath]

Yeah, something like that.

[Michael137]

• Use the C++ language-plugin to format the function name

[Michael137]

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

1538 ↗

(On Diff #471468)

Not great that this is copied from FormatEntity but didn't see a great way of making this a useful API across both components

[Michael137]

• Add more tests

[labath]

This isn't your fault, but this GetFunctionDisplayName doesn't seems particularly well suited, as it forces you to reimplement a lot of the printing functionality for each language plugin. I'd expect the customization point to be something like your helper PrettyPrintFunctionNameWithArgs function.

[Michael137]

lldb/test/Shell/Settings/TestFrameFormatNameWithArgs.test

15

The __1 breaks the linux build bots apparently. Will make this check a bit more flexible in follow-up commit

[Michael137]

Closed with commit 031096d04d09ac4a93859d161fb42d1a1b308253 (for some reason the Phab link wasn't in the commit message)

[stella.stamenova]

Looks like in addition to the Linux failure, this also broke the Windows LLDB bot: https://lab.llvm.org/buildbot/#/builders/83/builds/25424/steps/7/logs/stdio

[Michael137]

In D136761#3897029, @stella.stamenova wrote:

Looks like in addition to the Linux failure, this also broke the Windows LLDB bot: https://lab.llvm.org/buildbot/#/builders/83/builds/25424/steps/7/logs/stdio

Thanks for letting me know. Will be able to take a look only in an hour or so.

lldb/source/Core/FormatEntity.cpp

if (language_plugin_handled) {
               else
                 open_paren = nullptr;
             }
             if (open_paren) {
               if (IsToken(open_paren, "(anonymous namespace)")) {
                 open_paren =
                     strchr(open_paren + strlen("(anonymous namespace)"), '(');
                 if (open_paren)
-                  close_paren = strchr(open_paren, ')');
+                  close_paren = strrchr(open_paren, ')');
               } else
-                close_paren = strchr(open_paren, ')');
+                close_paren = strrchr(open_paren, ')');
             }
 
             if (open_paren)
               s.Write(cstr, open_paren - cstr + 1);
             else {
               s.PutCString(cstr);
               s.PutChar('(');
             }

lldb/test/Shell/Settings/Inputs/names.cpp

+#include <functional>
+
+struct Foo {
+  template <typename T> void foo(T const &t) const noexcept(true) {}
+};
+
+template <typename T> int foo(T const &t) { return 0; }
+
+int bar() { return 1; }
+
+namespace {
+int anon_bar() { return 1; }
+auto anon_lambda = [](std::function<int(int (*)(int))>) mutable {};
+} // namespace
+
+int main() {
+  foo(bar);
+  foo(std::function{bar});
+  foo(anon_lambda);
+  foo(std::function{anon_bar});
+  foo(&Foo::foo<std::function<int(int)>>);
+  Foo f;
+  f.foo(std::function{bar});
+  f.foo(std::function{anon_bar});
+  return 0;
+}

lldb/test/Shell/Settings/TestFrameFormatNameWithArgs.test

+# RUN: %clangxx_host -g -O0 %S/Inputs/names.cpp -std=c++17 -o %t.out
+# RUN: %lldb -b -s %s %t.out | FileCheck %s
+settings set -f frame-format "frame ${function.name-with-args}\n"
+break set -n foo
+run
+# CHECK: frame int foo<int ()>(t={{.*}})
+c
+# CHECK: frame int foo<std::__1::function<int ()>>(t= Function = bar() )
+c
+# CHECK: frame int foo<(anonymous namespace)::$_0>(t={{.*}})
+c
+# CHECK: frame int foo<std::__1::function<int ()>>(t= Function = (anonymous namespace)::anon_bar() )
+c
+# CHECK: frame int foo<void (Foo::*)(std::__1::function<int (int)> const&) const noexcept>(t={{.*}})
+c
+# CHECK: frame void Foo::foo<std::__1::function<int ()>>(this={{.*}}, t= Function = bar() ) const
+c
+# CHECK: frame void Foo::foo<std::__1::function<int ()>>(this={{.*}}, t= Function = (anonymous namespace)::anon_bar() ) const
+q