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

JDevlieghere · JDevlieghere · commit c1d55d26d373 · 2023-04-21T10:23:24.000-07:00
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 are getting this wrong and 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 use `function.name-without-args` in the frame format. The formatter calls `Symbol::GetNameNoArguments()` which is a wrapper around `Mangled::GetName(ePreferDemangledWithoutArguments)`. The latter will first try using the appropriate language plugin to get the demangled name without arguments, and if that fails, falls back to returning the demangled name. Because we forced Mangled to treat the symbol as a mangled name (even though it's not) there's no demangled name. The result is that frames don't show any symbol at all. Differential revision: https://reviews.llvm.org/D148846
diff --git a/lldb/include/lldb/Core/Mangled.h b/lldb/include/lldb/Core/Mangled.h
@@ -185,19 +185,6 @@ class Mangled {
   ///     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
diff --git a/lldb/source/Core/Mangled.cpp b/lldb/source/Core/Mangled.cpp
@@ -90,23 +90,6 @@ int Mangled::Compare(const Mangled &a, const Mangled &b) {
                               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())) {
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -3030,7 +3030,7 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
                               // 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 {
@@ -3076,7 +3076,7 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
                                 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;
                             }
@@ -3468,17 +3468,13 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
                         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]
@@ -3938,8 +3934,8 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
               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 {
@@ -3977,7 +3973,7 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
                   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;
               }
@@ -4330,17 +4326,14 @@ void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
             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);
         }
       }
 
diff --git a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
@@ -246,7 +246,7 @@ FunctionSP SymbolFileBreakpad::GetOrCreateFunction(CompileUnit &comp_unit) {
 
   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) {
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2417,7 +2417,7 @@ DWARFASTParserClang::ParseFunctionFromDWARF(CompileUnit &comp_unit,
                                &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(
@@ -2428,9 +2428,9 @@ DWARFASTParserClang::ParseFunctionFromDWARF(CompileUnit &comp_unit,
       // 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;
diff --git a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -1980,7 +1980,7 @@ SymbolFilePDB::GetMangledForPDBFunc(const llvm::pdb::PDBSymbolFunc &pdb_func) {
   } 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;
 }
diff --git a/lldb/test/API/macosx/format/Makefile b/lldb/test/API/macosx/format/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/macosx/format/TestFunctionNameWithoutArgs.py b/lldb/test/API/macosx/format/TestFunctionNameWithoutArgs.py
@@ -0,0 +1,28 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class TestFunctionNameWithoutArgs(TestBase):
+    @skipUnlessDarwin
+    @no_debug_info_test
+    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"],
+        )
diff --git a/lldb/test/API/macosx/format/main.c b/lldb/test/API/macosx/format/main.c
@@ -0,0 +1,6 @@
+#include <assert.h>
+
+int main(int argc, char **argv) {
+  assert(argc != 1);
+  return 0;
+}

Original file line number	Diff line number	Diff line change
`@@ -1980,7 +1980,7 @@ SymbolFilePDB::GetMangledForPDBFunc(const llvm::pdb::PDBSymbolFunc &pdb_func) {`
`1980`	`1980`	`} else if (!func_undecorated_name.empty()) {`
`1981`	`1981`	`mangled.SetDemangledName(ConstString(func_undecorated_name));`
`1982`	`1982`	`} else if (!func_name.empty())`
`1983`		`- mangled.SetValue(ConstString(func_name), false);`
	`1983`	`+ mangled.SetValue(ConstString(func_name));`
`1984`	`1984`
`1985`	`1985`	`return mangled;`
`1986`	`1986`	`}`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+C_SOURCES := main.c`
	`2`	`+`
	`3`	`+include Makefile.rules`