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

[Pavel Labath]

labath accepted this revision.
This revision is now accepted and ready to land.

================
Comment at: test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py:93
@@ -90,1 +92,3 @@
+                       'main',
+                       'stop reason = breakpoint'])
 
----------------
amccarth wrote:
> 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.
>`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.


http://reviews.llvm.org/D10850