This is an archive of the discontinued LLVM Phabricator instance.

lit/Register: Avoid stdio in register write tests
AbandonedPublic

Authored by labath on Jun 6 2019, 6:07 AM.

Details

Reviewers
mgorny
jgorbe
Summary

The linux kernel doesn't guarantee that the data written to a pty will
instantly be available for reading on the other side. So what sometimes
happens here is that the inferior dumps all register values and exits,
lldb registers the exit, goes to read the inferior stdio, but the data
is not there.

Depending on how the machine is configured, the frequency of this
occurring can range from "almost never" to "pretty much always". This
patch side-steps the issue by redirecting the inferior output to a file.
Files have better synchronization properties (plenty of applications
depend on those), and this fixes the flakyness of these tests on
affected machines.

Event Timeline

labath created this revision.Jun 6 2019, 6:07 AM
mgorny added inline comments.Jun 6 2019, 6:20 AM
lit/Register/x86-64-gp-write.test
6–7

Any reason not to use %T directly in paths rather than changing working directory? I suppose either way is fine but I have a bad feeling that this could be confusing in the long run.

labath marked an inline comment as done.Jun 6 2019, 6:27 AM
labath added inline comments.
lit/Register/x86-64-gp-write.test
6–7

%T doesn't work inside "process launch"

mgorny added a comment.Jun 6 2019, 6:37 AM

I'm sorry but could you also change other write tests (zmm?) to match.

lit/Register/x86-64-gp-write.test
6–7

Right, makes sense.

labath updated this revision to Diff 203370.Jun 6 2019, 8:09 AM

Update other tests too. I also forgot to mention that this enables the 32-bit
tests to run on 64-bit platforms too by explicitly compiling for 32-bit (-m32).

mgorny added a comment.Jun 6 2019, 8:20 AM

Update other tests too. I also forgot to mention that this enables the 32-bit
tests to run on 64-bit platforms too by explicitly compiling for 32-bit (-m32).

I'm not sure if this is a good idea. I was thinking about it originally but I suspected it might cause failures on non-multilib systems.

labath added a comment.Jun 6 2019, 8:34 AM

Update other tests too. I also forgot to mention that this enables the 32-bit
tests to run on 64-bit platforms too by explicitly compiling for 32-bit (-m32).

I'm not sure if this is a good idea. I was thinking about it originally but I suspected it might cause failures on non-multilib systems.

Yeah, you're right. I'll revert that part. I did it mainly so I could test my changes there, though it would be nice to have these test run generally too, as these days I expect most people who debug 32-bit apps will be using a 64-bit debugger. I guess we'll one day need a "can-run-32-bit-code" feature, though I'm not sure how to go about implementing that. in cmake?

labath updated this revision to Diff 203378.Jun 6 2019, 8:43 AM

revert the 32-on-64-bit part

mgorny accepted this revision.Jun 6 2019, 8:58 AM

LGTM, thanks.

I also hacked -m32 for local testing. I generally agree this is a good thing to do but I don't really know how to reliably determine support for this. My main concern were C++ stdlib problems (which can be trivially worked around) but we also need to know whether the system has needed crt, libc… and kernel support for running 32-bit apps. Sounds like the only reliable way would be to crash-test this.

This revision is now accepted and ready to land.Jun 6 2019, 8:58 AM
jgorbe added a comment.Jun 6 2019, 1:44 PM

About %T not working for "process launch", what about something like RUN: %lldb -b --one-line-before-file "process launch --stdout %T/x86-zmm-write.out" -s %s %t and then FileCheck-ing?

labath planned changes to this revision.Jun 7 2019, 2:46 AM

It looks like this problem is more widespread than we originally thought (a bunch of other tests are affected too). I'll need to think whether we can come up with a more general solution.

About %T not working for "process launch", what about something like RUN: %lldb -b --one-line-before-file "process launch --stdout %T/x86-zmm-write.out" -s %s %t and then FileCheck-ing?

There are two issues with that. The first one is that with --one-line, lldb stops processing commands after hitting the int3 instruction. I think this is related to the fact that lldb aborts the script if the inferior crashes, and it considers an unexpected int3 instruction to be a "crash". However, it's not clear to me why should the behavior depend on whether the command is in the script or on the command line, so this may be a bug actually.

The second one is that putting complex commands on the command line creates a bit of a quoting nightmare, as the command is quote-processed both by the shell and by lldb. And shells (particularly windows ones) differ in how they handle that. If %T doesn't contain any funny characters, then everything is fine (and I expect a lot of our tests would fail if it did, so we kind of already assume that). However, I am reluctant to recommend that as the best practice for handling these kinds of things.

labath abandoned this revision.Aug 9 2019, 1:56 AM