[Lldb-commits] [PATCH] D88796: [lldb] Initial version of FreeBSD remote process plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks pretty straigh-forward. I'm getting a lot of deja-vu when looking at this code, though it's not clear to me whether its identical of just similar. Have you investigated the possibility of factoring some parts of this && the netbsd code (and maybe linux too) into some common place?



================
Comment at: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp:269-375
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
+  LLDB_LOG(log, "target {0}", target);
+
+  // If we're a remote host, use standard behavior from parent class.
+  if (!IsHost()) {
+    printf("pare\n");
+    return PlatformPOSIX::DebugProcess(launch_info, debugger, target, error);
----------------
I think it's time for a switcheroo -- move this code into PlatformPOSIX::DebugProcess, and move that function into PlatformDarwin.


================
Comment at: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp:274
+  if (!IsHost()) {
+    printf("pare\n");
+    return PlatformPOSIX::DebugProcess(launch_info, debugger, target, error);
----------------
?


================
Comment at: lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:82
 void ProcessFreeBSD::Initialize() {
-  static llvm::once_flag g_once_flag;
+  if (!getenv("FREEBSD_REMOTE_PLUGIN")) {
+    static llvm::once_flag g_once_flag;
----------------
I would expect that the check above is sufficient, is it not?


================
Comment at: lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp:22
+
+//#include "Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD.h"
+#include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
----------------
krytarowski wrote:
> mgorny wrote:
> > krytarowski wrote:
> > > Why this line?
> > Because otherwise it fails to compile? ;-)
> OK, you have removed the unused include.
Bonus cookies for anyone who makes it possible to remove that line.


================
Comment at: lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.cpp:9-19
+#include "NativeThreadFreeBSD.h"
+#include "NativeRegisterContextFreeBSD.h"
+
+#include "NativeProcessFreeBSD.h"
+
+#include "Plugins/Process/POSIX/CrashReason.h"
+#include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
----------------
This grouping is very random. Normal llvm style is to just put all includes in one block and let clang-format sort that out.


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

https://reviews.llvm.org/D88796