[Lldb-commits] [PATCH] Fix TestCreateAfterAttach on Windows

[Adrian McCarthy]

================
Comment at: test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py:93
@@ -90,1 +92,3 @@
+                       'main',
+                       'stop reason = breakpoint'])
 
----------------
chaoren wrote:
> amccarth wrote:
> > chaoren wrote:
> > > amccarth wrote:
> > > > labath wrote:
> > > > > 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)
> > > > 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.
> > > ```
> > > 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.
> > 
> > I could do self.assertGreaterEqual(len(threads), 2), if you think that's useful.
> 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?
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.

http://reviews.llvm.org/D10850

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