[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG482a0ad5ba72: [lldb] Improve logging for process state 
change (NFC) (authored by mib).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148399/new/

https://reviews.llvm.org/D148399

Files:
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -1055,14 +1055,16 @@
   std::lock_guard guard(m_exit_status_mutex);
 
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(
-  log, "Process::SetExitStatus (status=%i (0x%8.8x), description=%s%s%s)",
-  status, status, cstr ? "\"" : "", cstr ? cstr : "NULL", cstr ? "\"" : "");
+  LLDB_LOG(log, "(plugin = %s status=%i (0x%8.8x), description=%s%s%s)",
+   GetPluginName().data(), status, status, cstr ? "\"" : "",
+   cstr ? cstr : "NULL", cstr ? "\"" : "");
 
   // We were already in the exited state
   if (m_private_state.GetValue() == eStateExited) {
-LLDB_LOGF(log, "Process::SetExitStatus () ignoring exit status because "
-   "state was already set to eStateExited");
+LLDB_LOG(log,
+ "(plugin = %s) ignoring exit status because state was already set "
+ "to eStateExited",
+ GetPluginName().data());
 return false;
   }
 
@@ -1314,8 +1316,8 @@
   }
 
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(log, "Process::SetPublicState (state = %s, restarted = %i)",
-StateAsCString(new_state), restarted);
+  LLDB_LOG(log, "(plugin = %s, state = %s, restarted = %i)",
+   GetPluginName().data(), StateAsCString(new_state), restarted);
   const StateType old_state = m_public_state.GetValue();
   m_public_state.SetValue(new_state);
 
@@ -1324,16 +1326,16 @@
   // program to run.
   if (!StateChangedIsExternallyHijacked()) {
 if (new_state == eStateDetached) {
-  LLDB_LOGF(log,
-"Process::SetPublicState (%s) -- unlocking run lock for detach",
-StateAsCString(new_state));
+  LLDB_LOG(log,
+   "(plugin = %s, state = %s) -- unlocking run lock for detach",
+   GetPluginName().data(), StateAsCString(new_state));
   m_public_run_lock.SetStopped();
 } else {
   const bool old_state_is_stopped = StateIsStoppedState(old_state, false);
   if ((old_state_is_stopped != new_state_is_stopped)) {
 if (new_state_is_stopped && !restarted) {
-  LLDB_LOGF(log, "Process::SetPublicState (%s) -- unlocking run lock",
-StateAsCString(new_state));
+  LLDB_LOG(log, "(plugin = %s, state = %s) -- unlocking run lock",
+   GetPluginName().data(), StateAsCString(new_state));
   m_public_run_lock.SetStopped();
 }
   }
@@ -1343,10 +1345,11 @@
 
 Status Process::Resume() {
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(log, "Process::Resume -- locking run lock");
+  LLDB_LOG(log, "(plugin = %s) -- locking run lock", GetPluginName().data());
   if (!m_public_run_lock.TrySetRunning()) {
 Status error("Resume request failed - process still running.");
-LLDB_LOGF(log, "Process::Resume: -- TrySetRunning failed, not resuming.");
+LLDB_LOG(log, "(plugin = %s) -- TrySetRunning failed, not resuming.",
+ GetPluginName().data());
 return error;
   }
   Status error = PrivateResume();
@@ -1419,7 +1422,8 @@
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process | LLDBLog::Unwind));
   bool state_changed = false;
 
-  LLDB_LOGF(log, "Process::SetPrivateState (%s)", StateAsCString(new_state));
+  LLDB_LOG(log, "(plugin = %s, state = %s)", GetPluginName().data(),
+   StateAsCString(new_state));
 
   std::lock_guard thread_guard(m_thread_list.GetMutex());
   std::lock_guard guard(m_private_state.GetMutex());
@@ -1460,15 +1464,15 @@
   if (!m_mod_id.IsLastResumeForUserExpression())
 m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
   m_memory_cache.Clear();
-  LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
-StateAsCString(new_state), m_mod_id.GetStopID());
+  LLDB_LOG(log, "(plugin = %s, state = %s, stop_id = %u",
+   GetPluginName().data(), StateAsCString(new_state),
+   m_mod_id.GetStopID());
 }
 
 m_private_state_broadcaster.BroadcastEvent(event_sp);
   } else {
-LLDB_LOGF(log,
-  "Process::SetPrivateState (%s) state didn't change. Ignoring...",
-  StateAsCString(new_state));
+LLDB_LOG(log, "(plugin = %s, state = %s) state didn't change. Ignoring...",
+ GetPluginName().data(), StateAsCString(new_state));
   }
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailma

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.
This revision is now accepted and ready to land.

