[Lldb-commits] [PATCH] Patch for LLDB demangler for demangling upon actual language

[Kirill Lapshin]

In http://reviews.llvm.org/D7302#116115, @clayborg wrote:

> I really can't take the memory hit that is incurred by adding anything to the Mangled class. lldb_private::Symbols are one of the largest sources of memory consumption by a large debug session in LLDB and adding and extra 64 bits (32 for the enum and 1 byte for the bool and then padding) to each one it too much. Can we make this change in a way that doesn't require the extra storage?
>
> Can't we tell what is mangled by looking at the prefix in a Mangled instance? If not, you will need to make a MangleWithLanguage instance for places the language needs to be used if it can't be deduced by the mangling prefix. The "m_language" is not really used inside the the Mangled class, so it isn't required for anything inside of the Mangled class.
>
> You should also not have to modify the DWARF parser at all, you can just get the lldb_private::CompileUnit from the DWARF compile unit and ask the lldb_private::CompileUnit for its language (lldb_private::CompileUnit::GetLanguage()).


Greg, Mangled class is low level helper to Symbol, Function and other code which use demangling. If DWARF allocated Mangled instance - language should be passed from DWARF attrs here - because by itself Mangled can't have reverse references to classes which can use Mangled class.


REPOSITORY
  rL LLVM

================
Comment at: include/lldb/Core/Mangled.h:347
@@ -318,1 +346,3 @@
+            Language m_language;        ///< Language, used for taking into account language differences in demangling
+    mutable bool m_bLanguage_was_set;   ///< Flag, means known language was set explicitly
 };
----------------
zturner wrote:
> Just call it m_language_was_set.  We don't use hungarian notation.
Fixed - see reworked patch at http://reviews.llvm.org/D7409.

================
Comment at: source/Core/Mangled.cpp:5095
@@ +5094,3 @@
+//----------------------------------------------------------------------
+Mangled::Mangled (const ConstString &s, bool mangled, lldb::LanguageType language) :
+    m_mangled(),
----------------
zturner wrote:
> I think the default constructor needs to be updated to initialialize m_language and m_b_language_was_set.
Fixed - see reworked patch at http://reviews.llvm.org/D7409.

================
Comment at: source/Core/Mangled.cpp:5226
@@ -5211,1 +5225,3 @@
+    if ((m_mangled && !m_demangled)
+        || m_bLanguage_was_set)
     {
----------------
zturner wrote:
> Instead of having this check, can you just clear m_demangled when you call SetLanguage()?  In fact, we could probably get rid of m_bLanguage_was_set entirely if you do this.
No, because my idea was - initially Symbol or Function name demangled against unknown (i.e. = C++ implicitly) language in places where language cannot be actualized, then language may be actualized (from CU language or if user set language explicitly for frame) and Mangled should do re-demangling upon actual language given via constructor or via SetLanguage() method. If set language still unknown - re-demangling is not performed. If you will clear m_demangled unconditionally on each SetLanguage() invocation you will be obliged to re-demangle name again and again - i.e. performance impact. So flag needed in addition to m_demangled != NULL check.

================
Comment at: source/Core/Mangled.cpp:5271-5273
@@ -5253,2 +5270,5 @@
                 {
-                    m_demangled.SetCStringWithMangledCounterpart(demangled_name, m_mangled);
+                    std::string temp_demangled_name(demangled_name);
+                    std::string cplusplus_sep("::");
+                    std::string pascaljava_sep(".");
+
----------------
zturner wrote:
> can the two separators be const char * instead of an std::string (to avoid a heap allocation), and also moved inside of the case statement for Pascal / Java?  None of the other branches use them.  Also temp_demangled_name can be moved into the case as well, and the default case statement can just use demangled_name.c_str()
Fixed - see reworked patch at http://reviews.llvm.org/D7409.

================
Comment at: source/Core/Mangled.cpp:5301
@@ -5265,3 +5300,3 @@
     }
-
+    m_bLanguage_was_set = false;
     return m_demangled;
----------------
zturner wrote:
> This line also goes away if we delete this variable and call m_demangled.Clear() in SetLanguage()
Please see my answer about why flag needed instead just m_demangled != NULL check above.

================
Comment at: source/Core/Mangled.cpp:5364
@@ +5363,3 @@
+    if (m_language.GetLanguage() != lldb::eLanguageTypeUnknown)
+        m_bLanguage_was_set = true;
+}
----------------
zturner wrote:
> Should this only happen if the new language is not equal to the old language?  (Also reiterating the comment about removing this variable and just clearing m_demangled)
Yes - delayed re-demangling starts only in case language changed to known language instead of unknown. But initially (first time) demangling always performed blindly against unknown language in C++ manner - because lots of language dependent lldb code written in implicit assumption what language is C++/ObjC, so I created my patch with respect to this.

http://reviews.llvm.org/D7302

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/