[lldb][ClangExpression] Remove storage-class check when creating AsmLabel

[Michael137]

Summary

This check was put in place to prevent static functions
from translation units outside the one that the current
expression is evaluated from, from taking precedence over functions
in the global namespace. However, this is really a different
bug. LLDB lumps functions from all CUs into a single AST and
ends up picking the file-static even when C++ context rules
wouldn't allow that to happen.

This patch removes the check so we apply the AsmLabel to all
FunctionDecls we create from DWARF if we have a linkage name
available. This makes the code-path easier to reason about and
allows calling static functions in contexts where we previously
would've chosen the wrong function.

We also flip the XFAILs in the API test to reflect what effect
this change has.

Testing

• Fixed API tests and added XFAIL

[Michael137]

lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py

228–229

should probably move this to the appropriate test-function that isn't xfailed so we don't spin up an lldb instance just for this

[Michael137]

• Combine xfail tests cases

[Michael137]

• Remove redundant breakpoint

[labath]

Yeah, fixing this correctly is not going to be trivial. Off the top of my head, I can think of two solutions (creating per-file ASTs for static objects, or putting all static objects from a given file into a special namespace) and each of them has a lot of drawbacks.

[Michael137]

• Move one passing test case out of xfail

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

if (!type_handled) {
           // using symbols that are mangled according to the DW_AT_linkage_name.
           // If we didn't do this, the external symbols wouldn't exactly
           // match the mangled name LLDB knows about and the IRExecutionUnit
           // would have to fall back to searching object files for
           // approximately matching function names. The motivating
           // example is generating calls to ABI-tagged template functions.
           // This is done separately for member functions in
           // AddMethodToCXXRecordType.
-          if (attrs.mangled_name && attrs.storage == clang::SC_Extern)
+          if (attrs.mangled_name)
             function_decl->addAttr(clang::AsmLabelAttr::CreateImplicit(
                 m_ast.getASTContext(), attrs.mangled_name, /*literal=*/false));
 
           LinkDeclContextToDIE(function_decl, die);
 
           if (!function_param_decls.empty()) {
             m_ast.SetFunctionParameters(function_decl, function_param_decls);
             if (template_function_decl)

lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py

class NamespaceLookupTestCase(TestBase):
     def runToBkpt(self, command):
         self.runCmd(command, RUN_SUCCEEDED)
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stopped',
                              'stop reason = breakpoint'])
 
     @skipIfWindows # This is flakey on Windows: llvm.org/pr38373
