[Lldb-commits] [PATCH] D27223: Expression evaluation for overloaded C functions (redux)

[Luke Drummond via Phabricator via lldb-commits]

ldrumm added inline comments.


================
Comment at: include/lldb/Core/FastDemangle.h:22
+char *
+FastDemangle(const char *mangled_name, size_t mangled_name_length,
+             std::function<void(const char *s)> primitive_type_hook = nullptr);
----------------
alexshap wrote:
> 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). 
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.


================
Comment at: packages/Python/lldbsuite/test/expression_command/call-overloaded-c-fuction/main.c:17
+    return 0; // break here
+}
----------------
alexshap wrote:
> 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)")
Yes. The only thing that matters to the current implementation is that we know the mangled symbol name.


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:449
+/// Given a mangled function `mangled`, replace all the primitive function type
+/// arguments of `mangled` with type `replace`.
+static ConstString SubsPrimitiveParmItanium(const char *mangled,
----------------
clayborg wrote:
> Don't you mean `search` instead of `mangled` above?
I did. That was a silly mistake.


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:465-466
+  std::unique_ptr<char[]> output_buf(new char[max_len]);
+  ::memset(output_buf.get(), '\0', max_len);
+  ::strncpy(output_buf.get(), mangled, max_len - 1);
+
----------------
clayborg wrote:
> Remove both lines above if you use a std::string. Also, why zero it first and then copy the string over it?
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 :)


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:471
+
+  auto swap_parms_hook = [&](const char *s) {
+    if (!s || !*s)
----------------
clayborg wrote:
> Does this only get called with parameters? Or does it get called for everything?
I'm not sure I follow; `s` should always be a valid pointer if that's what you mean?


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:476
+    // Check whether we've found a substitutee
+    if (::strncmp(s, search, search_len) == 0) {
+      // account for the case where a replacement is of a different length
----------------
clayborg wrote:
> 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.
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.


https://reviews.llvm.org/D27223