[lldb-dev] Testing through api vs. commands

[Todd Fiala via lldb-dev]

> I didn't follow those threads, can you summarize what the race condition
was?  Is it likely to crop up in the normal test suite?  i.e. not the test
suite that tests the test suite.

Sure.  At least on OS X and Linux, the impl of subprocess.Popen.wait() and
subprocess.Popen.poll() are written such that they are not thread safe.
And subprocess.Popen.communicate() calls wait() internally.  In order to
implement a timeout in Python, we essentially need to run the communicate()
part on a separate thread, and have another thread timing out on that first
one not finishing up within the timeout.  Two things can potentially touch
a wait() or poll() call.  If and when they race, the actual
subprocess.Popen.returncode (the return value of the executed program) can
get mis-set.  i.e. the return value of the called program can come back
incorrectly.  This is catastrophically wrong in the common case of the
race, which is that it gets written 0 when in fact there was an error in
the underlying process (e.g. signal/exceptional process exit).

It was bugged here:
https://llvm.org/bugs/show_bug.cgi?id=25019

and fixed here:
r249182

-Todd

On Mon, Oct 5, 2015 at 12:31 PM, Zachary Turner <zturner at google.com> wrote:

> I didn't follow those threads, can you summarize what the race condition
> was?  Is it likely to crop up in the normal test suite?  i.e. not the test
> suite that tests the test suite.
>
> On Mon, Oct 5, 2015 at 12:30 PM Todd Fiala <todd.fiala at gmail.com> wrote:
>
>> IMHO that all sounds reasonable.
>>
>> FWIW - I wrote some tests for the test system changes I put in (for the
>> pure-python impl of timeout support), and in the process, I discovered a
>> race condition in using a python facility that there really is no way I
>> would have found anywhere near as reasonably without having added the
>> tests.  (For those of you who are test-centric, this is not a surprising
>> outcome, but I'm adding this for those who may be inclined to think of it
>> as an afterthought).
>>
>> -Todd
>>
>> On Mon, Oct 5, 2015 at 11:24 AM, Zachary Turner via lldb-dev <
>> lldb-dev at lists.llvm.org> wrote:
>>
>>> On Fri, Sep 11, 2015 at 11:42 AM Jim Ingham <jingham at apple.com> wrote:
>>>
>>>> I have held from the beginning that the only tests that should be
>>>> written using HandleCommand are those that explicitly test command
>>>> behavior, and if it is possible to write a test using the SB API you should
>>>> always do it that way for the very reasons you cite.  Not everybody agreed
>>>> with me at first, so we ended up with a bunch of tests that do complex
>>>> things using HandleCommand where they really ought not to.  I'm not sure it
>>>> is worth the time to go rewrite all those tests, but we shouldn't write any
>>>> new tests that way.
>>>>
>>>
>>> I would like to revive this thread, because there doesn't seem to be
>>> consensus that this is the way to go.  I've suggested on a couple of
>>> reviews recently that people put new command api tests under a new
>>> top-level folder under tests, and so far the responses I've gotten have not
>>> indicated that people are willing to do this.
>>>
>>> Nobody chimed in on this thread with a disagreement, which indicates to
>>> me that we are ok with moving this forward.  So I'm reviving this in hopes
>>> that we can come to agreement.  With that in mind, my goal is:
>>>
>>> 1) Begin enforcing this on new CLs that go in.  We need to maintain a
>>> consistent message and direction for the project, and if this is a "good
>>> idea", then it should be applied and enforced consistently.  Command api
>>> tests should be the exception, not the norm.
>>>
>>> 2) Begin rejecting or reverting changes that go in without tests.  I
>>> understand there are some situations where tests are difficult.  Core dumps
>>> and unwinding come to mind.  There are probably others.  But this is the
>>> exception, and not the norm.  Almost every change should go in with tests.
>>>
>>> 3) If a CL cannot be tested without a command api test due to
>>> limitations of the SB API, require new changes to go in *with a
>>> corresponding SB API change*.  I know that people just want to get
>>> their stuff done, but I dont' feel is an excuse for having a subpar testing
>>> situation.  For the record, I'm not singling anyone out.  Everyone is
>>> guilty, including me.  I'm offering to do my part, and I would like to be
>>> able to enforce this at the project level.  As with #2, there are times
>>> when an SB API isn't appropriate or doesn't make sense.  We can figure that
>>> out when we come to it.  But I believe a large majority of these command
>>> api tests go in the way they do because there is no corresponding SB API
>>> *yet*.  And I think the change should not go in without designing the
>>> appropriate SB API at the same time.
>>>
>>> Zach
>>>
>>> _______________________________________________
>>> lldb-dev mailing list
>>> lldb-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>>>
>>>
>>
>>
>> --
>> -Todd
>>
>


-- 
-Todd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20151005/b33f0837/attachment-0001.html>