[Lldb-commits] [PATCH] D17957: Expression evaluation for overloaded C functions

[Sean Callanan via lldb-commits]

spyffe requested changes to this revision.
spyffe added a comment.
This revision now requires changes to proceed.

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.


================
Comment at: packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/Makefile:9
@@ +8,3 @@
+ifneq (,$(findstring clang,$(CC)))
+  CFLAGS_EXTRAS += -fno-limit-debug-info
+endif
----------------
Should we do this for all tests?  **Greg**?

================
Comment at: source/Expression/IRExecutionUnit.cpp:736
@@ +735,3 @@
+    return ConstString(modified_str);
+}
+
----------------
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.

================
Comment at: source/Expression/IRExecutionUnit.cpp:815
@@ -766,1 +814,3 @@
+        ConstString ulong_fixup = SubstituteMangledParameters(name.AsCString(), llvm::StringRef("y"), llvm::StringRef("m"));
+        CPP_specs.push_back(SearchSpec(ulong_fixup, lldb::eFunctionNameTypeFull));
     }
----------------
This is going to add `SearchSpec`s 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.

================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:2081
@@ -2078,1 +2080,3 @@
+            extern_c = !CPlusPlusLanguage::IsCPPMangledName(function->GetMangled().GetMangledName().AsCString());
+
         if (!function_type)
----------------
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


Repository:
  rL LLVM

http://reviews.llvm.org/D17957