Patch for LLDB demangler for demangling upon actual language

[ki.stfu]

Summary

This patch includes following changes:

• Add Language member to Mangled class with corresponding constructor. Minor refactoring in places where Mangled instances used.
• Function name demangling depending on its compile unit actual language.
• Enable demangling for case if function/sym language still unknown at moment when demangling requested.
• Flag indicating known language was set added. Demangler now is able to re-demangle given sym name if it language was re-set since Mangled object creation. Demangling against unknown language is allowed because in lldb unknown language means C++/ObjC in most cases implicitly.

[zturner]

include/lldb/Core/Mangled.h

94

I don't like having this second argument here. If you know in advance the name is demangled, why even use this class for anything?

347

Just call it m_language_was_set. We don't use hungarian notation.

source/Core/Mangled.cpp

5095

I think the default constructor needs to be updated to initialialize m_language and m_b_language_was_set.

5226

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.

5271–5273

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()

5301

This line also goes away if we delete this variable and call m_demangled.Clear() in SetLanguage()

5364

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)

[zturner]

include/lldb/Core/Mangled.h

94

Looks like this argument is here before. Removing it seems like a bigger task that's outside the scope of this CL. So ignore this comment for now.

[clayborg]

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()).

[clayborg]

To make my comments clearer:

1 - Can't we tell the language by looking at the Mangled prefix? "_Z" = C++, "$" = C++?
2 - Where do we need to know the language? I the JIT? If so we might need a new MangledWithLanguage class and use it only where we need it so it doesn't make lldb_private::Symbol and lldb_private::Function and lldb_private::Block's inlined info take up more space. We spent a lot of time doing memory analysis on LLDB and lldb_private::Symbol objects were consistently at the top of the heap.

[zturner]

Greg: How important is it from a performance perspective that Symbol be able to cache the demangled name and not recompute it every time you call GetDemangledName?

[clayborg]

Not very important actually because our constant string pool (lldb_private::ConstString) has a:

bool
ConstString::GetMangledCounterpart (ConstString &counterpart) const;

So we could just store the name and note if is is mangled or not and we would still have an accessor on symbol that we could change from:

Mangled&
GetMangled ()
{
    return m_mangled;
}

const Mangled&
GetMangled () const
{
    return m_mangled;
}

to be:

Mangled
GetMangled ()
{

Mangled mangled;

/// Calculate and return a Mangled instance...

...
return mangled;

}

But again, this would help get lldb_private::Symbol to take less memory than it already does, but I still wouldn't want to lose those gains in savings by adding language with every mangled name. It doesn't really need to be there. And where it does and where it is important, we could add a language. It seems like a heavy hammer for a small controlled use case.

Greg

[zturner]

I agree that if memory is important then we should use the opportunity to reduce memory usage rather than keeping it the same by changing stuff. But the reason I asked leads into my next question.

I've been thinking about mangling and demangling for a while and how it relates to Windows. I see a lot of code all over the place that manually inspects mangled names, and usually the code is all custom and handrolled. (If you're interested I can point you to a bunch of examples). I don't like this way of doing things and I think it's generally fragile. There should be one place that's responsible for anything to do with mangling. All these places that are inspecting strings for _Z or ? should just be calling some class to ask it about the properties of this string.

The most sensible place to do that, to me, seems like the ABI. So I'm imagining that there's a Mangler base class, and then from that there is an ItaniumCppMangler, a MsCppMangler, and let's say perhaps a JavaMangler for the purposes of this CL. Maybe they share some code, but that's not the important part.

ABI provides a method called getMangler(). It returns a singleton instance (which for Windows would be an MsCppMangler, and for everyone else would be an ItaiumCppMangler).

In the Symbol class, then, all you need to store is the mangled name. Implement a method in Symbol called getMangler() which looks at m_comp_unit, and either gets the ABI and calls getCppMangler (if Lang is C++) or a null mangler (if Lang is C) or a java mangler (if Lang is Java), etc.

Then, just call the method on it.

All this seems complicated, but the advantage is that now this logic is abstracted for anyone else who wants to use it. The Mangler interface could provide such methods as IsGuardVariable() or IsFunction() that things like the interpreter could use by getting the correct mangler from the ABI, for example. And all of the places in the code that currently have hardcoded mangler checks could be made to work in the presence of ABI differences and language differences easily.

