[lldb] Let Mangled decide whether a name is mangled or not

[JDevlieghere]

Summary

We have a handful of places in LLDB where we try to outsmart the logic in Mangled to determine whether a string is mangled or not. There's at least one place (*) where we were getting this wrong and this causes a subtle bug. The cstring_is_mangled is cheap enough that we should always rely on it to determine whether a string is mangled or not.

(*) ObjectFileMachO assumes that a symbol that starts with a double underscore (such as __pthread_kill) is mangled. That's mostly harmless, until you want to use function.name-without-args in your frame format. For this formatter, we call Symbol::GetNameNoArguments() which is a wrapper around Mangled::GetName(Mangled::ePreferDemangledWithoutArguments). The latter will first try using the appropriate language plugin to get the demangled name without arguments, but if that fails, it falls back to returning the demangled name. Because we forced Mangled to treat the symbol as a mangled name while it's not, there's no demangled name, and your frames don't show symbols at all. Alternatively, I could've worked around the issue by checking if we have a demangeld at all, but that's just working around the bug.

[JDevlieghere]

Add simple test

[aprantl]

It seems like we can quickly and unambiguously decide whether a name is mangled or not by looking at the first characters so that seems fine to me. Does Mangled try all Language plugins to decide?
This seems like a risky change, but I think it's going into the right direction.

[aprantl]

lldb/test/API/macosx/format/TestFunctionNameWithoutArgs.py

1

why?

7

I guess this is a no debug info test?

lldb/test/API/macosx/format/main.c

2

why?

[JDevlieghere]

lldb/test/API/macosx/format/TestFunctionNameWithoutArgs.py

1

I assume the question is rhetoric, but on the off-chance it's not: I copy-pasted it from another test. Same for the other "why?".

lldb/include/lldb/Core/Mangled.h

public:
   /// refer to.
   ///
   /// \return
   ///     The number of bytes that this object occupies in memory.
   size_t MemorySize() const;
 
   /// Set the string value in this object.
   ///
-  /// If \a is_mangled is \b true, then the mangled named is set to \a name,
-  /// else the demangled name is set to \a name.
-  ///
-  /// \param[in] name
-  ///     The already const version of the name for this object.
-  ///
-  /// \param[in] is_mangled
-  ///     If \b true then \a name is a mangled name, if \b false then
-  ///     \a name is demangled.
-  void SetValue(ConstString name, bool is_mangled);
-
-  /// Set the string value in this object.
-  ///
   /// This version auto detects if the string is mangled by inspecting the
   /// string value and looking for common mangling prefixes.
   ///
   /// \param[in] name
   ///     The already const version of the name for this object.
   void SetValue(ConstString name);
 
   /// Try to guess the language from the mangling.

lldb/source/Core/Mangled.cpp

 }
 
 // Compare the string values.
 int Mangled::Compare(const Mangled &a, const Mangled &b) {
   return ConstString::Compare(a.GetName(ePreferMangled),
                               b.GetName(ePreferMangled));
 }
 
-// Set the string value in this objects. If "mangled" is true, then the mangled
-// named is set with the new value in "s", else the demangled name is set.
-void Mangled::SetValue(ConstString s, bool mangled) {
-  if (s) {
-    if (mangled) {
-      m_demangled.Clear();
-      m_mangled = s;
-    } else {
-      m_demangled = s;
-      m_mangled.Clear();
-    }
-  } else {
-    m_demangled.Clear();
-    m_mangled.Clear();
-  }
-}
-
 void Mangled::SetValue(ConstString name) {
   if (name) {
     if (cstring_is_mangled(name.GetStringRef())) {
       m_demangled.Clear();
       m_mangled = name;
     } else {
       m_demangled = name;
       m_mangled.Clear();

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

if (nlist_buffer) {
                           const bool N_SO_has_full_path = symbol_name[0] == '/';
                           if (N_SO_has_full_path) {
                             if ((N_SO_index == sym_idx - 1) &&
                                 ((sym_idx - 1) < num_syms)) {
                               // We have two consecutive N_SO entries where
                               // the first contains a directory and the
                               // second contains a full path.
                               sym[sym_idx - 1].GetMangled().SetValue(
-                                  ConstString(symbol_name), false);
+                                  ConstString(symbol_name));
                               m_nlist_idx_to_sym_idx[nlist_idx] = sym_idx - 1;
                               add_nlist = false;
                             } else {
                               // This is the first entry in a N_SO that
                               // contains a directory or
                               // a full path to the source file
                               N_SO_index = sym_idx;
                             }
if (nlist_buffer) {
                                     full_so_path.erase(0, double_slash_pos + 1);
                                   }
                                 }
                               }
                               if (*full_so_path.rbegin() != '/')
                                 full_so_path += '/';
                               full_so_path += symbol_name;
                               sym[sym_idx - 1].GetMangled().SetValue(
-                                  ConstString(full_so_path.c_str()), false);
+                                  ConstString(full_so_path.c_str()));
                               add_nlist = false;
                               m_nlist_idx_to_sym_idx[nlist_idx] = sym_idx - 1;
                             }
                           } else {
                             // This could be a relative path to a N_SO
                             N_SO_index = sym_idx;
                           }
                         }
