Fix TestCreateAfterAttach on Windows

[amccarth]

Summary

Ported pthreads-based inferior to std::thread.

Adjusted TestCreateAfterAttach accordingly. The thread numbers with std::thread are not deterministic, so instead we check which functions the stopped threads are in.

This uncovered that stepping over a breakpoint was broken on Windows because it was expecting the thread resume state rather than the temporary resume states to change. This was fixed. (This seemed to work in the past, until my change from a couple weeks ago which stopped TargetWindowsThread::WillResume from stomping the thread resume state.)

Removed slightly misleading Windows-only logging message.

TestCreateAfterAttach now works on Windows except for some lingering POSIXisms in lldbtest.py. I'll address those in a later patch.

[chaoren]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

83

I thought these thread numbers are assigned by order of creation. When you say non-deterministic, do you mean the new thread notifications (if it exists on Windows), come in non-deterministic order? Even if the thread creations are separated by a long interval?

104

It should still show 3 threads on Windows right? Just in non-deterministic order?

Would something like '(thread #(\\n|.)+){3}' work?

[amccarth]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

83

Microsoft's std::thread implementation seems to spin up a thread pool when you create your first thread. On my machine, it immediately creates three new threads, and which if them is actually tasked to do the work seems to be non-deterministic.

104

Nope, the thread pool may contain spare threads. At times, I've seen about three more threads than the program has explicitly created. I believe one of the threads may be to manage the thread pool.

[chaoren]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

83

Ah, I see what you mean. Would it be possible to identify and hide these auxiliary threads? I can't think of a scenario in which it would be beneficial to see them. Aside from that, the regular threads do come in the correct order right? If I understood you correctly, a new thread creation could be 1) actually creating a new thread 2) assigning an existing thread from the thread pool to the task. Would it be possible to detect case 2) and treat it as a new thread creation?

[amccarth]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

83

I don't see a way to distinguish a between a thread pool thread that's waiting for work v. one that's been assigned to do work. I'm not even sure how to identify if one of them is the thread-pool manager (if there is such a thing).

[labath]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

83

I don't think we should be in the business of trying to comprehend the internal logic of an std::thread implementation detail at this point. Maybe in the future, with a toggleable setting (and it does make sense to have the setting off, for example when you are debugging std::thread itself), but right now, I think there are far too many things that can go wrong. In any case, if we start using the SB api for this test we will be far less dependent on thread numbers.

PS: Note that this means that this test does not exactly test (on windows at least) what it says it does (a thread being *created* after we attach). On windows, you merely dispatch work to an already standing-by thread, which was present before you attached. It might be worth considering adding (as a separate patch) a test which somehow forces at actual thread to be *created* after the point of attach.

93

I think this would be a good time to switch from substring matching to checking things properly using the SBAPI. Right now, it's getting quite hard to tell what this tests exactly, and if the all edge cases are treated properly (e.g. what would happen if one thread stops in "main", but some other thread has "stop reason = breakpoint"). lldbutil has a lot of wrappers to make it easy to use the python api. For example, this fragment could be written way more robustly like this:

threads = lldbutil.continue_to_breakpoint(process, 1) # run to first breakpoint
self.assertEquals(len(threads), 1)
self.assertEquals(threads[0].GetFrameAtIndex(0).GetFunctionName(), "main")

(disclaimer: I did not test this code, but I believe you get the idea)

[amccarth]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

83

I agree that we shouldn't make the debugger specific to implementation details of various libraries. That was my point.

93

I agree that should be done, but I would prefer to do it later, in a separate patch. This patch has been blocked for two weeks while I tracked down the problem with the confusion between the thread resume state and the thread temporary resume state in the Windows implementation. Further modification of the test increases the risk of breaking it for other platforms than the one I'm focused on.

There are higher priority things now. For example, because of some crashing tests, simply running ninja check-lldb hangs, leaving dozens of orphaned processes.

[chaoren]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

83

Does the visual studio debugger show the spare threads?

93

self.assertEquals(len(threads), 1)

I don't think you can make any assumptions on the number of threads because of the thread pool. But it is an important assertion to make on other platforms.

[amccarth]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

83

Yes, VS shows the spare threads.

93

At this point, even on other platforms, there are at least two threads at this point: the main thread, and the one spawned before this breakpoint.

