Expression evaluation for overloaded C functions

[EwanCrawford]

Summary

Fixes bugzilla ticket https://llvm.org/bugs/show_bug.cgi?id=26694
Where we can currently pick the incorrect declaration when calling a C function with the overloadable attribute.

This is done by checking if we're using language C, but the function has a mangled name.
Patch also includes some additional fallback parameter manglings for when clang doesn't emit a matching symbol.

[EwanCrawford]

friendly ping

[clayborg]

I will let Sean Callanan review this one as the expression parser is his area.

[spyffe]

There are a few changes I'd recommend to this patch. The biggest one is to move mangling knowledge from IRExecutionUnit to LanguageCPlusPlus, where it logically fits better.
The testsuite change should be applied across the board in my opinion, but I'm going to add Greg Clayton as a reviewer to cover that part.

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

10

Should we do this for all tests? Greg?

source/Expression/IRExecutionUnit.cpp

736

I'm pretty sure this should be on LanguageCPlusPlus, along with the code in CollectCandidateCPlusPlusNames that tries _ZN -> _ZNK and _Z -> _ZL.

LanguageCPlusPlus should have a method that takes a name and collects "equivalent" mangled names. That way all this mangling knowledge is kept in one place.

765

This is going to add SearchSpecs regardless of whether SubstituteMangledParameters did anything, slowing down symbol lookups for cases where it isn't necessary. This should only add specs if there was actually a difference.

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

2082

Two things:

• Why does this have to happen down here? Couldn't it be done up at the declaration of extern_c so that the bool stays const?
• Looking at this logic, it might also be cool to also set extern_c if the compile unit is C++ but the function in the DWARF has a non-mangled name... but that's solving a separate problem

[spyffe]

Added Greg to look at the testsuite changes.

[EwanCrawford]

Thanks for taking a look Sean.

I moved the mangling logic into LanguageCPlusPlus and created a new method for it.
There was an existing function FindEquivalentNames that used CPPRuntimeEquivalents, but looked like that was targeted at types. Let me know if you prefer I reuse that.

[clayborg]

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

8

Maybe we can have a standard clean rule in the main makefiles:

standard_clean:

rm -rf $(wildcard *.o *.d *.dSYM)

And maybe we can call this before calling the "clean" where each test case can override this. Not required for this patch though.

[labath]

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

8

The main Makefile.rules does the cleanup already, I'm pretty sure this rule is not necessary.

[EwanCrawford]

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

8

Good to know, I'll remove this case

[EwanCrawford]

Rebased on tip

[EwanCrawford]

Abandoning this patch since it's out of date, and @ldrumm has an alternative fix he's about to post

[ldrumm]

New candidate is here

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

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules
+
+clean::
+  rm -rf $(wildcard *.o *.d *.dSYM)

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 *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestExprCallOverloadedCFunction(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        TestBase.setUp(self)
+        # Find the line number to break for main.c.
+        self.line = line_number('main.c',
+                                '// break here')
+
+    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24489: Name lookup not working correctly on Windows")
+    @expectedFailureAll(compiler='gcc') # attribute overloadable only supported by clang
+    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/Expression/IRExecutionUnit.cpp

for (const SearchSpec &C_spec : C_specs)
         const ConstString &name = C_spec.name;
 
         if (CPlusPlusLanguage::IsCPPMangledName(name.GetCString()))
         {
             Mangled mangled(name, true);
             ConstString demangled = mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus);
 
             if (demangled)
             {
                 ConstString best_alternate_mangled_name = FindBestAlternateMangledName(demangled, lldb::eLanguageTypeC_plus_plus, sc);
 
                 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::vector<ConstString> alternates;
+            CPlusPlusLanguage::FindAlternateFunctionMangling(name, alternates);
+            CPP_specs.insert(CPP_specs.end(), alternates.begin(), alternates.end());
         }
 
     }
 }
 
 lldb::addr_t
 IRExecutionUnit::FindInSymbols(const std::vector<IRExecutionUnit::SearchSpec> &specs, const lldb_private::SymbolContext &sc)
 {
     Target *target = sc.target_sp.get();
 
     if (!target)
     {
         // we shouldn't be doing any symbol lookup at all without a target
         return LLDB_INVALID_ADDRESS;
     }
 
     for (const SearchSpec &spec : specs)
     {
         SymbolContextList sc_list;
 
         lldb::addr_t best_internal_load_address = LLDB_INVALID_ADDRESS;
 
         std::function<bool (lldb::addr_t &, SymbolContextList &, const lldb_private::SymbolContext &)> get_external_load_address =
             [&best_internal_load_address, target](lldb::addr_t &load_address,

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

ClangExpressionDeclMap::AddOneFunction (NameSearchContext &context,
 
     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) ||
+        const bool extern_c = (Language::LanguageIsC(comp_unit_language) &&
+                              !CPlusPlusLanguage::IsCPPMangledName(function->GetMangled().GetMangledName().AsCString())) ||
                               (Language::LanguageIsObjC(comp_unit_language) &&
-                               !Language::LanguageIsCPlusPlus(comp_unit_language));
         
         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();
                 clang::FunctionDecl *src_function_decl = llvm::dyn_cast_or_null<clang::FunctionDecl>(src_decl_context);
if (function)
                     }
                 }
             }
         }
 
         if (!function_type)
         {
             if (log)
                 log->PutCString("  Skipped a function because it has no type");
             return;
         }
 
         function_clang_type = function_type->GetFullCompilerType ();
 
         if (!function_clang_type)
         {
             if (log)

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h

public:
     // in some cases, compilers will output different names for one same type. when that happens, it might be impossible
     // to construct SBType objects for a valid type, because the name that is available is not the same as the name that
     // 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
+    FindAlternateFunctionMangling(ConstString mangled_name, std::vector<ConstString>& alternates);
+
     //------------------------------------------------------------------
     // PluginInterface protocol
     //------------------------------------------------------------------
     ConstString
     GetPluginName() override;
     
     uint32_t
     GetPluginVersion() override;
 };
     
 } // namespace lldb_private
 
 #endif // liblldb_CPlusPlusLanguage_h_

