[Lldb-commits] [lldb] 680082a - [lldb/Reproducers] Add a small artificial delay before exiting

[Pavel Labath via lldb-commits]

On 09/04/2020 20:51, Jonas Devlieghere via lldb-commits wrote:
> 
> 
> On Thu, Apr 9, 2020 at 11:39 AM Davide Italiano <ditaliano at apple.com
> <mailto:ditaliano at apple.com>> wrote:
> 
> 
> 
>     > On Apr 9, 2020, at 11:05, Jonas Devlieghere via lldb-commits
>     <lldb-commits at lists.llvm.org <mailto:lldb-commits at lists.llvm.org>>
>     wrote:
>     >
>     >
>     > Author: Jonas Devlieghere
>     > Date: 2020-04-09T11:03:24-07:00
>     > New Revision: 680082a408dd2df7410d77696100eac8ce9d5530
>     >
>     > URL:
>     https://github.com/llvm/llvm-project/commit/680082a408dd2df7410d77696100eac8ce9d5530
>     > DIFF:
>     https://github.com/llvm/llvm-project/commit/680082a408dd2df7410d77696100eac8ce9d5530.diff
>     >
>     > LOG: [lldb/Reproducers] Add a small artificial delay before exiting
>     >
>     > Add a small artificial delay in replay mode before exiting to ensure
>     > that all asynchronous events have completed. This should reduce the
>     > level of replay flakiness on some of the slower bots.
>     >
>     > Added:
>     >
>     >
>     > Modified:
>     >    lldb/source/Utility/ReproducerInstrumentation.cpp
>     >
>     > Removed:
>     >
>     >
>     >
>     >
>     ################################################################################
>     > diff  --git a/lldb/source/Utility/ReproducerInstrumentation.cpp
>     b/lldb/source/Utility/ReproducerInstrumentation.cpp
>     > index 4c32d9407830..3bf81286bbe9 100644
>     > --- a/lldb/source/Utility/ReproducerInstrumentation.cpp
>     > +++ b/lldb/source/Utility/ReproducerInstrumentation.cpp
>     > @@ -8,6 +8,7 @@
>     >
>     > #include "lldb/Utility/ReproducerInstrumentation.h"
>     > #include "lldb/Utility/Reproducer.h"
>     > +#include <thread>
>     > #include <stdio.h>
>     > #include <stdlib.h>
>     >
>     > @@ -94,6 +95,10 @@ bool Registry::Replay(llvm::StringRef buffer) {
>     >     GetReplayer(id)->operator()(deserializer);
>     >   }
>     >
>     > +  // Add a small artificial delay to ensure that all asynchronous
>     events have
>     > +  // completed before we exit.
>     > +  std::this_thread::sleep_for (std::chrono::milliseconds(100));
>     > +
>     >   return true;
>     > }
>     >
> 
>     I understand this is (unfortunately) sorely needed, but I’m somehow
>     skeptical of random delays.
>     You probably thought about this and dropped the idea on the floor —
>     but, is there a way to make the sync more explicit?
>     If you can’t, at least you can detect not everything is in order and
>     sleep, for, e.g. another 100 milliseconds in a retry-loop?
> 
> 
> Yeah, it's a hack, and I'm not really happy with it. The last replayed
> API call can be literaly anything, so you don't know what you might have
> to wait for. I can't think of a good way to do the synchronization
> without breaking layering/abstractions. The issue on GreenDragon was
> that we exited before the stop event came in, and while we could
> probably synchronize that, it would be even more ad-hoc than this...  
>  
> 

Wouldn't the original session suffer from the same problem? I.e., if the
thread making the last SB call exited very quickly (or the other thread
was slightly delayed), then we'd get the same crash even without
reproducers. It probably doesn't happen very often because the path
taken by real code is slower than the reproducer main loop, but it could
still happen in theory.

If so, than that would mean we either have a bug in lldb, or in the code
which uses it (lack of SBDebugger::Terminate ?). And that this code is
papering over that bug...

pl