I could do self.assertGreaterEqual(len(threads), 2), if you think that's useful.

[chaoren]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

93

I think it's useful to assert to exact number of threads, and the creation order of those threads, and we'd still have a problem even if using the SB api. Perhaps you could create a Windows version of this test case where thread numbers/order don't matter and disable this one?

[amccarth]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

93

I don't see the value in basing the test on the implementation details of either threading library (pthreads or std::thread), especially when you weigh that against the maintenance costs of having different tests for different platforms.

Note that the original test didn't even do exactly what you're asking for, since it also created a thread before attaching to the process. This change makes the test no worse. What it does is make it useful for Windows as well as the other platforms. And, in fact, it helped us find a Windows specific bug which must also be fixed and is the most important part of this patch.

If you like, I could move the creation of the first auxiliary thread until after the attach, which would allow us to assert that there's exactly one thread as you suggested, and would also address Pavel's concern that we're actually creating the thread after the attach.

[labath]

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

93

self.assertEquals(len(threads), 1)
I don't think you can make any assumptions on the number of threads because of the thread pool. But it is an important assertion to make on other platforms.
At this point, even on other platforms, there are at least two threads at this point: the main thread, and the one spawned before this breakpoint.

Perhaps it was not clear from the snippet, but continue_to_breakpoint returns only the threads that are stopped at the breakpoint, which should be exactly 1 in all cases.
This is why I am trying to push this idea. With the SBAPI, you can test *exactly* the condition you want, relying on string matching is always going to be fragile.

This patch has been blocked for two weeks while I tracked down the problem with the confusion between the thread resume state and the thread temporary resume state in the Windows implementation. Further modification of the test increases the risk of breaking it for other platforms than the one I'm focused on.

I know windows implementation still has a long way to go, and I am sorry if the review process is slowing you down. However, I feel that compared to 2 weeks fixing the bug, spending two more hours on improving the test is not bad. And since the bulk of the change is in modifying the test, I think this fits the fix-things-as-you-go-along philosophy of llvm.

However, it may have been a step to far to ask you to do that after the fact. At the time I was making it, it was not meant as a binding requirement ( I reserve the "Request changes" button for that :) ). I though of it more as a suggestion on how to resolve the issues chaoren has already raised, although it probably didn't come out that way.

That said, I think we have wasted enough time on this now, so I think you should go ahead and commit it. However, I would welcome a follow-up patch which cleans up the pattern matching magic in this test.

source/Plugins/Process/Windows/TargetThreadWindows.cpp

TargetThreadWindows::RefreshStateAfterStop()
     ::SuspendThread(m_host_thread.GetNativeThread().GetSystemHandle());
     SetState(eStateStopped);
     GetRegisterContext()->InvalidateIfNeeded(false);
 }
 
 void
 TargetThreadWindows::WillResume(lldb::StateType resume_state)
 {
-    Log *log(lldb_private::GetLogIfAllCategoriesSet (WINDOWS_LOG_THREAD));
-    if (log)
-        log->Printf ("TargetThreadWindows::WillResume (tid = %" PRIi64 ") setting thread resume state to %s",
-                     GetID(), StateAsCString(resume_state));
 }
 
 void
 TargetThreadWindows::DidStop()
 {
 }
 
 RegisterContextSP