if (nlist_buffer) {
                     if (add_nlist) {
                       uint64_t symbol_value = nlist.n_value;
                       if (symbol_name_non_abi_mangled) {
                         sym[sym_idx].GetMangled().SetMangledName(
                             ConstString(symbol_name_non_abi_mangled));
                         sym[sym_idx].GetMangled().SetDemangledName(
                             ConstString(symbol_name));
                       } else {
-                        bool symbol_name_is_mangled = false;
-
                         if (symbol_name && symbol_name[0] == '_') {
-                          symbol_name_is_mangled = symbol_name[1] == '_';
                           symbol_name++; // Skip the leading underscore
                         }
 
                         if (symbol_name) {
                           ConstString const_symbol_name(symbol_name);
-                          sym[sym_idx].GetMangled().SetValue(
-                              const_symbol_name, symbol_name_is_mangled);
+                          sym[sym_idx].GetMangled().SetValue(const_symbol_name);
                           if (is_gsym && is_debug) {
                             const char *gsym_name =
                                 sym[sym_idx]
                                     .GetMangled()
                                     .GetName(Mangled::ePreferMangled)
                                     .GetCString();
                             if (gsym_name)
                               N_GSYM_name_to_sym_idx[gsym_name] = sym_idx;
auto ParseSymbolLambda = [&](struct nlist_64 &nlist, uint32_t nlist_idx,
             // We use the current number of symbols in the symbol table in
             // lieu of using nlist_idx in case we ever start trimming entries
             // out
             const bool N_SO_has_full_path = symbol_name[0] == '/';
             if (N_SO_has_full_path) {
               if ((N_SO_index == sym_idx - 1) && ((sym_idx - 1) < num_syms)) {
                 // We have two consecutive N_SO entries where the first
                 // contains a directory and the second contains a full path.
-                sym[sym_idx - 1].GetMangled().SetValue(ConstString(symbol_name),
-                                                       false);
+                sym[sym_idx - 1].GetMangled().SetValue(
+                    ConstString(symbol_name));
                 m_nlist_idx_to_sym_idx[nlist_idx] = sym_idx - 1;
                 add_nlist = false;
               } else {
                 // This is the first entry in a N_SO that contains a
                 // directory or a full path to the source file
                 N_SO_index = sym_idx;
               }
             } else if ((N_SO_index == sym_idx - 1) &&
auto ParseSymbolLambda = [&](struct nlist_64 &nlist, uint32_t nlist_idx,
                       full_so_path.erase(0, double_slash_pos + 1);
                     }
                   }
                 }
                 if (*full_so_path.rbegin() != '/')
                   full_so_path += '/';
                 full_so_path += symbol_name;
                 sym[sym_idx - 1].GetMangled().SetValue(
-                    ConstString(full_so_path.c_str()), false);
+                    ConstString(full_so_path.c_str()));
                 add_nlist = false;
                 m_nlist_idx_to_sym_idx[nlist_idx] = sym_idx - 1;
               }
             } else {
               // This could be a relative path to a N_SO
               N_SO_index = sym_idx;
             }
           }
auto ParseSymbolLambda = [&](struct nlist_64 &nlist, uint32_t nlist_idx,
 
       uint64_t symbol_value = nlist.n_value;
 
       if (symbol_name_non_abi_mangled) {
         sym[sym_idx].GetMangled().SetMangledName(
             ConstString(symbol_name_non_abi_mangled));
         sym[sym_idx].GetMangled().SetDemangledName(ConstString(symbol_name));
       } else {
-        bool symbol_name_is_mangled = false;
 
         if (symbol_name && symbol_name[0] == '_') {
-          symbol_name_is_mangled = symbol_name[1] == '_';
           symbol_name++; // Skip the leading underscore
         }
 
         if (symbol_name) {
           ConstString const_symbol_name(symbol_name);
-          sym[sym_idx].GetMangled().SetValue(const_symbol_name,
-                                             symbol_name_is_mangled);
+          sym[sym_idx].GetMangled().SetValue(const_symbol_name);
         }
       }
 
       if (is_gsym) {
         const char *gsym_name = sym[sym_idx]
                                     .GetMangled()
                                     .GetName(Mangled::ePreferMangled)
                                     .GetCString();

lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp

FunctionSP SymbolFileBreakpad::GetOrCreateFunction(CompileUnit &comp_unit) {
 
   const SectionList *list = comp_unit.GetModule()->GetSectionList();
   CompUnitData &data = m_cu_data->GetEntryRef(id).data;
   LineIterator It(*m_objfile_sp, Record::Func, data.bookmark);
   assert(Record::classify(*It) == Record::Func);
 
   if (auto record = FuncRecord::parse(*It)) {
     Mangled func_name;
-    func_name.SetValue(ConstString(record->Name), false);
+    func_name.SetValue(ConstString(record->Name));
     addr_t address = record->Address + base;
     SectionSP section_sp = list->FindSectionContainingFileAddress(address);
     if (section_sp) {
       AddressRange func_range(
           section_sp, address - section_sp->GetFileAddress(), record->Size);
       // Use the CU's id because every CU has only one function inside.
       func_sp = std::make_shared<Function>(&comp_unit, id, 0, func_name,
                                            nullptr, func_range);

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

DWARFASTParserClang::ParseFunctionFromDWARF(CompileUnit &comp_unit,
   if (tag != DW_TAG_subprogram)
     return nullptr;
 
   if (die.GetDIENamesAndRanges(name, mangled, func_ranges, decl_file, decl_line,
                                decl_column, call_file, call_line, call_column,
                                &frame_base)) {
     Mangled func_name;
     if (mangled)
-      func_name.SetValue(ConstString(mangled), true);
+      func_name.SetValue(ConstString(mangled));
     else if ((die.GetParent().Tag() == DW_TAG_compile_unit ||
               die.GetParent().Tag() == DW_TAG_partial_unit) &&
              Language::LanguageIsCPlusPlus(
                  SymbolFileDWARF::GetLanguage(*die.GetCU())) &&
              !Language::LanguageIsObjC(
                  SymbolFileDWARF::GetLanguage(*die.GetCU())) &&
              name && strcmp(name, "main") != 0) {
       // If the mangled name is not present in the DWARF, generate the
       // demangled name using the decl context. We skip if the function is
       // "main" as its name is never mangled.
-      func_name.SetValue(ConstructDemangledNameFromDWARF(die), false);
+      func_name.SetValue(ConstructDemangledNameFromDWARF(die));
     } else
-      func_name.SetValue(ConstString(name), false);
+      func_name.SetValue(ConstString(name));
 
     FunctionSP func_sp;
     std::unique_ptr<Declaration> decl_up;
     if (decl_file || decl_line || decl_column)
       decl_up = std::make_unique<Declaration>(
           die.GetCU()->GetFile(decl_file ? *decl_file : 0),
           decl_line ? *decl_line : 0, decl_column ? *decl_column : 0);
 

lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

if (!func_decorated_name.empty()) {
     // PDB source unless we also apply same flags in getting undecorated
     // name through PDBSymbolFunc::getUndecoratedNameEx method.
     if (!func_undecorated_name.empty() &&
         mangled.GetDemangledName() != ConstString(func_undecorated_name))
       mangled.SetDemangledName(ConstString(func_undecorated_name));
   } else if (!func_undecorated_name.empty()) {
     mangled.SetDemangledName(ConstString(func_undecorated_name));
   } else if (!func_name.empty())
-    mangled.SetValue(ConstString(func_name), false);
+    mangled.SetValue(ConstString(func_name));
 
   return mangled;
 }
 
 bool SymbolFilePDB::DeclContextMatchesThisSymbolFile(
     const lldb_private::CompilerDeclContext &decl_ctx) {
   if (!decl_ctx.IsValid())
     return true;

lldb/test/API/macosx/format/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/macosx/format/TestFunctionNameWithoutArgs.py

+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestFunctionNameWithoutArgs(TestBase):
+    @skipUnlessDarwin
+    def test_function_name_without_args(self):
+        self.build()
+        target = self.createTestTarget()
+        target.LaunchSimple(None, None, self.get_process_working_directory())
+
+        self.runCmd("run", RUN_SUCCEEDED)
+        self.expect(
+            "bt",
+            substrs=[
+                "stop reason = hit program assert",
+                "libsystem_kernel.dylib`__pthread_kill",
+            ],
+        )
+        self.runCmd(
+            'settings set frame-format "frame #${frame.index}: ${function.name-without-args}\n"'
+        )
+        self.expect(
+            "bt",
+            substrs=["stop reason = hit program assert", "frame #0: __pthread_kill"],
+        )

lldb/test/API/macosx/format/main.c

+#include <assert.h>
+#include <stdio.h>
+
+int main(int argc, char **argv) {
+  assert(argc != 1);
+  return 0;
+}