source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

bool might_have_partials=
             && strchr(type_name.AsCString(), '>') != nullptr); // angle brackets in the type_name before trying to scan for partial matches
     
     if ( might_have_partials )
         count = GetEquivalentsMap().FindPartialMatches(type_name, equivalents);
     
     return count;
 }
 
+// Given a mangled function, replaces all the function parameters of mangled type
+// 'search', with parameters of type 'replace'.
+static ConstString
+SubstituteMangledParameters(const llvm::StringRef& mangled, const llvm::StringRef& search, const llvm::StringRef& replace)
+{
+    // Find length of function name, already encoded in mangled symbol
+    static const llvm::StringRef numeric_digits("0123456789");
+    const size_t encoded_length_start = mangled.find_first_of(numeric_digits);
+    if (encoded_length_start == llvm::StringRef::npos)
+        return ConstString();
+
+    const size_t encoded_length_end = mangled.find_first_not_of(numeric_digits, encoded_length_start);
+    if (encoded_length_end == llvm::StringRef::npos || encoded_length_end <= encoded_length_start)
+       return ConstString();
+
+    // Parse encoded length into name_length
+    int name_length;
+    const llvm::StringRef encoded_length_str = mangled.substr(encoded_length_start, encoded_length_end - encoded_length_start);
+    if (encoded_length_str.getAsInteger(10, name_length) || name_length == 0)
+       return ConstString();
+
+    // Position in 'mangled' string of first function parameter
+    size_t param_pos = encoded_length_end + name_length;
+
+    // Iterate over all matching parameters, replacing them with string 'replace'
+    std::string modified_str = mangled.str();
+    while ((param_pos = modified_str.find(search, param_pos)) != std::string::npos)
+    {
+        modified_str.replace(param_pos, search.size(), replace);
+        param_pos += replace.size();
+    }
+
+    return ConstString(modified_str);
+}
+
+uint32_t
+CPlusPlusLanguage::FindAlternateFunctionMangling(ConstString mangled_name, std::vector<ConstString>& alternates)
+{
+    const uint32_t start_size = alternates.size();
+
+    // Maybe we're looking for a const symbol but the debug info told us it was 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.push_back(ConstString(fixed_scratch.c_str()));
+    }
+
+    // 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.push_back(ConstString(fixed_scratch.c_str()));
+    }
+
+    // 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'.
+    ConstString char_fixup = SubstituteMangledParameters(mangled_name.GetStringRef(), llvm::StringRef("a"), llvm::StringRef("c"));
+    if (!char_fixup.IsEmpty())
+        alternates.push_back(char_fixup);
+
+    // long long parameter mangling 'x', may actually just be a long 'l' argument
+    ConstString long_fixup = SubstituteMangledParameters(mangled_name.GetStringRef(), llvm::StringRef("x"), llvm::StringRef("l"));
+    if (!long_fixup.IsEmpty())
+        alternates.push_back(long_fixup);
+
+    // unsigned long long parameter mangling 'y', may actually just be unsigned long 'm' argument
+    ConstString ulong_fixup = SubstituteMangledParameters(mangled_name.AsCString(), llvm::StringRef("y"), llvm::StringRef("m"));
+    if (!ulong_fixup.IsEmpty())
+        alternates.push_back(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)