Page MenuHomePhabricator

Fix TestProcessLaunch.test_environment_with_special_char for Python 3
AbandonedPublic

Authored by zturner on Jan 12 2016, 4:29 PM.

Details

Reviewers
amccarth
labath
Summary
There were a number of problems preventing this from working:

1. The SWIG typemaps for converting Python lists to and from C++
   arrays were not updated for Python 3.  So they were doing things
   like PyString_Check instead of using the PythonString from
   PythonDataObjects.
2. ProcessLauncherWindows was ignoring the environment completely.
   So any test that involved launching an inferior with any kind
   of environment variable would have failed.
3. The test itself was using process.GetSTDOUT(), which isn't
   implemented on Windows.  So this was changed to redirect
   stdout to a file, and then the test reads the file.

Diff Detail

Event Timeline

zturner updated this revision to Diff 44694.Jan 12 2016, 4:29 PM
zturner retitled this revision from to Fix TestProcessLaunch.test_environment_with_special_char for Python 3.
zturner updated this object.
zturner added a reviewer: amccarth.
zturner added a subscriber: lldb-commits.
amccarth accepted this revision.Jan 12 2016, 4:53 PM
amccarth edited edge metadata.

I don't know enough about SWIG to give that a thorough review.

source/Host/windows/ProcessLauncherWindows.cpp
38

I would explicitly set

*cur_entry = '\0';

after the loop to make it clear that this is a null-terminated block of null-terminated strings.

I get that buffer.resize(bytes) is going to zero out the bytes initially, _if_ it grows, so it's not technically necessary in the current use case. But if a future caller calls it with a junk buffer that's already long enough (or longer), then you could end up with a non-zero in that last byte.

This revision is now accepted and ready to land.Jan 12 2016, 4:53 PM
labath requested changes to this revision.Jan 13 2016, 2:24 AM
labath added a reviewer: labath.
labath added a subscriber: labath.

This will make the test fail on remote platforms, as the redirected file needs to be transferred in before you can check it (see TestQuoting for an example).

It sounds like we should come up with some guidelines wrt. using stdio in tests. I would like to use it as it is much simpler than using files, but I guess supporting that for windows, where you launch the inferior in a separate console window (which i very like btw) could be difficult (?). If we get rid of it, we might even kill two birds with one stone, as we have some stdio issues on linux as well (https://llvm.org/bugs/show_bug.cgi?id=25652). My favourite replacement for that would be inferior sticking whatever output in needs in a char[] variable and debugger reading it from there. This should hopefully work in all scenarios, as it does not depend on any external concepts.

Thoughts ?

This revision now requires changes to proceed.Jan 13 2016, 2:24 AM

Yea, supporting GetStdio is really difficult on Windows. We might try to do it again someday. Putting it in a char[] variable and reading the variable seems like a decent solution, I will try that.

There's lots of tests currently that redirect stdio, are those all broken on Linux currently?

Yea, supporting GetStdio is really difficult on Windows. We might try to do it again someday. Putting it in a char[] variable and reading the variable seems like a decent solution, I will try that.

Thanks.

There's lots of tests currently that redirect stdio, are those all broken on Linux currently?

The redirection works on linux, it's just not 100% reliable in tests. The thing is that inferior stdio arrives asynchronously wrt. everything else, so you really cannot be sure that by the time you do GetSTDOUT(), all output will be available. It works most of the time, but it fails in like ~0.1% of cases -- not much, but enough to be annoying when scrutinizing buildbot failures.

There's lots of tests currently that redirect stdio, are those all broken on Linux currently?

The redirection works on linux, it's just not 100% reliable in tests. The thing is that inferior stdio arrives asynchronously wrt. everything else, so you really cannot be sure that by the time you do GetSTDOUT(), all output will be available. It works most of the time, but it fails in like ~0.1% of cases -- not much, but enough to be annoying when scrutinizing buildbot failures.

Or, if you meant the other kind of redirection, in all tests that redirect stdio to a file, we have added a if(remote) { platform.GetFile(...)}, to retrieve the file from the remote side -- not really elegant and I would prefer using the char[] approach going forward.

labath added a comment.Sep 5 2016, 8:20 AM

There has been no activity here for 6 months. Shall we close it?

zturner abandoned this revision.Sep 5 2016, 8:58 AM