LGTM


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148399/new/

https://reviews.llvm.org/D148399

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 515473.
mib edited the summary of this revision.
mib added a comment.

Address @JDevlieghere @bulbazord comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148399/new/

https://reviews.llvm.org/D148399

Files:
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -1053,14 +1053,16 @@
   std::lock_guard guard(m_exit_status_mutex);
 
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(
-  log, "Process::SetExitStatus (status=%i (0x%8.8x), description=%s%s%s)",
-  status, status, cstr ? "\"" : "", cstr ? cstr : "NULL", cstr ? "\"" : "");
+  LLDB_LOG(log, "(plugin = %s status=%i (0x%8.8x), description=%s%s%s)",
+   GetPluginName().data(), status, status, cstr ? "\"" : "",
+   cstr ? cstr : "NULL", cstr ? "\"" : "");
 
   // We were already in the exited state
   if (m_private_state.GetValue() == eStateExited) {
-LLDB_LOGF(log, "Process::SetExitStatus () ignoring exit status because "
-   "state was already set to eStateExited");
+LLDB_LOG(log,
+ "(plugin = %s) ignoring exit status because state was already set "
+ "to eStateExited",
+ GetPluginName().data());
 return false;
   }
 
@@ -1312,8 +1314,8 @@
   }
 
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(log, "Process::SetPublicState (state = %s, restarted = %i)",
-StateAsCString(new_state), restarted);
+  LLDB_LOG(log, "(plugin = %s, state = %s, restarted = %i)",
+   GetPluginName().data(), StateAsCString(new_state), restarted);
   const StateType old_state = m_public_state.GetValue();
   m_public_state.SetValue(new_state);
 
@@ -1322,16 +1324,16 @@
   // program to run.
   if (!StateChangedIsExternallyHijacked()) {
 if (new_state == eStateDetached) {
-  LLDB_LOGF(log,
-"Process::SetPublicState (%s) -- unlocking run lock for detach",
-StateAsCString(new_state));
+  LLDB_LOG(log,
+   "(plugin = %s, state = %s) -- unlocking run lock for detach",
+   GetPluginName().data(), StateAsCString(new_state));
   m_public_run_lock.SetStopped();
 } else {
   const bool old_state_is_stopped = StateIsStoppedState(old_state, false);
   if ((old_state_is_stopped != new_state_is_stopped)) {
 if (new_state_is_stopped && !restarted) {
-  LLDB_LOGF(log, "Process::SetPublicState (%s) -- unlocking run lock",
-StateAsCString(new_state));
+  LLDB_LOG(log, "(plugin = %s, state = %s) -- unlocking run lock",
+   GetPluginName().data(), StateAsCString(new_state));
   m_public_run_lock.SetStopped();
 }
   }
@@ -1341,10 +1343,11 @@
 
 Status Process::Resume() {
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(log, "Process::Resume -- locking run lock");
+  LLDB_LOG(log, "(plugin = %s) -- locking run lock", GetPluginName().data());
   if (!m_public_run_lock.TrySetRunning()) {
 Status error("Resume request failed - process still running.");
-LLDB_LOGF(log, "Process::Resume: -- TrySetRunning failed, not resuming.");
+LLDB_LOG(log, "(plugin = %s) -- TrySetRunning failed, not resuming.",
+ GetPluginName().data());
 return error;
   }
   Status error = PrivateResume();
@@ -1416,7 +1419,8 @@
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process | LLDBLog::Unwind));
   bool state_changed = false;
 
-  LLDB_LOGF(log, "Process::SetPrivateState (%s)", StateAsCString(new_state));
+  LLDB_LOG(log, "(plugin = %s, state = %s)", GetPluginName().data(),
+   StateAsCString(new_state));
 
   std::lock_guard thread_guard(m_thread_list.GetMutex());
   std::lock_guard guard(m_private_state.GetMutex());
@@ -1457,15 +1461,15 @@
   if (!m_mod_id.IsLastResumeForUserExpression())
 m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
   m_memory_cache.Clear();
-  LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
-StateAsCString(new_state), m_mod_id.GetStopID());
+  LLDB_LOG(log, "(plugin = %s, state = %s, stop_id = %u",
+   GetPluginName().data(), StateAsCString(new_state),
+   m_mod_id.GetStopID());
 }
 
 m_private_state_broadcaster.BroadcastEvent(event_sp);
   } else {
-LLDB_LOGF(log,
-  "Process::SetPrivateState (%s) state didn't change. Ignoring...",
-  StateAsCString(new_state));
+LLDB_LOG(log, "(plugin = %s, state = %s) state didn't change. Ignoring...",
+ GetPluginName().data(), StateAsCString(new_state));
   }
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D148399#4284809 , @bulbazord wrote:

