Expression evaluation for overloaded C functions (redux)

[ldrumm]

Summary

This is a redux of Ewan's patch , refactored to properly substitute primitive types using a hook in the itanium demangler, and updated after the previous patch went stale

The new SubsPrimitiveParmItanium function takes a symbol name and replacement primitive type parameter as before but parses it using the FastDemangler, which has been modified to be able to notify clients of parse events (primitive types at this point).

Additionally, we now use a set of ConstStrings instead of a vector so that we try and resolve the same invalid candidate multiple times.

I'd particularly welcome feedback on whether there is a better way of doing the hook from the demangler. In the present implementation, the time tradeoff is a single nullptr check in the hot path, which was the least invasive change of a few I prototyped.

[clayborg]

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

449

Don't you mean search instead of mangled above?

450–452

Use llvm::StringRef instead of "const char *" and then use the llvm::StringRef functions instead of libc functions for string searching.

456–458

Remove since using llvm::StringRef will have lengths already.

460

remove llvm::StringRef temp as no longer needed since "mangled" will be a llvm::StringRef already.

464

Why not use a std::string?

465–466

Remove both lines above if you use a std::string. Also, why zero it first and then copy the string over it?

471

Does this only get called with parameters? Or does it get called for everything?

476

Use starts_with StringRef function.

You also need to verify that there is a word boundary at the end of this. If you are searching for "F", and replacing it with "E" and you get "Foo, Bar) const" in s then this will match you will change it to "Eoo, Bar) const" right? So you need to check s[search.size] and make sure it is a non identifier character. Unless this function only gets called with identifier boundaries. From my quick look at the FastDemangle changes, it seems that this will get called with the entire remaining part of the string.

481–486

If you use a std::string just call .erase() and .insert()

[alexander-shaposhnikov]

include/lldb/Core/FastDemangle.h

22

