Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle error in call trace function #70

Open
todo bot opened this issue Dec 8, 2020 · 4 comments · May be fixed by #210
Open

Handle error in call trace function #70

todo bot opened this issue Dec 8, 2020 · 4 comments · May be fixed by #210

Comments

@todo
Copy link

todo bot commented Dec 8, 2020

Pyjion/src/pyjion/intrins.cpp

Lines 2044 to 2049 in 4995e76

// TODO : Handle error in call trace function
}
}
void PyJit_TraceFrameEntry(PyFrameObject* f){
auto tstate = PyThreadState_GET();


This issue was generated by todo based on a TODO comment in 4995e76 when #68 was merged. cc @tonybaloney.
@tonybaloney
Copy link
Owner

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.
    GLOBAL_METHOD(METHOD_TRACE_LINE, &PyJit_TraceLine, CORINFO_TYPE_VOID, Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT));
  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>)

    Pyjion/src/pyjion/absint.cpp

    Lines 1569 to 1571 in 4995e76

    if (mTracingEnabled){
    m_comp->emit_trace_line(mTracingInstrLowerBound, mTracingInstrUpperBound, mTracingLastInstr);
    }

    e.g.,
if (mTracingEnabled){ 
     m_comp->emit_trace_line(mTracingInstrLowerBound, mTracingInstrUpperBound, mTracingLastInstr); 
     intErrorCheck("Trace line failed");
 } 
  1. 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
Copy link

DanielRosenwasser commented Feb 16, 2021

Thanks for the details!

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

Pyjion/src/pyjion/intrins.cpp

Lines 2636 to 2637 in 1cfad51

if (tstate->tracing)
return;

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
Copy link
Owner

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
Copy link
Owner

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

@DanielRosenwasser DanielRosenwasser linked a pull request Feb 17, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants