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] Don't forcefully initialize the process trace plugin #71455

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

walter-erquinigo
Copy link
Member

@walter-erquinigo walter-erquinigo commented Nov 6, 2023

This was causing some process to wrongfully be handled by ProcessTrace.

The only place this was being used is in the intel pt plugin, but it doesn't even build anymore, so I'm sure no one is using it.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 6, 2023

@llvm/pr-subscribers-lldb

Author: Walter Erquinigo (walter-erquinigo)

Changes
  • [LLDB][easy] Fix a bug in DummySyntheticFrontEnd
  • [LLDB] Don't forcefully initialize the process trace plugin

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

3 Files Affected:

  • (modified) lldb/source/API/SystemInitializerFull.cpp (-3)
  • (modified) lldb/source/Core/ValueObjectSyntheticFilter.cpp (+1-1)
  • (modified) lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp (+5-4)
diff --git a/lldb/source/API/SystemInitializerFull.cpp b/lldb/source/API/SystemInitializerFull.cpp
index 27319debc858288..c48466f25ede81b 100644
--- a/lldb/source/API/SystemInitializerFull.cpp
+++ b/lldb/source/API/SystemInitializerFull.cpp
@@ -68,9 +68,6 @@ llvm::Error SystemInitializerFull::Initialize() {
 #define LLDB_PLUGIN(p) LLDB_PLUGIN_INITIALIZE(p);
 #include "Plugins/Plugins.def"
 
-  // Initialize plug-ins in core LLDB
-  ProcessTrace::Initialize();
-
   // Scan for any system or user LLDB plug-ins
   PluginManager::Initialize();
 
diff --git a/lldb/source/Core/ValueObjectSyntheticFilter.cpp b/lldb/source/Core/ValueObjectSyntheticFilter.cpp
index 59ed780b654f3af..43bc532c4a0410b 100644
--- a/lldb/source/Core/ValueObjectSyntheticFilter.cpp
+++ b/lldb/source/Core/ValueObjectSyntheticFilter.cpp
@@ -41,7 +41,7 @@ class DummySyntheticFrontEnd : public SyntheticChildrenFrontEnd {
     return m_backend.GetIndexOfChildWithName(name);
   }
 
-  bool MightHaveChildren() override { return true; }
+  bool MightHaveChildren() override { return m_backend.MightHaveChildren(); }
 
   bool Update() override { return false; }
 };
diff --git a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
index 6bed78fd83f0b70..bd9cca675f2d747 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceIntelPTBundleLoader.cpp
@@ -103,10 +103,11 @@ TraceIntelPTBundleLoader::CreateEmptyProcess(lldb::pid_t pid,
   ParsedProcess parsed_process;
   parsed_process.target_sp = target_sp;
 
-  ProcessSP process_sp = target_sp->CreateProcess(
-      /*listener*/ nullptr, "trace",
-      /*crash_file*/ nullptr,
-      /*can_connect*/ false);
+  // This should instead try to directly create an instance of ProcessTrace.
+  // ProcessSP process_sp = target_sp->CreateProcess(
+  //    /*listener*/ nullptr, "trace",
+  //    /*crash_file*/ nullptr,
+  //    /*can_connect*/ false);
 
   process_sp->SetID(static_cast<lldb::pid_t>(pid));
 

This was causing some process to wrongfully be handled by ProcessTrace.

The only place this was being used is in the intel pt plugin, but it doesn't even build anymore, so I'm sure no one is using it.
@walter-erquinigo walter-erquinigo changed the title walter/fix [LLDB] Don't forcefully initialize the process trace plugin Nov 6, 2023
Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

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

There is no LLDB_PLUGIN_DEFINE() macro in ProcessTrace.cpp, so that is why it was being manually initialized. But this plugin in a problem in that if it's ProcessTrace::CreateInstance(...) is called, it will always return a valid Process plug-in which is bad as it isn't filtering on an architecture, os, or other features of the target.

@walter-erquinigo walter-erquinigo merged commit 555a71b into llvm:main Nov 7, 2023
3 checks passed
@walter-erquinigo walter-erquinigo deleted the walter/fix branch November 7, 2023 00:45
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.

4 participants