This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Reproducers] Make it possible to capture reproducers for the Python test suite.
ClosedPublic

Authored by JDevlieghere on Apr 6 2020, 2:25 PM.

Details

Diff Detail

Event Timeline

JDevlieghere created this revision.Apr 6 2020, 2:25 PM
labath added inline comments.Apr 7 2020, 7:54 AM
lldb/bindings/python.swig
132–136 ↗(On Diff #255469)

I am doubly unhappy about this. Not only does it leak testing logic into production code, it actually does that using environment variables... :/

I take it the problem here is that reproducers need to be set up before the Initialize call, but the lldb module does not let us do that as it calls Initialize automatically? I am not sure that was such a wise idea, but that ship has sailed a long time ago.

So, can we at least do something like this instead? I.e. have a separate configuration module which would be imported here and would drive this logic.

I believe it should be possible to arrange it so that the configuration module does not even have to get shipped -- this code could just attempt the import and ignore any errors.

And the configuration module itself would not need to do anything reproducer-related either. It could just contain a single flag to disable the automatic initialization. That sounds like a thing someone might conceivably want anyway, and it could be later extended to handle other kinds of global lldb configuration (ConstString pool size maybe ?).

Once the Initialize call is disabled, the normal test harness can handle the rest...

Use lldbconfig to initialize reproducers.

Update lldbtest.py too.

labath added inline comments.Apr 7 2020, 12:00 PM
lldb/packages/Python/lldbsuite/test/decorators.py
861

Should this use variables from configuration.py now?

lldb/test/API/lit.cfg.py
68

lldb-repro

lldb/test/API/lldbtest.py
90

I remember some objections to shoving everything in $TMP. Are we not able to put this in the build dir somewhere?

Address code review feedback

JDevlieghere marked 3 inline comments as done.Apr 7 2020, 1:27 PM

I think the code looks ok now, though I have to say I am still somewhat fuzzy on what exactly will this patch enable. I think I understand the capture part, but what happens during replay? Will that already be smart enough to match the SB calls made by the test harness during replay to the captured calls that were made during recording, or is there some more work needed there? Could you add a brief documentation on this somewhere?

I think the code looks ok now, though I have to say I am still somewhat fuzzy on what exactly will this patch enable. I think I understand the capture part, but what happens during replay? Will that already be smart enough to match the SB calls made by the test harness during replay to the captured calls that were made during recording, or is there some more work needed there?

That's implemented in https://reviews.llvm.org/D77602

Could you add a brief documentation on this somewhere?

Sure thing: https://reviews.llvm.org/D77771

Ok, I'm starting to understand what's going on here. This patch looks mostly fine, but IIUC, it won't really do anything until the subsequent patch lands as that is what implements the inline/api/passive replay. I'm going to start looking at that other patch now.

Ok, I'm starting to understand what's going on here. This patch looks mostly fine, but IIUC, it won't really do anything until the subsequent patch lands as that is what implements the inline/api/passive replay. I'm going to start looking at that other patch now.

It's sufficient for stage one, i.e. capture the test suite and use the driver (active replay), but it also contains the necessary changes for stage 2 (passive replay). I could've split it up in two patches but given that the changes are so similar that didn't seem worth it.

Update naming to passive replay.

labath accepted this revision.Apr 14 2020, 5:14 AM

Ok, I'm starting to understand what's going on here. This patch looks mostly fine, but IIUC, it won't really do anything until the subsequent patch lands as that is what implements the inline/api/passive replay. I'm going to start looking at that other patch now.

It's sufficient for stage one, i.e. capture the test suite and use the driver (active replay), but it also contains the necessary changes for stage 2 (passive replay). I could've split it up in two patches but given that the changes are so similar that didn't seem worth it.

Ah, ok. I can live with that.

This revision is now accepted and ready to land.Apr 14 2020, 5:14 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2020, 9:40 AM