[Lldb-commits] [PATCH] D148400: [lldb] Fix bug to update process public run lock with process state

[Alex Langford via Phabricator via lldb-commits]

bulbazord requested changes to this revision.
bulbazord added a comment.
This revision now requires changes to proceed.

Idea is good, few concerns about the implementation.



================
Comment at: lldb/source/Target/Process.cpp:404-419
+llvm::StringRef Process::GetAttachSynchronousHijackListenerName() {
+  static ConstString class_name(
+      "lldb.internal.Process.AttachSynchronous.hijack");
+  return class_name.GetCString();
+}
+
+llvm::StringRef Process::GetLaunchSynchronousHijackListenerName() {
----------------
Do these need to be ConstStrings? They will live in the string pool forever, and it looks like in the code below you're just manipulating `const char *` anyway.

Could be something like:
```
llvm::StringRef Process::GetAttachSynchronousHijackListenerName() {
  return "lldb.internal.Process.AttachSynchronous.hijack";
}
```


================
Comment at: lldb/source/Target/Process.cpp:1427
     if (hijacking_name &&
-        strcmp(hijacking_name, g_resume_sync_name))
+        strstr(hijacking_name, "lldb.internal") != hijacking_name)
       return true;
----------------
Instead of using the `strstr` function directly, I would at least do `strnstr` to ensure that we're not touching memory we don't have. Especially if we could get `hijacking_name` to be shorter than `"lldb.internal"` somehow...

We could also change this to use StringRefs and use the `starts_with`/`startswith` methods instead.


================
Comment at: lldb/source/Target/Process.cpp:1437-1438
     if (hijacking_name &&
-        strcmp(hijacking_name, g_resume_sync_name) == 0)
+        strcmp(hijacking_name,
+               GetResumeSynchronousHijackListenerName().data()) == 0)
       return true;
----------------
First, I would probably avoid `strcmp` directly and use `strncmp` instead. We know the length of the HijackListener name, that would be a good choice for `n` here.

But you could probably simplify this even further. You can compare `GetResumeSynchronousHijackListenerName` to `hijacking_name` with StringRef's operator==. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148400