[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 ↗

(On Diff #470791)

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 ↗

(On Diff #470791)

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 ↗

(On Diff #470791)

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 ↗

(On Diff #470791)

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

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/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

public:
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
   
   bool DemangledNameContainsPath(llvm::StringRef path, 
                                  ConstString demangled) const override;
 
   ConstString
   GetDemangledFunctionNameWithoutArguments(Mangled mangled) const override;
 
+  bool GetFunctionDisplayName(const SymbolContext *sc,
+                              const ExecutionContext *exe_ctx,
+                              FunctionNameRepresentation representation,
+                              Stream &s) override;
+
   static bool IsCPPMangledName(llvm::StringRef name);
 
   // Extract C++ context and identifier from a string using heuristic matching
   // (as opposed to
   // CPlusPlusLanguage::MethodName which has to have a fully qualified C++ name
   // with parens and arguments.
   // If the name is a lone C identifier (e.g. C) or a qualified C identifier
   // (e.g. A::B::C) it will return true,

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

 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Demangle/ItaniumDemangle.h"
 
 #include "lldb/Core/Mangled.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/UniqueCStringMap.h"
+#include "lldb/Core/ValueObjectVariable.h"
 #include "lldb/DataFormatters/CXXFunctionPointer.h"
 #include "lldb/DataFormatters/DataVisualization.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/DataFormatters/VectorType.h"
 #include "lldb/Symbol/SymbolFile.h"
+#include "lldb/Symbol/VariableList.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegularExpression.h"
 
 #include "BlockPointer.h"
 #include "CPlusPlusNameParser.h"
 #include "Coroutines.h"
if (!std::isalnum(basename[idx]) && basename[idx] != '_')
       break;
     ++idx;
   }
 
   // We processed all characters. It is a vaild basename.
   return idx == basename.size();
 }
 
+/// Writes out the function name in 'full_name' to 'out_stream'
+/// but replaces each argument type with the variable name
+/// and the corresponding pretty-printed value
+static bool PrettyPrintFunctionNameWithArgs(Stream &out_stream,
+                                            char const *full_name,
+                                            ExecutionContextScope *exe_scope,
+                                            VariableList const &args) {
+  CPlusPlusLanguage::MethodName cpp_method{ConstString(full_name)};
+
+  if (!cpp_method.IsValid())
+    return false;
+
+  llvm::StringRef return_type = cpp_method.GetReturnType();
+  if (!return_type.empty()) {
+    out_stream.PutCString(return_type);
+    out_stream.PutChar(' ');
+  }
+  out_stream.PutCString(cpp_method.GetScopeQualifiedName());
+  out_stream.PutChar('(');
+
+  FormatEntity::PrettyPrintFunctionArguments(out_stream, args, exe_scope);
+
+  out_stream.PutChar(')');
+
+  llvm::StringRef qualifiers = cpp_method.GetQualifiers();
+  if (!qualifiers.empty()) {
+    out_stream.PutChar(' ');
+    out_stream.PutCString(qualifiers);
+  }
+
+  return true;
+}
+
 bool CPlusPlusLanguage::MethodName::TrySimplifiedParse() {
   // This method tries to parse simple method definitions which are presumably
   // most comman in user programs. Definitions that can be parsed by this
   // function don't have return types and templates in the name.
   // A::B::C::fun(std::vector<T> &) const
   size_t arg_start, arg_end;
   llvm::StringRef full(m_full.GetCString());
   llvm::StringRef parens("()", 2);
for (auto suffix : suffixes) {
     if (file_path.endswith_insensitive(suffix))
       return true;
   }
 
   // Check if we're in a STL path (where the files usually have no extension
   // that we could check for.
   return file_path.contains("/usr/include/c++/");
 }
+
+bool CPlusPlusLanguage::GetFunctionDisplayName(
+    const SymbolContext *sc, const ExecutionContext *exe_ctx,
+    FunctionNameRepresentation representation, Stream &s) {
+  switch (representation) {
+  case FunctionNameRepresentation::eNameWithArgs: {
+    // Print the function name with arguments in it
+    if (sc->function) {
+      ExecutionContextScope *exe_scope =
+          exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr;
+      const char *cstr = sc->function->GetName().AsCString(nullptr);
+      if (cstr) {
+        const InlineFunctionInfo *inline_info = nullptr;
+        VariableListSP variable_list_sp;
+        bool get_function_vars = true;
+        if (sc->block) {
+          Block *inline_block = sc->block->GetContainingInlinedBlock();
+
+          if (inline_block) {
+            get_function_vars = false;
+            inline_info = sc->block->GetInlinedFunctionInfo();
+            if (inline_info)
+              variable_list_sp = inline_block->GetBlockVariableList(true);
+          }
+        }
+
+        if (get_function_vars) {
+          variable_list_sp =
+              sc->function->GetBlock(true).GetBlockVariableList(true);
+        }
+
+        if (inline_info) {
+          s.PutCString(cstr);
+          s.PutCString(" [inlined] ");
+          cstr = inline_info->GetName().GetCString();
+        }
+
+        VariableList args;
+        if (variable_list_sp)
+          variable_list_sp->AppendVariablesWithScope(eValueTypeVariableArgument,
+                                                     args);
+        if (args.GetSize() > 0) {
+          if (!PrettyPrintFunctionNameWithArgs(s, cstr, exe_scope, args))
+            return false;
+        } else {
+          s.PutCString(cstr);
+        }
+        return true;
+      }
+    } else if (sc->symbol) {
+      const char *cstr = sc->symbol->GetName().AsCString(nullptr);
+      if (cstr) {
+        s.PutCString(cstr);
+        return true;
+      }
+    }
+  } break;
+  default:
+    return false;
+  }
+
+  return false;
+}

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

+#include <functional>
+
+struct Foo {
+  template <typename T> void foo(T const &t) const noexcept(true) {}
+
+  template <size_t T> void operator<<(size_t) {}
+};
+
+namespace ns {
+template <typename T> int foo(T const &t) noexcept(false) { return 0; }
+} // namespace ns
+
+int bar() { return 1; }
+
+namespace {
+int anon_bar() { return 1; }
+auto anon_lambda = [](std::function<int(int (*)(int))>) mutable {};
+} // namespace
+
+int main() {
+  ns::foo(bar);
+  ns::foo(std::function{bar});
+  ns::foo(anon_lambda);
+  ns::foo(std::function{anon_bar});
+  ns::foo(&Foo::foo<std::function<int(int)>>);
+  Foo f;
+  f.foo(std::function{bar});
+  f.foo(std::function{anon_bar});
+  f.operator<< <(2 > 1)>(0);
+  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
+break set -n operator<<
+run
+# CHECK: frame int ns::foo<int ()>(t={{.*}})
+c
+# CHECK: frame int ns::foo<std::__1::function<int ()>>(t= Function = bar() )
+c
+# CHECK: frame int ns::foo<(anonymous namespace)::$_0>(t={{.*}})
+c
+# CHECK: frame int ns::foo<std::__1::function<int ()>>(t= Function = (anonymous namespace)::anon_bar() )
+c
+# CHECK: frame int ns::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
+c
+# CHECK: frame void Foo::operator<<<1ul>(this={{.*}}, (null)=0)
+q