+    @expectedFailure("CU-local objects incorrectly scoped")
+    def test_scope_lookup_with_run_command_globals(self):
+        """Test scope lookup of functions in lldb."""
+        self.build()
+
+        lldbutil.run_to_source_breakpoint(
+                self,
+                self.line_break_global_scope,
+                lldb.SBFileSpec("ns.cpp"))
+
+        lldbutil.run_break_set_by_file_and_line(
+            self,
+            "ns3.cpp",
+            self.line_break_before_using_directive,
+            num_expected_locations=1,
+            loc_exact=False)
+
+        # Run to BP_global_scope at file scope
+        self.runToBkpt("run")
+
+        # FIXME: LLDB does not correctly scope CU-local objects.
+        # LLDB currently lumps functions from all files into
+        # a single AST and depending on the order with which
+        # functions are considered, LLDB can incorrectly call
+        # the static local ns.cpp::func() instead of the expected
+        # ::func()
+
+        # Evaluate func() - should call ::func()
+        self.expect_expr("func()", expect_type="int", expect_value="1")
+
+        # Evaluate ::func() - should call A::func()
+        self.expect_expr("::func()", result_type="int", result_value="1")
+
+        # Continue to BP_before_using_directive at file scope
+        self.runToBkpt("continue")
+
+        # Evaluate func() - should call ::func()
+        self.expect_expr("func()", result_type="int", result_value="1")
+
+        # Evaluate ::func() - should call ::func()
+        self.expect_expr("::func()", result_type="int", result_value="1")
+
+        # Continue to BP_after_using_directive at file scope
+        self.runToBkpt("continue")
+
+        # Evaluate ::func() - should call ::func()
+        self.expect_expr("::func()", result_type="int", result_value="1")
+
+    @skipIfWindows # This is flakey on Windows: llvm.org/pr38373
     def test_scope_lookup_with_run_command(self):
         """Test scope lookup of functions in lldb."""
         self.build()
         self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
 
         lldbutil.run_break_set_by_file_and_line(
             self,
             "ns.cpp",
def test_scope_lookup_with_run_command(self):
         lldbutil.run_break_set_by_file_and_line(
             self,
             "ns2.cpp",
             self.line_break_nested_ns_scope_after_using,
             num_expected_locations=1,
             loc_exact=False)
         lldbutil.run_break_set_by_file_and_line(
             self,
+            "ns2.cpp",
+            self.line_break_file_scope,
+            num_expected_locations=1,
+            loc_exact=False)
+        lldbutil.run_break_set_by_file_and_line(
+            self,
             "ns3.cpp",
             self.line_break_before_using_directive,
             num_expected_locations=1,
             loc_exact=False)
         lldbutil.run_break_set_by_file_and_line(
             self,
             "ns3.cpp",
             self.line_break_after_using_directive,
             num_expected_locations=1,
             loc_exact=False)
 
         # Run to BP_global_scope at global scope
         self.runToBkpt("run")
-        # Evaluate func() - should call ::func()
-        self.expect_expr("func()", result_type="int", result_value="1")
+
         # Evaluate A::B::func() - should call A::B::func()
         self.expect_expr("A::B::func()", result_type="int", result_value="4")
         # Evaluate func(10) - should call ::func(int)
         self.expect_expr("func(10)", result_type="int", result_value="11")
-        # Evaluate ::func() - should call A::func()
-        self.expect_expr("::func()", result_type="int", result_value="1")
         # Evaluate A::foo() - should call A::foo()
         self.expect_expr("A::foo()", result_type="int", result_value="42")
 
+        # Continue to BP_file_scope at file scope
+        self.runToBkpt("continue")
+        self.expect_expr("func()", result_type="int", result_value="2")
+
         # Continue to BP_ns_scope at ns scope
         self.runToBkpt("continue")
         # Evaluate func(10) - should call A::func(int)
         self.expect_expr("func(10)", result_type="int", result_value="13")
         # Evaluate B::func() - should call B::func()
         self.expect_expr("B::func()", result_type="int", result_value="4")
         # Evaluate func() - should call A::func()
         self.expect_expr("func()", result_type="int", result_value="3")
def test_scope_lookup_with_run_command(self):
         # using declaration
         self.runToBkpt("continue")
         # Evaluate A::func(10) - should call A::func(int)
         self.expect_expr("A::func(10)", result_type="int", result_value="13")
 
         # Continue to BP_before_using_directive at global scope before using
         # declaration
         self.runToBkpt("continue")
-        # Evaluate ::func() - should call ::func()
-        self.expect_expr("::func()", result_type="int", result_value="1")
         # Evaluate B::func() - should call B::func()
         self.expect_expr("B::func()", result_type="int", result_value="4")
 
         # Continue to BP_after_using_directive at global scope after using
         # declaration
         self.runToBkpt("continue")
-        # Evaluate ::func() - should call ::func()
-        self.expect_expr("::func()", result_type="int", result_value="1")
         # Evaluate B::func() - should call B::func()
         self.expect_expr("B::func()", result_type="int", result_value="4")
 
     @expectedFailure("lldb scope lookup of functions bugs")
     def test_function_scope_lookup_with_run_command(self):
         """Test scope lookup of functions in lldb."""
         self.build()
         self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
def test_function_scope_lookup_with_run_command(self):
         self.expect_expr("::foo()", result_type="int", result_value="42")
 
         # Continue to BP_ns_scope at ns scope
         self.runToBkpt("continue")
         # Evaluate foo() - should call A::foo()
         # FIXME: lldb finds Y::foo because lookup for variables is done
         # before functions.
         self.expect_expr("foo()", result_type="int", result_value="42")
 
-    @expectedFailure("lldb file scope lookup bugs")
-    @skipIfWindows # This is flakey on Windows: llvm.org/pr38373
-    def test_file_scope_lookup_with_run_command(self):
-        """Test file scope lookup in lldb."""
-        self.build()
-        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
-
-        lldbutil.run_break_set_by_file_and_line(
-            self,
-            "ns2.cpp",
-            self.line_break_file_scope,
-            num_expected_locations=1,
-            loc_exact=False)
-
-        # Run to BP_file_scope at file scope
-        self.runToBkpt("run")
-        # Evaluate func() - should call static ns2.cpp:func()
-        # FIXME: This test fails because lldb doesn't know about file scopes so
-        # finds the global ::func().
-        self.expect_expr("func()", result_type="int", result_value="2")
-
-    @skipIfWindows # This is flakey on Windows: llvm.org/pr38373
-    def test_scope_lookup_before_using_with_run_command(self):
-        """Test scope lookup before using in lldb."""
-        self.build()
-        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
-
-        lldbutil.run_break_set_by_file_and_line(
-            self,
-            "ns3.cpp",
-            self.line_break_before_using_directive,
-            num_expected_locations=1,
-            loc_exact=False)
-
-        # Run to BP_before_using_directive at global scope before using
-        # declaration
-        self.runToBkpt("run")
-        # Evaluate func() - should call ::func()
-        self.expect_expr("func()", result_type="int", result_value="1")
-
     # NOTE: this test may fail on older systems that don't emit import
     # entries in DWARF - may need to add checks for compiler versions here.
     @skipIf(
         compiler="gcc",
         oslist=["linux"],
         debug_info=["dwo"])  # Skip to avoid crash
     def test_scope_after_using_directive_lookup_with_run_command(self):
         """Test scope lookup after using directive in lldb."""
         self.build()