some thoughts:
(also i assume i might be mistaken / missing smth)
in ./source/Core/Mangled.cpp at least 3 demanglers are used:
FastDemangle, itaniumDemangle, abi::__cxa_demangle .
Adding primitive_type_hook to FastDemangle makes the interface
more complicated & inconsistent (although yes, it's not particularly consistent right now).

packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/main.c

18

does this patch work when the functions come from a shared library ?
(i mean if we have libFoo.so which contains the implementations of get_arg_type(float), get_arg_type(int), and then in lldb we want to call "p get_arg_type(0.12f)")

[ldrumm]

updated to use StringRef-based parser rather than char pointers

include/lldb/Core/FastDemangle.h

22

You're right there are a bunch of manglers in use, and this is suboptimal. There is recent traction in LLVM to switch to using LLDB's FastDemangler once it supports everything; in terms of making the interface messy - you're again spot-on. None of the 4-or-so manglers visible to lldb have a symmetric API to support things like demangle -> remangle, so this is the least intrusive way I could come up with that didn't involve adding a new demangler to the list. I did think of several other possible approaches, including using the llvm demangler (which has no real public API apart from llvm::itaniumDemangle where the internal datastructure (struct Db) is not exposed, and modifying the llvm demangler to fix a problem in LLDB is going to be a much harder sell.

packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/main.c

18

Yes. The only thing that matters to the current implementation is that we know the mangled symbol name.

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

449

I did. That was a silly mistake.

465–466

The reason I set it to zero first was that I didn't know how many replacements would be made or how many times swap_parms_hook would be called, so this case guarantees it's always NUL-terminated. I've switched to std::string as you suggested, which conveniently sidesteps the issue :)

471

I'm not sure I follow; s should always be a valid pointer if that's what you mean?

476

This function only gets called when the demangler is trying to resolve a builtin type, so it only get called in a context where it's encoding a type, not an identifier. I could be wrong, but as far as I can tell, from reading the spec for itanium demangling (https://mentorembedded.github.io/cxx-abi/abi.html#mangle.builtin-type), and the code for the FastDemangler, this should be safe in the case you mentioned.

[clayborg]

Looks good.

include/lldb/Core/FastDemangle.h

 //===-- FastDemangle.h ------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_FastDemangle_h_
 #define liblldb_FastDemangle_h_
 
+#include <cstddef>
+
+#include <functional>
+
 namespace lldb_private {
 
 char *FastDemangle(const char *mangled_name);
 
-char *FastDemangle(const char *mangled_name, long mangled_name_length);
+char *
+FastDemangle(const char *mangled_name, size_t mangled_name_length,
+             std::function<void(const char *s)> primitive_type_hook = nullptr);
 }
 
 #endif

packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/Makefile

+LEVEL = ../../make
+
+C_SOURCES := main.c
+
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/TestCallOverloadedCFunction.py

+"""
+Test calling user defined C functions using expression evaluation.
+This test checks that expression evaluation works correctly for
+functions defined with __attribute__((overloadable)).
+
+Ticket: https://llvm.org/bugs/show_bug.cgi?id=26694
+"""
+
+from __future__ import print_function
+
+import lldb
+
+from lldbsuite.test.decorators import (
+    skipIf,
+    expectedFailureAll
+)
+from lldbsuite.test.lldbtest import (
+    TestBase,
+    CURRENT_EXECUTABLE_SET,
+    RUN_SUCCEEDED,
+    line_number,
+    no_match,
+)
+from lldbsuite.test import lldbutil
+
+
+class TestExprCallOverloadedCFunction(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        super(TestExprCallOverloadedCFunction, self).setUp()
+        # Find the line number to break for main.c.
+        self.line = line_number('main.c', '// break here')
+
+    @skipIf(compiler=no_match('clang'))  # `overloadable` is only supported by clang"
+    @expectedFailureAll(
+        oslist=["windows"],
+        bugnumber="llvm.org/pr24489: Name lookup broken on Windows")
+    def test(self):
+        """Test calling overloaded C functions."""
+        self.build()
+
+        # Set breakpoint in main and run exe
+        self.runCmd("file a.out", CURRENT_EXECUTABLE_SET)
+        lldbutil.run_break_set_by_file_and_line(
+            self,
+            "main.c",
+            self.line,
+            num_expected_locations=-1,
+            loc_exact=True
+        )
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # Test floating point type overload.
+        self.expect("expr get_arg_type(0.5f)", substrs=["$0 = 'F'"])
+
+        # Test integer type overload
+        self.expect("expr get_arg_type(8)", substrs=["$1 = 'I'"])

packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/main.c

+char __attribute__((overloadable)) get_arg_type(float x)
+{
+    return 'F'; // 'F' for float
+}
+
+char __attribute__((overloadable)) get_arg_type(int x)
+{
+    return 'I'; // 'I' for int
+}
+
+int main()
+{
+    char float_result = get_arg_type(0.1f);
+    char int_result = get_arg_type(2);
+
+    return 0; // break here
+}

source/Core/FastDemangle.cpp

 //===-- FastDemangle.cpp ----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 
+#include <functional>
+
+#include "lldb/Core/FastDemangle.h"
 #include "lldb/lldb-private.h"
 
 //#define DEBUG_FAILURES 1
 //#define DEBUG_SUBSTITUTIONS 1
 //#define DEBUG_TEMPLATE_ARGS 1
 //#define DEBUG_HIGHWATER 1
 //#define DEBUG_REORDERING 1
 
public:
   /// and managed by the demangler instance.
   ///
   /// @param storage_ptr Valid pointer to at least storage_size bytes of
   /// space that the SymbolDemangler can use during demangling
   ///
   /// @param storage_size Number of bytes of space available scratch memory
   /// referenced by storage_ptr
 
-  SymbolDemangler(void *storage_ptr, int storage_size) {
+  SymbolDemangler(void *storage_ptr, size_t storage_size,
+                  std::function<void(const char *)> builtins_hook = nullptr)
+      : m_builtins_hook(builtins_hook) {
     // Use up to 1/8th of the provided space for rewrite ranges
     m_rewrite_ranges_size = (storage_size >> 3) / sizeof(BufferRange);
     m_rewrite_ranges = (BufferRange *)storage_ptr;
     m_owns_m_rewrite_ranges = false;
 
     // Use the rest for the character buffer
     m_buffer =
         (char *)storage_ptr + m_rewrite_ranges_size * sizeof(BufferRange);
#endif
   //                ::= Dh   # IEEE 754r half-precision floating point (16 bits)
   //                ::= Di   # char32_t
   //                ::= Ds   # char16_t
   //                ::= Da   # auto (in dependent new-expressions)
   //                ::= Dn   # std::nullptr_t (i.e., decltype(nullptr))
   //                ::= u <source-name>    # vendor extended type
 
   const char *TryParseBuiltinType() {
+    if (m_builtins_hook)
+      m_builtins_hook(m_read_ptr);
+
     switch (*m_read_ptr++) {
     case 'v':
       return "void";
     case 'w':
       return "wchar_t";
     case 'b':
       return "bool";
     case 'c':
bool ParseTemplateParam() {
     if (!Parse('_'))
       return false;
 
     // When no number is present we get -1, which is convenient since
     // T_ is the zeroth element T0_ is element 1, and so on
     return RewriteTemplateArg(count + 1);
   }
 
+  // <vector-type>
+  // Dv <dimension number> _ <vector type>
+  bool TryParseVectorType() {
+    const int dimension = TryParseNumber();
+    if (dimension == -1)
+      return false;
+
+    if (*m_read_ptr++ != '_')
+      return false;
+
+    char vec_dimens[32] = {'\0'};
+    ::snprintf(vec_dimens, sizeof vec_dimens - 1, " __vector(%d)", dimension);
+    ParseType();
+    Write(vec_dimens);
+    return true;
+  }
+
   // <type> ::= <builtin-type>
   //        ::= <function-type>
   //        ::= <class-enum-type>
   //        ::= <array-type>
   //        ::= <pointer-to-member-type>
   //        ::= <template-param>
   //        ::= <template-template-param> <template-args>
   //        ::= <decltype>
#endif
     switch (*m_read_ptr) {
     case 'D':
       ++m_read_ptr;
       switch (*m_read_ptr++) {
       case 'p':
         if (!ParseType())
           return false;
         break;
+      case 'v':
+        if (!TryParseVectorType())
+          return false;
+        break;
       case 'T':
       case 't':
-      case 'v':
       default:
 #ifdef DEBUG_FAILURES
         printf("*** Unsupported type: %.3s\n", failed_type);
 #endif
         return false;
       }
       break;
     case 'T':
private:
 
   // Internal state used during demangling
 
   const char *m_read_ptr;
   const char *m_read_end;
   char *m_write_ptr;
   int m_next_template_arg_index;
   int m_next_substitute_index;
+  std::function<void(const char *s)> m_builtins_hook;
 };
 
 } // Anonymous namespace
 
 // Public entry points referenced from Mangled.cpp
 namespace lldb_private {
 char *FastDemangle(const char *mangled_name) {
   char buffer[16384];
   SymbolDemangler demangler(buffer, sizeof(buffer));
   return demangler.GetDemangledCopy(mangled_name);
 }
 
-char *FastDemangle(const char *mangled_name, long mangled_name_length) {
+char *FastDemangle(const char *mangled_name, size_t mangled_name_length,
+                   std::function<void(const char *s)> builtins_hook) {
   char buffer[16384];
-  SymbolDemangler demangler(buffer, sizeof(buffer));
+  SymbolDemangler demangler(buffer, sizeof(buffer), builtins_hook);
   return demangler.GetDemangledCopy(mangled_name, mangled_name_length);
 }
 } // lldb_private namespace

source/Expression/IRExecutionUnit.cpp

if (CPlusPlusLanguage::IsCPPMangledName(name.GetCString())) {
         if (best_alternate_mangled_name) {
           CPP_specs.push_back(best_alternate_mangled_name);
         }
 
         CPP_specs.push_back(SearchSpec(demangled, lldb::eFunctionNameTypeFull));
       }
     }
 
-    // Maybe we're looking for a const symbol but the debug info told us it was
-    // const...
-    if (!strncmp(name.GetCString(), "_ZN", 3) &&
-        strncmp(name.GetCString(), "_ZNK", 4)) {
-      std::string fixed_scratch("_ZNK");
-      fixed_scratch.append(name.GetCString() + 3);
-      CPP_specs.push_back(ConstString(fixed_scratch.c_str()));
-    }
-
-    // Maybe we're looking for a static symbol but we thought it was global...
-    if (!strncmp(name.GetCString(), "_Z", 2) &&
-        strncmp(name.GetCString(), "_ZL", 3)) {
-      std::string fixed_scratch("_ZL");
-      fixed_scratch.append(name.GetCString() + 2);
-      CPP_specs.push_back(ConstString(fixed_scratch.c_str()));
-    }
+    std::set<ConstString> alternates;
+    CPlusPlusLanguage::FindAlternateFunctionManglings(name, alternates);
+    CPP_specs.insert(CPP_specs.end(), alternates.begin(), alternates.end());
   }
 }
 
 void IRExecutionUnit::CollectFallbackNames(
     std::vector<SearchSpec> &fallback_specs,
     const std::vector<SearchSpec> &C_specs) {
   // As a last-ditch fallback, try the base name for C++ names.  It's terrible,
   // but the DWARF doesn't always encode "extern C" correctly.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

void ClangExpressionDeclMap::AddOneFunction(NameSearchContext &context,
   Address fun_address;
   CompilerType function_clang_type;
 
   bool is_indirect_function = false;
 
   if (function) {
     Type *function_type = function->GetType();
 
-    const lldb::LanguageType comp_unit_language =
-        function->GetCompileUnit()->GetLanguage();
-    const bool extern_c = Language::LanguageIsC(comp_unit_language) ||
-                          (Language::LanguageIsObjC(comp_unit_language) &&
-                           !Language::LanguageIsCPlusPlus(comp_unit_language));
+    const auto lang = function->GetCompileUnit()->GetLanguage();
+    const auto name = function->GetMangled().GetMangledName().AsCString();
+    const bool extern_c = (Language::LanguageIsC(lang) &&
+                           !CPlusPlusLanguage::IsCPPMangledName(name)) ||
+                          (Language::LanguageIsObjC(lang) &&
+                           !Language::LanguageIsCPlusPlus(lang));
 
     if (!extern_c) {
       TypeSystem *type_system = function->GetDeclContext().GetTypeSystem();
       if (ClangASTContext *src_ast =
               llvm::dyn_cast<ClangASTContext>(type_system)) {
         clang::DeclContext *src_decl_context =
             (clang::DeclContext *)function->GetDeclContext()
                 .GetOpaqueDeclContext();

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

 //===-- CPlusPlusLanguage.h -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_CPlusPlusLanguage_h_
 #define liblldb_CPlusPlusLanguage_h_
 
 // C Includes
 // C++ Includes
+#include <set>
 #include <vector>
 
 // Other libraries and framework includes
 #include "llvm/ADT/StringRef.h"
 
 // Project includes
 #include "lldb/Core/ConstString.h"
 #include "lldb/Target/Language.h"
public:
 
   ~CPlusPlusLanguage() override = default;
 
   lldb::LanguageType GetLanguageType() const override {
     return lldb::eLanguageTypeC_plus_plus;
   }
 
   std::unique_ptr<TypeScavenger> GetTypeScavenger() override;
-  
   lldb::TypeCategoryImplSP GetFormatters() override;
 
   HardcodedFormatters::HardcodedSummaryFinder GetHardcodedSummaries() override;
 
   HardcodedFormatters::HardcodedSyntheticFinder
   GetHardcodedSynthetics() override;
 
   //------------------------------------------------------------------
public:
   // can be used as a search key in FindTypes(). the equivalents map here is
   // meant to return possible alternative names
   // for a type through which a search can be conducted. Currently, this is only
   // enabled for C++ but can be extended
   // to ObjC or other languages if necessary
   static uint32_t FindEquivalentNames(ConstString type_name,
                                       std::vector<ConstString> &equivalents);
 
+  // Given a mangled function name, calculates some alternative manglings since
+  // the compiler mangling may not line up with the symbol we are expecting
+  static uint32_t
+  FindAlternateFunctionManglings(const ConstString mangled,
+                                 std::set<ConstString> &candidates);
+
   //------------------------------------------------------------------
   // PluginInterface protocol
   //------------------------------------------------------------------
   ConstString GetPluginName() override;
 
   uint32_t GetPluginVersion() override;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_CPlusPlusLanguage_h_

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

 //===-- CPlusPlusLanguage.cpp -----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CPlusPlusLanguage.h"
 
 // C Includes
-// C++ Includes
 #include <cctype>
 #include <cstring>
+
+// C++ Includes
 #include <functional>
+#include <memory>
 #include <mutex>
+#include <set>
 
 // Other libraries and framework includes
 #include "llvm/ADT/StringRef.h"
 
 // Project includes
 #include "lldb/Core/ConstString.h"
+#include "lldb/Core/FastDemangle.h"
+#include "lldb/Core/Log.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/RegularExpression.h"
 #include "lldb/Core/UniqueCStringMap.h"
 #include "lldb/DataFormatters/CXXFunctionPointer.h"
 #include "lldb/DataFormatters/DataVisualization.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/DataFormatters/VectorType.h"
 
bool might_have_partials =
                                                              // matches
 
   if (might_have_partials)
     count = GetEquivalentsMap().FindPartialMatches(type_name, equivalents);
 
   return count;
 }
 
+/// Given a mangled function `mangled`, replace all the primitive function type
+/// arguments of `search` with type `replace`.
+static ConstString SubsPrimitiveParmItanium(llvm::StringRef mangled,
+                                            llvm::StringRef search,
+                                            llvm::StringRef replace) {
+  Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_LANGUAGE);
+
+  const size_t max_len =
+      mangled.size() + mangled.count(search) * replace.size() + 1;
+
+  // Make a temporary buffer to fix up the mangled parameter types and copy the
+  // original there
+  std::string output_buf;
+  output_buf.reserve(max_len);
+  output_buf.insert(0, mangled.str());
+  ptrdiff_t replaced_offset = 0;
+
+  auto swap_parms_hook = [&](const char *parsee) {
+    if (!parsee || !*parsee)
+      return;
+
+    // Check whether we've found a substitutee
+    llvm::StringRef s(parsee);
+    if (s.startswith(search)) {
+      // account for the case where a replacement is of a different length to
+      // the original
+      replaced_offset += replace.size() - search.size();
+
+      ptrdiff_t replace_idx = (mangled.size() - s.size()) + replaced_offset;
+      output_buf.erase(replace_idx, search.size());
+      output_buf.insert(replace_idx, replace.str());
+    }
+  };
+
+  // FastDemangle will call our hook for each instance of a primitive type,
+  // allowing us to perform substitution
+  const char *const demangled =
+      FastDemangle(mangled.str().c_str(), mangled.size(), swap_parms_hook);
+
+  if (log)
+    log->Printf("substituted mangling for %s:{%s} %s:{%s}\n",
+                mangled.str().c_str(), demangled, output_buf.c_str(),
+                FastDemangle(output_buf.c_str()));
+
+  return output_buf == mangled ? ConstString() : ConstString(output_buf);
+}
+
+uint32_t CPlusPlusLanguage::FindAlternateFunctionManglings(
+    const ConstString mangled_name, std::set<ConstString> &alternates) {
+  const auto start_size = alternates.size();
+  /// Get a basic set of alternative manglings for the given symbol `name`, by
+  /// making a few basic possible substitutions on basic types, storage duration
+  /// and `const`ness for the given symbol. The output parameter `alternates`
+  /// is filled with a best-guess, non-exhaustive set of different manglings
+  /// for the given name.
+
+  // Maybe we're looking for a const symbol but the debug info told us it was
+  // non-const...
+  if (!strncmp(mangled_name.GetCString(), "_ZN", 3) &&
+      strncmp(mangled_name.GetCString(), "_ZNK", 4)) {
+    std::string fixed_scratch("_ZNK");
+    fixed_scratch.append(mangled_name.GetCString() + 3);
+    alternates.insert(ConstString(fixed_scratch));
+  }
+
+  // Maybe we're looking for a static symbol but we thought it was global...
+  if (!strncmp(mangled_name.GetCString(), "_Z", 2) &&
+      strncmp(mangled_name.GetCString(), "_ZL", 3)) {
+    std::string fixed_scratch("_ZL");
+    fixed_scratch.append(mangled_name.GetCString() + 2);
+    alternates.insert(ConstString(fixed_scratch));
+  }
+
+  // `char` is implementation defined as either `signed` or `unsigned`.  As a
+  // result a char parameter has 3 possible manglings: 'c'-char, 'a'-signed
+  // char, 'h'-unsigned char.  If we're looking for symbols with a signed char
+  // parameter, try finding matches which have the general case 'c'.
+  if (ConstString char_fixup =
+          SubsPrimitiveParmItanium(mangled_name.GetStringRef(), "a", "c"))
+    alternates.insert(char_fixup);
+
+  // long long parameter mangling 'x', may actually just be a long 'l' argument
+  if (ConstString long_fixup =
+          SubsPrimitiveParmItanium(mangled_name.GetStringRef(), "x", "l"))
+    alternates.insert(long_fixup);
+
+  // unsigned long long parameter mangling 'y', may actually just be unsigned
+  // long 'm' argument
+  if (ConstString ulong_fixup =
+          SubsPrimitiveParmItanium(mangled_name.GetStringRef(), "y", "m"))
+    alternates.insert(ulong_fixup);
+
+  return alternates.size() - start_size;
+}
+
 static void LoadLibCxxFormatters(lldb::TypeCategoryImplSP cpp_category_sp) {
   if (!cpp_category_sp)
     return;
 
   TypeSummaryImpl::Flags stl_summary_flags;
   stl_summary_flags.SetCascades(true)
       .SetSkipPointers(false)
       .SetSkipReferences(false)
virtual CompilerType AdjustForInclusion(CompilerType &candidate) override {
       if (!Language::LanguageIsC(lang_type) &&
           !Language::LanguageIsCPlusPlus(lang_type))
         return CompilerType();
       if (candidate.IsTypedefType())
         return candidate.GetTypedefedType();
       return candidate;
     }
   };
-  
   return std::unique_ptr<TypeScavenger>(new CPlusPlusTypeScavenger());
 }
 
 lldb::TypeCategoryImplSP CPlusPlusLanguage::GetFormatters() {
   static std::once_flag g_initialize;
   static TypeCategoryImplSP g_category;
 
   std::call_once(g_initialize, [this]() -> void {