And this doesn't impose any memory penalty on Symbol (and actually reduces the footprint of each Symbol by the size of 1 pointer)

[clayborg]

I do also want to stress that I believe that all the places that are manually checking for "_Z" and the likes switch over to using methods on Mangled so we can avoid the issues you are seeing.

[KLapshin]

Changes requested in this review were taken into account in reworked patch - see http://reviews.llvm.org/D7409.

[KLapshin]

In D7302#116129, @clayborg wrote:

To make my comments clearer:

1 - Can't we tell the language by looking at the Mangled prefix? "_Z" = C++, "$" = C++?
2 - Where do we need to know the language? I the JIT? If so we might need a new MangledWithLanguage class and use it only where we need it so it doesn't make lldb_private::Symbol and lldb_private::Function and lldb_private::Block's inlined info take up more space. We spent a lot of time doing memory analysis on LLDB and lldb_private::Symbol objects were consistently at the top of the heap.

1 - No - because, for example, Delphi use same mangling, but has thin differences in types name demangling and namespace separator (same with Java).

2 - We need to know exact language for given function or method name demangling with its parameters types on full header.

[KLapshin]

In 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.

include/lldb/Core/Mangled.h

347

Fixed - see reworked patch at http://reviews.llvm.org/D7409.

source/Core/Mangled.cpp

5095

Fixed - see reworked patch at http://reviews.llvm.org/D7409.

5226

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.

5271–5273

Fixed - see reworked patch at http://reviews.llvm.org/D7409.

5301

Please see my answer about why flag needed instead just m_demangled != NULL check above.

5364

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.

[ki.stfu]

See additional information at http://reviews.llvm.org/D7409

[dawn]

See http://reviews.llvm.org/D10744 for a rewrite of this patch.

include/lldb/Core/Mangled.h

 
 #include "lldb/lldb-private.h"
 #include "lldb/Core/ConstString.h"
+#include "lldb/Core/Language.h"
 #include <vector>
 
 namespace lldb_private {

     Mangled (const ConstString &name);
 
     //----------------------------------------------------------------------
+    /// Construct with name and language.
+    ///
+    /// Constructor with an optional string, a boolean indicating if it is the
+    /// mangled version, and the preferred language.
+    ///
+    /// @param[in] name
+    ///     The already const name to copy into this object.
+    ///
+    /// @param[in] is_mangled
+    ///     If \b true then \a name is a mangled name, if \b false then
+    ///     \a name is demangled.
+    //----------------------------------------------------------------------
+    explicit
+    Mangled (const ConstString &name, bool is_mangled, lldb::LanguageType language);
+
+    //----------------------------------------------------------------------
     /// Destructor
     ///
     /// Releases its ref counts on the mangled and demangled strings that

 
     //----------------------------------------------------------------------
     /// Get the language only if it is definitive what the language is from
-    /// the mangling.
+    /// the mangling (or unless set via SetLanguage).
     ///
     /// For a mangled name to have a language it must have both a mangled
     /// and a demangled name and it must be definitive from the mangling

     ///     if there is no mangled or demangled counterpart.
     //----------------------------------------------------------------------
     lldb::LanguageType
-    GetLanguage ();
+    GetLanguage () const;
 
+    //----------------------------------------------------------------------
+    /// Set accessor for the language object, associated with Mangled
+    /// instance.
+    ///
+    /// @param[in] language
+    ///     The new enumeration value that describes the programming
+    ///     language that a Mangled is associated with.
+    //------------------------------------------------------------------
+    void
+    SetLanguage (lldb::LanguageType language);
+
 private:
     //----------------------------------------------------------------------
     /// Mangled member variables.
     //----------------------------------------------------------------------
             ConstString m_mangled;      ///< The mangled version of the name
     mutable ConstString m_demangled;    ///< Mutable so we can get it on demand with a const version of this object
+            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
 };
 
 

source/Core/Mangled.cpp

 }
 
 //----------------------------------------------------------------------
