tonybaloney / Pyjion

Pyjion - A JIT for Python based upon CoreCLR
https://www.trypyjion.com
MIT License
1.42k stars 59 forks source link

Handle error in call trace function #70

Open todo[bot] opened 3 years ago

todo[bot] commented 3 years ago

https://github.com/tonybaloney/Pyjion/blob/4995e76ed0725adfd616dab04550b34bbcc28824/src/pyjion/intrins.cpp#L2044-L2049


This issue was generated by todo based on a TODO comment in 4995e76ed0725adfd616dab04550b34bbcc28824 when #68 was merged. cc @tonybaloney.
tonybaloney commented 3 years ago

Steps to implement this:

  1. Change the signature of PyJit_TraceLine to return an int in the header and the implementation. Return -1 if the tracing call failed (currently the result variable is keeping this, but its unused.
  2. Change the JIT CIL signature of the METHOD_TRACE_LINE method token so that the return type is CORINFO_TYPE_INT. Its currently CORINFO_TYPE_VOID. This will mean that the value stack will now have the result after execution. https://github.com/tonybaloney/Pyjion/blob/4995e76ed0725adfd616dab04550b34bbcc28824/src/pyjion/pycomp.cpp#L1497
  3. Change the code that calls the frame trace to evaluate the top of the stack (the integer) and raise a failure branch if it equals -1. The util function for this is intErrorCheck(<debug message>) https://github.com/tonybaloney/Pyjion/blob/4995e76ed0725adfd616dab04550b34bbcc28824/src/pyjion/absint.cpp#L1569-L1571 e.g.,
    if (mTracingEnabled){ 
     m_comp->emit_trace_line(mTracingInstrLowerBound, mTracingInstrUpperBound, mTracingLastInstr); 
     intErrorCheck("Trace line failed");
    } 
  4. Create a test for it in the Python tests. See test_tracing.py. What you're trying to simulate is the tracing function raising an exception.
DanielRosenwasser commented 3 years ago

Thanks for the details!

It seems like there are two branches that check whether tracing is already occurring before an early return:

https://github.com/tonybaloney/Pyjion/blob/1cfad5108c467202d6e3570a86865c16cd5c0e1e/src/pyjion/intrins.cpp#L2636-L2637

Are these error conditions (i.e. should they return -1)?

Additionally, I'm still a little bit stumped on actually setting up and running the tests. I'll see if I can figure it out from the CI config.

tonybaloney commented 3 years ago

If tracing is already running, it should return 0.

To setup the tests, you need to pass the 'BUILD_TESTS=ON' option to CMake when configuring.

Alternatively, use the Dev Container if you're running VS Code with the Remote Containers extension. When you open the folder it'll set everything up for you. https://code.visualstudio.com/docs/remote/containers#_getting-started

tonybaloney commented 3 years ago

I should mention there are two test suites. The unit tests are in C++ using Catch2. That's the unit_test.exe binary that will get compiled if you use that option. The other is the Python tests.

For tracing, there is a C++ unit test suite, which tests very simple scenarios. The test_tracing.py for Python tests the more complex and tests the integration of components.

The easiest way to get the Python tests running is to compile the project, then set PYTHONPATH to the build output