Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb][ClangExpressionParser] Don't by default enable Objecitve-C support when evaluating C++ expressions #87767

Conversation

Michael137
Copy link
Member

@Michael137 Michael137 commented Apr 5, 2024

This patch attempts to decouple C++ expression evaluation from Objective-C support. We've previously enabled it by default (if a runtime existed), but that meant we're opting into extra work we only need to do for Objective-C, which complicates/slows down C++ expression evaluation. Of course there's a valid use-case for this, which is calling Objective-C APIs when stopped in C++ frames (which Objective-C++ developers might want to do). In those cases we should really prompt the user to add the expr --language objc++ flag. To accomodate a likely frequent use-case where a user breaks in a system C++ library (without debug-symbols) but their application is actually an Objective-C app, we allow Objective-C support in C++ expressions if the current frame doesn't have debug-info.

This fixes #75443 and allows us to add more LangOpts.ObjC guards around the expression evaluator in the future (e.g., we could avoid looking into the Objective-C runtime during C++ expression evaluation, which we currently do unconditionally).

Depends on #87657

…port when evaluating C++ expressions

This patch attempts to decouple C++ expression evaluation from
Objective-C support. We've previously enabled it by default
(if a runtime existed), but that meant we're opting into extra
work we only need to do for Objective-C, which complicates/slows down
C++ expression evaluation. Of course there's a valid use-case for this,
which is calling Objective-C APIs when stopped in C++ frames (which
Objective-C++ developers might want to do). In those case we should
really prompt the user to add the `expr --language objc++` flag.
To accomodate a likely frequent use-case where a user breaks in a system
C++ library (without debug-symbols) but their application is actually
an Objective-C app, we allow Objective-C support in C++ expressions
if the current frame doesn't have debug-info.

This fixes llvm#75443 and
allows us to add more `LangOpts.ObjC` guards around the expression
evaluator in the future (e.g., we could avoid looking into the
Objective-C runtime during C++ expression evaluation, which we
currently do unconditionally).

Depends on llvm#87657
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 5, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This patch attempts to decouple C++ expression evaluation from Objective-C support. We've previously enabled it by default (if a runtime existed), but that meant we're opting into extra work we only need to do for Objective-C, which complicates/slows down C++ expression evaluation. Of course there's a valid use-case for this, which is calling Objective-C APIs when stopped in C++ frames (which Objective-C++ developers might want to do). In those case we should really prompt the user to add the expr --language objc++ flag. To accomodate a likely frequent use-case where a user breaks in a system C++ library (without debug-symbols) but their application is actually an Objective-C app, we allow Objective-C support in C++ expressions if the current frame doesn't have debug-info.

This fixes #75443 and allows us to add more LangOpts.ObjC guards around the expression evaluator in the future (e.g., we could avoid looking into the Objective-C runtime during C++ expression evaluation, which we currently do unconditionally).

Depends on #87657


Full diff: https://github.com/llvm/llvm-project/pull/87767.diff

7 Files Affected:

  • (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+4-1)
  • (modified) lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py (+1-1)
  • (modified) lldb/test/API/commands/expression/options/TestExprOptions.py (-1)
  • (modified) lldb/test/API/lang/objcxx/objc-builtin-types/TestObjCBuiltinTypes.py (+1-1)
  • (added) lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/Makefile (+4)
  • (added) lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/TestObjCFromCppFramesWithoutDebugInfo.py (+17)
  • (added) lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/main.cpp (+1)
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 822d286cd6c3c4..f48bdc730d9160 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -526,7 +526,10 @@ ClangExpressionParser::ClangExpressionParser(
     [[fallthrough]];
   case lldb::eLanguageTypeC_plus_plus_03:
     lang_opts.CPlusPlus = true;
-    if (process_sp)
+    if (process_sp
+        // We're stopped in a frame without debug-info. The user probably
+        // intends to make global queries (which should include Objective-C).
+        && !(frame_sp && frame_sp->HasDebugInformation()))
       lang_opts.ObjC =
           process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC) != nullptr;
     break;