+// Constructor with an optional string, a boolean indicating if it is the
+// mangled version, and the preferred language.
+//----------------------------------------------------------------------
+Mangled::Mangled (const ConstString &s, bool mangled, lldb::LanguageType language) :
+    m_mangled(),
+    m_demangled(),
+    m_language(language),
+    m_bLanguage_was_set(false)
+{
+    if (s)
+        SetValue(s, mangled);
+}
+
+//----------------------------------------------------------------------
 // Destructor
 //----------------------------------------------------------------------
 Mangled::~Mangled ()

 {
     m_mangled.Clear();
     m_demangled.Clear();
+    m_bLanguage_was_set = false;
 }
 
 

 Mangled::GetDemangledName () const
 {
     // Check to make sure we have a valid mangled name and that we
-    // haven't already decoded our mangled name.
-    if (m_mangled && !m_demangled)
+    // haven't already decoded our mangled name or language was re-set since object created.
+    if ((m_mangled && !m_demangled)
+        || m_bLanguage_was_set)
     {
         // We need to generate and cache the demangled name.
         Timer scoped_timer (__PRETTY_FUNCTION__,

         const char *mangled_cstr = m_mangled.GetCString();
         if (cstring_is_mangled(mangled_cstr))
         {
-            if (!m_mangled.GetMangledCounterpart(m_demangled))
+            if (!m_mangled.GetMangledCounterpart(m_demangled)
+                || m_bLanguage_was_set)
             {
                 // We didn't already mangle this name, demangle it and if all goes well
                 // add it to our map.

 
                 if (demangled_name)
                 {
-                    m_demangled.SetCStringWithMangledCounterpart(demangled_name, m_mangled);
+                    std::string temp_demangled_name(demangled_name);
+                    std::string cplusplus_sep("::");
+                    std::string pascaljava_sep(".");
+
+                    switch(m_language.GetLanguage())
+                    {
+                        case lldb::eLanguageTypePascal83:
+                        case lldb::eLanguageTypeJava:
+                            {
+                                size_t start_pos = temp_demangled_name.find(cplusplus_sep, 0);
+                                while(start_pos != std::string::npos)
+                                {
+                                    temp_demangled_name.replace(start_pos, cplusplus_sep.length(), pascaljava_sep);
+                                    start_pos = temp_demangled_name.find(cplusplus_sep, start_pos);
+                                }
+                            }
+                        default :
+                            m_demangled.SetCStringWithMangledCounterpart(temp_demangled_name.c_str(), m_mangled);
+                    }
                     free (demangled_name);
                 }
             }

             m_demangled.SetCString("");
         }
     }
-
+    m_bLanguage_was_set = false;
     return m_demangled;
 }
 

 }
 
 //----------------------------------------------------------------------
+// Get current language.
+//----------------------------------------------------------------------
+lldb::LanguageType
+Mangled::GetLanguage () const
+{
+    return m_language.GetLanguage();
+}
+
+//----------------------------------------------------------------------
+// Set current language.
+//----------------------------------------------------------------------
+void
+Mangled::SetLanguage (lldb::LanguageType language)
+{
+    m_language.SetLanguage(language);
+    if (m_language.GetLanguage() != lldb::eLanguageTypeUnknown)
+        m_bLanguage_was_set = true;
+}
+
+//----------------------------------------------------------------------
 // Dump a Mangled object to stream "s". We don't force our
 // demangled name to be computed currently (we don't use the accessor).
 //----------------------------------------------------------------------

     return m_mangled.MemorySize() + m_demangled.MemorySize();
 }
 
-lldb::LanguageType
-Mangled::GetLanguage ()
-{
-    ConstString mangled = GetMangledName();
-    if (mangled)
-    {
-        if (GetDemangledName())
-        {
-            if (cstring_is_mangled(mangled.GetCString()))
-                return lldb::eLanguageTypeC_plus_plus;
-        }
-    }
-    return  lldb::eLanguageTypeUnknown;
-}
-
 //----------------------------------------------------------------------
 // Dump OBJ to the supplied stream S.
 //----------------------------------------------------------------------

