[Lldb-commits] [PATCH] D77588: [lldb/Reproducers] Make it possible to capture reproducers for the Python test suite.

Tue Apr 7 08:07:14 PDT 2020

labath added inline comments.

================
Comment at: lldb/bindings/python.swig:132-136
+if 'LLDB_REPRODUCER_CAPTURE_PATH' in os.environ:
+   SBReproducer.Capture(os.environ['LLDB_REPRODUCER_CAPTURE_PATH'])
+   SBReproducer.SetAutoGenerate(True)
+elif 'LLDB_REPRODUCER_REPLAY_PATH' in os.environ:
+   SBReproducer.APIReplay(os.environ['LLDB_REPRODUCER_REPLAY_PATH'])
----------------
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 [[ https://stackoverflow.com/questions/3720740/pass-variable-on-import/39360070#39360070 | 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...

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77588/new/

https://reviews.llvm.org/D77588