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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