> We'd go from:
>
>   LLDB_LOGF(log,
> "Process::SetPrivateState (plugin = %s, state = %s) state didn't "
> "change. Ignoring...",
> GetPluginName().data(), StateAsCString(new_state));
>
> to
>
>   LLDB_LOG(log, "(plugin = %s, state = %s) state didn't "
> "change. Ignoring...",
> GetPluginName().data(), StateAsCString(new_state));
>
> `LLDB_LOG` automatically inserts the file and func into the log. There are 
> plenty of places where we copy/paste logs like these into other files without 
> remembering to update this piece of information, so removing it from the log 
> line entirely helps prevent those kinds of issues in the future. It's a minor 
> thing, but if the point of this patch is to improve the logging mechanism 
> entirely, then I think we should also think about these kinds of things. See 
> 0a74fbb7b1d3e04ac03389f1fc455ac593c2e5ee 
>  for an 
> example of what I mean.

I didn't understand that originally (since what you guys are suggesting doesn't 
have anything to do with my change), but I'll update it in this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148399/new/

https://reviews.llvm.org/D148399

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

We'd go from:

  LLDB_LOGF(log,
"Process::SetPrivateState (plugin = %s, state = %s) state didn't "
"change. Ignoring...",
GetPluginName().data(), StateAsCString(new_state));

to

  LLDB_LOG(log, "(plugin = %s, state = %s) state didn't "
"change. Ignoring...",
GetPluginName().data(), StateAsCString(new_state));

`LLDB_LOG` automatically inserts the file and func into the log. There are 
plenty of places where we copy/paste logs like these into other files without 
remembering to update this piece of information, so removing it from the log 
line entirely helps prevent those kinds of issues in the future. It's a minor 
thing, but if the point of this patch is to improve the logging mechanism 
entirely, then I think we should also think about these kinds of things. See 
0a74fbb7b1d3e04ac03389f1fc455ac593c2e5ee 
 for an 
example of what I mean.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148399/new/

https://reviews.llvm.org/D148399

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D148399#4274470 , @JDevlieghere 
wrote:

> I think a lot of this can be simplified by using `LLDB_LOG` instead of 
> `LLDB_LOGF`.



In D148399#4275219 , @bulbazord wrote:

> +1 to what Jonas said. `LLDB_LOG` would greatly simplify this since it puts 
> `__FILE__` and `__func__` in each message, which is what these are doing 
> manually.

@bulbazord @JDevlieghere I don't think using using `LLDB_LOG` instead of 
`LLDB_LOGF` would be of any use here, because we're interested because that 
will just say we're in `Process::func`. What we care about here is to know 
which process plugin calling that function, so I still have to pass 
`GetPluginName` as an argument.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148399/new/

https://reviews.llvm.org/D148399

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

+1 to what Jonas said. `LLDB_LOG` would greatly simplify this since it puts 
`__FILE__` and `__func__` in each message, which is what these are doing 
manually.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148399/new/

https://reviews.llvm.org/D148399

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

I think a lot of this can be simplified by using `LLDB_LOG` instead of 
`LLDB_LOGF`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148399/new/

https://reviews.llvm.org/D148399

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D148399: [lldb] Improve logging for process state change (NFC)

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: bulbazord, jingham, JDevlieghere.
mib added a project: LLDB.
Herald added a project: All.
mib requested review of this revision.
Herald added a subscriber: lldb-commits.

This patch improves process state change logging messages to include to
process plugin name.

This is very useful when investigating interactions between different
types of process plugins. That comes very handy when investigating bugs
related to interactive scripted process debugging.

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148399