TargetThreadWindows::GetUnwinder()
     if (m_unwinder_ap.get() == NULL)
         m_unwinder_ap.reset(new UnwindLLDB(*this));
     return m_unwinder_ap.get();
 }
 
 bool
 TargetThreadWindows::DoResume()
 {
-    StateType resume_state = GetResumeState();
+    StateType resume_state = GetTemporaryResumeState();
     StateType current_state = GetState();
     if (resume_state == current_state)
         return true;
 
     if (resume_state == eStateStepping)
     {
         uint32_t flags_index = GetRegisterContext()->ConvertRegisterKindToRegisterNumber(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FLAGS);
         uint64_t flags_value = GetRegisterContext()->ReadRegisterAsUnsigned(flags_index, 0);

test/functionalities/thread/create_after_attach/Makefile

-LEVEL = ../../../make
-
-ENABLE_THREADS := YES
-C_SOURCES := main.c
-include $(LEVEL)/Makefile.rules
+LEVEL = ../../../make
+
+CXX_SOURCES := main.cpp
+ENABLE_STD_THREADS := YES
+include $(LEVEL)/Makefile.rules

test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

def test_create_after_attach_with_dwarf_and_fork(self):
         """Test thread creation after process attach."""
         self.buildDwarf(dictionary=self.getBuildFlags(use_cpp11=False))
         self.create_after_attach(use_fork=True)
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line numbers for our breakpoints.
-        self.break_1 = line_number('main.c', '// Set first breakpoint here')
-        self.break_2 = line_number('main.c', '// Set second breakpoint here')
-        self.break_3 = line_number('main.c', '// Set third breakpoint here')
+        self.break_1 = line_number('main.cpp', '// Set first breakpoint here')
+        self.break_2 = line_number('main.cpp', '// Set second breakpoint here')
+        self.break_3 = line_number('main.cpp', '// Set third breakpoint here')
 
     def create_after_attach(self, use_fork):
         """Test thread creation after process attach."""
 
         exe = os.path.join(os.getcwd(), "a.out")
 
         # Spawn a new process
         if use_fork:
             pid = self.forkSubprocess(exe)
         else:
             popen = self.spawnSubprocess(exe)
             pid = popen.pid
         self.addTearDownHook(self.cleanupSubprocesses)
 
         # Attach to the spawned process
         self.runCmd("process attach -p " + str(pid))
 
         target = self.dbg.GetSelectedTarget()
 
         process = target.GetProcess()
         self.assertTrue(process, PROCESS_IS_VALID)
 
         # This should create a breakpoint in the main thread.
-        lldbutil.run_break_set_by_file_and_line (self, "main.c", self.break_1, num_expected_locations=1)
+        lldbutil.run_break_set_by_file_and_line (self, "main.cpp", self.break_1, num_expected_locations=1)
 
         # This should create a breakpoint in the second child thread.
-        lldbutil.run_break_set_by_file_and_line (self, "main.c", self.break_2, num_expected_locations=1)
+        lldbutil.run_break_set_by_file_and_line (self, "main.cpp", self.break_2, num_expected_locations=1)
 
         # This should create a breakpoint in the first child thread.
-        lldbutil.run_break_set_by_file_and_line (self, "main.c", self.break_3, num_expected_locations=1)
+        lldbutil.run_break_set_by_file_and_line (self, "main.cpp", self.break_3, num_expected_locations=1)
+
+        # Note:  With std::thread, we cannot rely on particular thread numbers.  Using
+        # std::thread may cause the program to spin up a thread pool (and it does on
+        # Windows), so the thread numbers are non-deterministic.
 
         # Run to the first breakpoint
         self.runCmd("continue")
 
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
             substrs = ['stopped',
-                       '* thread #1',
-                       'stop reason = breakpoint',
-                       'thread #2'])
+                       '* thread #',
+                       'main',
+                       'stop reason = breakpoint'])
 
         # Change a variable to escape the loop
         self.runCmd("expression main_thread_continue = 1")
 
         # Run to the second breakpoint
         self.runCmd("continue")
-        self.runCmd("thread select 3")
 
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
             substrs = ['stopped',
-                       'thread #1',
-                       'thread #2',
-                       '* thread #3',
+                       '* thread #',
+                       'thread_2_func',
                        'stop reason = breakpoint'])
 
         # Change a variable to escape the loop
         self.runCmd("expression child_thread_continue = 1")
 
         # Run to the third breakpoint
         self.runCmd("continue")
-        self.runCmd("thread select 2")
 
         # The stop reason of the thread should be breakpoint.
         # Thread 3 may or may not have already exited.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
             substrs = ['stopped',
-                       'thread #1',
-                       '* thread #2',
+                       '* thread #',
+                       'thread_1_func',
                        'stop reason = breakpoint'])
 
         # Run to completion
         self.runCmd("continue")
 
         # At this point, the inferior process should have exited.
         self.assertTrue(process.GetState() == lldb.eStateExited, PROCESS_EXITED)
 
 
 if __name__ == '__main__':
     import atexit
     lldb.SBDebugger.Initialize()
     atexit.register(lambda: lldb.SBDebugger.Terminate())
     unittest2.main()

test/functionalities/thread/create_after_attach/main.c

