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

[Adrian McCarthy]

================
Comment at: test/functionalities/thread/create_after_attach/TestCreateAfterAttach.py:83
@@ -80,1 +82,3 @@
+        # std::thread may cause the program to spin up a thread pool (and it does on
+        # Windows), so the thread numbers are non-deterministic.
 
----------------
labath wrote:
> amccarth wrote:
> > chaoren wrote:
> > > amccarth wrote:
> > > > chaoren wrote:
> > > > > 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?
> > > > 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.
> > > 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?
> > 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).
> 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.
I agree that we shouldn't make the debugger specific to implementation details of various libraries.  That was my point.

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

http://reviews.llvm.org/D10850

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