Files:
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -1053,14 +1053,19 @@
   std::lock_guard guard(m_exit_status_mutex);
 
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(
-  log, "Process::SetExitStatus (status=%i (0x%8.8x), description=%s%s%s)",
-  status, status, cstr ? "\"" : "", cstr ? cstr : "NULL", cstr ? "\"" : "");
+  LLDB_LOGF(log,
+"Process::SetExitStatus (plugin = %s status=%i (0x%8.8x), "
+"description=%s%s%s)",
+GetPluginName().data(), status, status, cstr ? "\"" : "",
+cstr ? cstr : "NULL", cstr ? "\"" : "");
 
   // We were already in the exited state
   if (m_private_state.GetValue() == eStateExited) {
-LLDB_LOGF(log, "Process::SetExitStatus () ignoring exit status because "
-   "state was already set to eStateExited");
+LLDB_LOGF(
+log,
+"Process::SetExitStatus (plugin = %s) ignoring exit status because "
+"state was already set to eStateExited",
+GetPluginName().data());
 return false;
   }
 
@@ -1312,8 +1317,9 @@
   }
 
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(log, "Process::SetPublicState (state = %s, restarted = %i)",
-StateAsCString(new_state), restarted);
+  LLDB_LOGF(log,
+"Process::SetPublicState (plugin = %s, state = %s, restarted = %i)",
+GetPluginName().data(), StateAsCString(new_state), restarted);
   const StateType old_state = m_public_state.GetValue();
   m_public_state.SetValue(new_state);
 
@@ -1323,15 +1329,18 @@
   if (!StateChangedIsExternallyHijacked()) {
 if (new_state == eStateDetached) {
   LLDB_LOGF(log,
-"Process::SetPublicState (%s) -- unlocking run lock for detach",
-StateAsCString(new_state));
+"Process::SetPublicState (plugin = %s, state = %s) -- "
+"unlocking run lock for detach",
+GetPluginName().data(), StateAsCString(new_state));
   m_public_run_lock.SetStopped();
 } else {
   const bool old_state_is_stopped = StateIsStoppedState(old_state, false);
   if ((old_state_is_stopped != new_state_is_stopped)) {
 if (new_state_is_stopped && !restarted) {
-  LLDB_LOGF(log, "Process::SetPublicState (%s) -- unlocking run lock",
-StateAsCString(new_state));
+  LLDB_LOGF(log,
+"Process::SetPublicState (plugin = %s, state = %s) -- "
+"unlocking run lock",
+GetPluginName().data(), StateAsCString(new_state));
   m_public_run_lock.SetStopped();
 }
   }
@@ -1341,10 +1350,14 @@
 
 Status Process::Resume() {
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
-  LLDB_LOGF(log, "Process::Resume -- locking run lock");
+  LLDB_LOGF(log, "Process::Resume (plugin = %s) -- locking run lock",
+GetPluginName().data());
   if (!m_public_run_lock.TrySetRunning()) {
 Status error("Resume request failed - process still running.");
-LLDB_LOGF(log, "Process::Resume: -- TrySetRunning failed, not resuming.");
+LLDB_LOGF(
+log,
+"Process::Resume (plugin = %s) -- TrySetRunning failed, not resuming.",
+GetPluginName().data());
 return error;
   }
   Status error = PrivateResume();
@@ -1420,7 +1433,8 @@
   Log *log(GetLog(LLDBLog::State | LLDBLog::Process | LLDBLog::Unwind));
   bool state_changed = false;
 
-  LLDB_LOGF(log, "Process::SetPrivateState (%s)", StateAsCString(new_state));
+  LLDB_LOGF(log, "Process::SetPrivateState (plugin = %s, state = %s)",
+GetPluginName().data(), StateAsCString(new_state));
 
   std::lock_guard thread_guard(m_thread_list.GetMutex());
   std::lock_guard guard(m_private_state.GetMutex());
@@ -1461,15 +1475,19 @@
   if (!m_mod_id.IsLastResumeForUserExpression())
 m_mod_id.SetStopEventForLastNaturalStopID(event_sp);
   m_memory_cache.Clear();
-  LLDB_LOGF(log, "Process::SetPrivateState (%s) stop_id = %u",
-StateAsCString(new_state), m_mod_id.GetStopID());
+  LLDB_LOGF(
+  log,
+  "Process::SetPrivateState (plugin = %s, state = %s, stop_id = %u",
+