-#include <stdio.h>
-#include <unistd.h>
-#include <pthread.h>
-
-#if defined(__linux__)
-#include <sys/prctl.h>
-#endif
-
-volatile int g_thread_2_continuing = 0;
-
-void *
-thread_1_func (void *input)
-{
-    // Waiting to be released by the debugger.
-    while (!g_thread_2_continuing) // The debugger will change this value
-    {
-        usleep(1);
-    }
-
-    // Return
-    return NULL;  // Set third breakpoint here
-}
-
-void *
-thread_2_func (void *input)
-{
-    // Waiting to be released by the debugger.
-    int child_thread_continue = 0;
-    while (!child_thread_continue) // The debugger will change this value
-    {
-        usleep(1);  // Set second breakpoint here
-    }
-
-    // Release thread 1
-    g_thread_2_continuing = 1;
-
-    // Return
-    return NULL;
-}
-
-int main(int argc, char const *argv[])
-{
-    pthread_t thread_1;
-    pthread_t thread_2;
-
-#if defined(__linux__)
-    // Immediately enable any ptracer so that we can allow the stub attach
-    // operation to succeed.  Some Linux kernels are locked down so that
-    // only an ancestor process can be a ptracer of a process.  This disables that
-    // restriction.  Without it, attach-related stub tests will fail.
-#if defined(PR_SET_PTRACER) && defined(PR_SET_PTRACER_ANY)
-    int prctl_result;
-
-    // For now we execute on best effort basis.  If this fails for
-    // some reason, so be it.
-    prctl_result = prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0);
-    (void) prctl_result;
-#endif
-#endif
-
-    // Create a new thread
-    pthread_create (&thread_1, NULL, thread_1_func, NULL);
-
-    // Waiting to be attached by the debugger.
-    int main_thread_continue = 0;
-    while (!main_thread_continue) // The debugger will change this value
-    {
-        usleep(1);  // Set first breakpoint here
-    }
-
-    // Create another new thread
-    pthread_create (&thread_2, NULL, thread_2_func, NULL);
-
-    // Wait for the threads to finish.
-    pthread_join(thread_1, NULL);
-    pthread_join(thread_2, NULL);
-
-    printf("Exiting now\n");
-}

test/functionalities/thread/create_after_attach/main.cpp

+#include <stdio.h>
+#include <chrono>
+#include <thread>
+
+using std::chrono::microseconds;
+
+#if defined(__linux__)
+#include <sys/prctl.h>
+#endif
+
+volatile int g_thread_2_continuing = 0;
+
+void *
+thread_1_func (void *input)
+{
+    // Waiting to be released by the debugger.
+    while (!g_thread_2_continuing) // Another thread will change this value
+    {
+        std::this_thread::sleep_for(microseconds(1));
+    }
+
+    // Return
+    return NULL;  // Set third breakpoint here
+}
+
+void *
+thread_2_func (void *input)
+{
+    // Waiting to be released by the debugger.
+    int child_thread_continue = 0;
+    while (!child_thread_continue) // The debugger will change this value
+    {
+        std::this_thread::sleep_for(microseconds(1));  // Set second breakpoint here
+    }
+
+    // Release thread 1
+    g_thread_2_continuing = 1;
+
+    // Return
+    return NULL;
+}
+
+int main(int argc, char const *argv[])
+{
+#if defined(__linux__)
+    // Immediately enable any ptracer so that we can allow the stub attach
+    // operation to succeed.  Some Linux kernels are locked down so that
+    // only an ancestor process can be a ptracer of a process.  This disables that
+    // restriction.  Without it, attach-related stub tests will fail.
+#if defined(PR_SET_PTRACER) && defined(PR_SET_PTRACER_ANY)
+    int prctl_result;
+
+    // For now we execute on best effort basis.  If this fails for
+    // some reason, so be it.
+    prctl_result = prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0);
+    (void) prctl_result;
+#endif
+#endif
+
+    // Create a new thread
+    std::thread thread_1(thread_1_func, nullptr);
+
+    // Waiting to be attached by the debugger.
+    int main_thread_continue = 0;
+    while (!main_thread_continue) // The debugger will change this value
+    {
+        std::this_thread::sleep_for(microseconds(1));  // Set first breakpoint here
+    }
+
+    // Create another new thread
+    std::thread thread_2(thread_2_func, nullptr);
+
+    // Wait for the threads to finish.
+    thread_1.join();
+    thread_2.join();
+
+    printf("Exiting now\n");
+}