diff --git a/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py b/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
index d64e9897a844f1..ddc1c3598480cf 100644
--- a/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
+++ b/lldb/test/API/commands/expression/diagnostics/TestExprDiagnostics.py
@@ -172,7 +172,7 @@ def test_source_locations_from_objc_modules(self):
 
         # Import foundation so that the Obj-C module is loaded (which contains source locations
         # that can be used by LLDB).
-        self.runCmd("expr @import Foundation")
+        self.runCmd("expr --language objective-c++ -- @import Foundation")
         value = frame.EvaluateExpression("NSLog(1);")
         self.assertFalse(value.GetError().Success())
         # LLDB should print the source line that defines NSLog. To not rely on any
diff --git a/lldb/test/API/commands/expression/options/TestExprOptions.py b/lldb/test/API/commands/expression/options/TestExprOptions.py
index 6652c71083a958..01899f3b97cf44 100644
--- a/lldb/test/API/commands/expression/options/TestExprOptions.py
+++ b/lldb/test/API/commands/expression/options/TestExprOptions.py
@@ -59,7 +59,6 @@ def test_expr_options(self):
         self.assertTrue(val.IsValid())
         self.assertFalse(val.GetError().Success())
 
-    @skipIfDarwin
     def test_expr_options_lang(self):
         """These expression language options should work as expected."""
         self.build()
diff --git a/lldb/test/API/lang/objcxx/objc-builtin-types/TestObjCBuiltinTypes.py b/lldb/test/API/lang/objcxx/objc-builtin-types/TestObjCBuiltinTypes.py
index 1eb7205f1bb465..280994fbde71ab 100644
--- a/lldb/test/API/lang/objcxx/objc-builtin-types/TestObjCBuiltinTypes.py
+++ b/lldb/test/API/lang/objcxx/objc-builtin-types/TestObjCBuiltinTypes.py
@@ -53,5 +53,5 @@ def test_with_python_api(self):
         )
         self.expect(
             "expr --language C++ -- id my_id = 0; my_id",
-            patterns=["\(id\) \$.* = nullptr"],
+            error=True
         )
diff --git a/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/Makefile b/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/Makefile
new file mode 100644
index 00000000000000..7c3c32d6f82df4
--- /dev/null
+++ b/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/Makefile
@@ -0,0 +1,4 @@
+CXX_SOURCES := main.cpp
+CXXFLAGS_EXTRAS := -g0
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/TestObjCFromCppFramesWithoutDebugInfo.py b/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/TestObjCFromCppFramesWithoutDebugInfo.py
new file mode 100644
index 00000000000000..ba33d44732c924
--- /dev/null
+++ b/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/TestObjCFromCppFramesWithoutDebugInfo.py
@@ -0,0 +1,17 @@
+"""
+Test that the the expression parser enables ObjC support
+when stopped in a C++ frame without debug-info.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestObjCFromCppFramesWithoutDebugInfo(TestBase):
+    def test(self):
+        self.build()
+        (_, process, _, _) = lldbutil.run_to_name_breakpoint(self, "main")
+
+        self.assertState(process.GetState(), lldb.eStateStopped)
+        self.expect("expr id", error=False)
diff --git a/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/main.cpp b/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/main.cpp
new file mode 100644
index 00000000000000..76e8197013aabc
--- /dev/null
+++ b/lldb/test/API/lang/objcxx/objc-from-cpp-frames-without-debuginfo/main.cpp
@@ -0,0 +1 @@
+int main() { return 0; }

Copy link

github-actions bot commented Apr 5, 2024

✅ With the latest revision this PR passed the Python code formatter.

Copy link
Collaborator

@jimingham jimingham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This is a reasonable strategy to limit opting in to all the ObjC runtime work, and should be understandable by users.

@Michael137 Michael137 merged commit 38f8fce into llvm:main Apr 11, 2024
4 checks passed
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Apr 11, 2024
Michael137 added a commit that referenced this pull request Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LLDB] no member named 'id' in namespace '$__lldb_local_vars'
3 participants