[Lldb-commits] [PATCH] Local Linux debugging via LLGS passing all Linux local debugging tests.

[Tamas Berghammer]

Most of my comments can be addressed later (mostly clarifications) if we want this patch upstream as soon as possible but fixing the two type (chcar -> char) and the two wrong return value (return pointer to local object) should be addressed now as they can cause some serious issues.


================
Comment at: scripts/Python/remote-build.py:28-29
@@ +27,4 @@
+def parse_args():
+    DEFAULT_REMOTE_ROOT_DIR = "/mnt/ssd/work/macosx.sync"
+    DEFAULT_REMOTE_HOSTNAME = "tfiala2.mtv.corp.google.com"
+    OPTIONS_FILENAME = ".remote-build.conf"
----------------
Can we replace it with some more generic defaults or make them mandatory?

================
Comment at: source/Host/common/NativeWatchpointList.cpp:20
@@ +19,3 @@
+{
+    m_watchpoints[addr] = {addr, size, watch_flags, hardware};
+    return Error ();
----------------
What is the expected behavior if there is already a watchpoint on the given address? Is it good to simply overwrite it?

================
Comment at: source/Plugins/Platform/Linux/PlatformLinux.cpp:764-774
@@ -763,11 +763,13 @@
     ListenerSP listener_sp;
+#if 0
     if (!launch_info.GetHijackListener ())
     {
         if (log)
             log->Printf ("PlatformLinux::%s setting up hijacker", __FUNCTION__);
 
         listener_sp.reset (new Listener("lldb.PlatformLinux.DebugProcess.hijack"));
         launch_info.SetHijackListener (listener_sp);
         process_sp->HijackProcessEvents (listener_sp.get ());
     }
+#endif
 
----------------
Can we remove it completely or add a comment why is it skipped for now?

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2429-2433
@@ +2428,7 @@
+            m_coordinator_up->RequestThreadResume (pid,
+                                                   [=](lldb::tid_t tid_to_resume, bool supress_signal)
+                                                   {
+                                                       reinterpret_cast<NativeThreadLinux*> (thread_sp.get ())->SetRunning ();
+                                                       return Resume (tid_to_resume, LLDB_INVALID_SIGNAL_NUMBER);
+                                                   },
+                                                   CoordinatorErrorHandler);
----------------
Can we factor out this lambda as it is used several times? (Also do it for the other lambdas in this file)

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2577
@@ -2711,4 +2576,3 @@
 
-        const ResumeAction *const action = resume_actions.GetActionForThread (thread_sp->GetID (), true);
-        assert (action && "NULL ResumeAction returned for thread during Resume ()");
+    // std::vector<NativeThreadProtocolSP> new_stop_threads;
 
----------------
Please remove as not used anymore

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2739-2741
@@ +2738,5 @@
+        {
+            // The thread shouldn't be null but lets just cover that here.
+            if (!thread_sp)
+                continue;
+
----------------
Can we add an assert instead of a fall through for nullptr-s?

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3488
@@ -3568,3 +3487,3 @@
     if (log)
-        log->Printf ("NativeProcessLinux::%s() resuming result = %s", __FUNCTION__, result ? "true" : "false");
-    return result;
+        log->Printf ("NativeProcessLinux::%s() resuming thread = %"  PRIu64 " result = %s", __FUNCTION__, tid, op.GetError().Success() ? "true" : "false");
+    return op.GetError();
----------------
Please include the error message in the log if possible (instead of just true/false)

================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3600-3623
@@ +3599,26 @@
+void *
+NativeProcessLinux::CoordinatorThread (void *arg)
+{
+    Log *const log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));
+
+    NativeProcessLinux *const process = static_cast<NativeProcessLinux*> (arg);
+    assert (process && "null process passed to CoordinatorThread");
+    if (!process)
+    {
+        if (log)
+            log->Printf ("NativeProcessLinux::%s null process, exiting ThreadStateCoordinator processing loop", __FUNCTION__);
+        return nullptr;
+    }
+
+    // Run the thread state coordinator loop until it is done.  This call uses
+    // efficient waiting for an event to be ready.
+    while (process->m_coordinator_up->ProcessNextEvent () == ThreadStateCoordinator::eventLoopResultContinue)
+    {
+    }
+
+    if (log)
+        log->Printf ("NativeProcessLinux::%s pid %" PRIu64 " exiting ThreadStateCoordinator processing loop due to coordinator indicating completion", __FUNCTION__, process->GetID ());
+
+    return nullptr;
+}
+
----------------
What is the return value of this function? Can we remove it as it is always nullptr and can't be overridden as it is a static method?

================
Comment at: source/Plugins/Process/POSIX/CrashReason.cpp:211
@@ +210,3 @@
+    // Just return the code in ascii for integration builds.
+    chcar str[8];
+    sprintf(str, "%d", reason);
----------------
Typo: chcar -> char

================
Comment at: source/Plugins/Process/POSIX/CrashReason.cpp:211
@@ +210,3 @@
+    // Just return the code in ascii for integration builds.
+    chcar str[8];
+    sprintf(str, "%d", reason);
----------------
tberghammer wrote:
> Typo: chcar -> char
Returning a pointer to a local, stack based object!

================
Comment at: source/Plugins/Process/POSIX/ProcessMessage.cpp:25-26
@@ -211,3 +24,4 @@
+    // Just return the code in ascii for integration builds.
     chcar str[8];
     sprintf(str, "%d", reason);
 #else
----------------
Typo: chcar -> char
Returning pointer to stack based object!

================
Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:3770
@@ -3752,1 +3769,3 @@
+            want_hardware = true;  want_breakpoint = false; break;
+        case eStoppointInvalid:
             return SendIllFormedResponse(packet, "Z packet had invalid software/hardware specifier");
----------------
Please add handle for default case

http://reviews.llvm.org/D7238

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/