source/Expression/ClangExpressionDeclMap.cpp

     {
         // Sometimes we get a mangled name for a global function that actually should be "extern C."
         // This is a hack to compensate.
-        
         const bool is_mangled = true;
-        Mangled mangled(name, is_mangled);
+        LanguageType language = m_parser_vars->m_sym_ctx.comp_unit ? m_parser_vars->m_sym_ctx.comp_unit->GetLanguage() : eLanguageTypeUnknown;
+
+        Mangled mangled(name, is_mangled, language);
                 
         CPPLanguageRuntime::MethodName method_name(mangled.GetDemangledName());
         

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp

                     // with duplicate entries
                     if (name != mangled_cstr && ((mangled_cstr[0] == '_') || (name && ::strcmp(name, mangled_cstr) != 0)))
                     {
-                        Mangled mangled (ConstString(mangled_cstr), true);
+                        LanguageType cu_language = (LanguageType)die.GetAttributeValueAsUnsigned(m_dwarf2Data, this, DW_AT_language, 0);
+                        Mangled mangled (ConstString(mangled_cstr), true, cu_language);
                         func_fullnames.Insert (mangled.GetMangledName(), die.GetOffset());
                         if (mangled.GetDemangledName())
                             func_fullnames.Insert (mangled.GetDemangledName(), die.GetOffset());

                     // with duplicate entries
                     if (name != mangled_cstr && ((mangled_cstr[0] == '_') || (::strcmp(name, mangled_cstr) != 0)))
                     {
-                        Mangled mangled (ConstString(mangled_cstr), true);
+                        LanguageType cu_language = (LanguageType)die.GetAttributeValueAsUnsigned(m_dwarf2Data, this, DW_AT_language, 0);
+                        Mangled mangled (ConstString(mangled_cstr), true, cu_language);
                         func_fullnames.Insert (mangled.GetMangledName(), die.GetOffset());
                         if (mangled.GetDemangledName())
                             func_fullnames.Insert (mangled.GetDemangledName(), die.GetOffset());

                 // with duplicate entries
                 if (mangled_cstr && name != mangled_cstr && ((mangled_cstr[0] == '_') || (::strcmp(name, mangled_cstr) != 0)))
                 {
-                    Mangled mangled (ConstString(mangled_cstr), true);
+                    LanguageType cu_language = (LanguageType)die.GetAttributeValueAsUnsigned(m_dwarf2Data, this, DW_AT_language, 0);
+                    Mangled mangled (ConstString(mangled_cstr), true, cu_language);
                     globals.Insert (mangled.GetMangledName(), die.GetOffset());
                     if (mangled.GetDemangledName())
                         globals.Insert (mangled.GetDemangledName(), die.GetOffset());

source/Symbol/Function.cpp

 {
     m_block.SetParentScope(this);
     assert(comp_unit != nullptr);
+    m_mangled.SetLanguage(comp_unit->GetLanguage());
 }
 
 Function::Function

     m_comp_unit (comp_unit),
     m_type_uid (type_uid),
     m_type (type),
-    m_mangled (ConstString(mangled), true),
+    m_mangled (ConstString(mangled), true, eLanguageTypeUnknown),
     m_block (func_uid),
     m_range (range),
     m_frame_base (),

 {
     m_block.SetParentScope(this);
     assert(comp_unit != nullptr);
+    m_mangled.SetLanguage(comp_unit->GetLanguage());
 }
 
 

source/Symbol/Symbol.cpp

     m_size_is_valid (size_is_valid || size > 0),
     m_demangled_is_synthesized (false),
     m_type (type),
-    m_mangled (ConstString(name), name_is_mangled),
+    // TODO: use context language
+    m_mangled(ConstString(name), name_is_mangled, eLanguageTypeUnknown),
     m_addr_range (section_sp, offset, size),
     m_flags (flags)
 {

     m_size_is_valid (size_is_valid || range.GetByteSize() > 0),
     m_demangled_is_synthesized (false),
     m_type (type),
-    m_mangled (ConstString(name), name_is_mangled),
+    // TODO: use context language
+    m_mangled(ConstString(name), name_is_mangled, eLanguageTypeUnknown),
     m_addr_range (range),
     m_flags (flags)
 {

source/Symbol/Variable.cpp

 ) :
     UserID(uid),
     m_name(name),
-    m_mangled (ConstString(mangled), true),
+    // TODO: language
+    m_mangled(ConstString(mangled), true, eLanguageTypeUnknown),
     m_symfile_type_sp(symfile_type_sp),
     m_scope(scope),
     m_owner_scope(context),