[fix] Null-guard the cling.printValue lookup in op_str - #56
Conversation
cc6b1ab to
06c8899
Compare
| PyObject* cl = PyObject_GetAttrString(gbl, (char*)"cling"); | ||
| printValue = PyObject_GetAttrString(cl, (char*)"printValue"); | ||
| Py_DECREF(cl); | ||
| // no cling namespace exists unless user code declares one |
There was a problem hiding this comment.
Perhaps we can just short-circuit this path completely if we know the interpreter is clang-repl, since that lookup would always return null. We could use the compile-time definition CPPJIT_USE_CLING, or a runtime check like in test/support.py
There was a problem hiding this comment.
Good call — done. The whole pretty-printing block now sits behind #ifdef CPPJIT_USE_CLING, so clang-repl builds compile it out and str() falls straight through to the generic repr; strings on the clang-repl libcppjit.so shows the printValue lookup gone, and the object shrinks slightly. One thing the gate needed: CPPJIT_USE_CLING was only ever a CMake option and never reached the sources, so I added the matching target_compile_definitions entry — without it the #ifdef would have been false on the cling build too. The null-guards stay inside the cling branch, in case an odd cling state leaves the lookup empty. Worth naming the trade-off: a user-declared namespace cling { printValue } is no longer honored on clang-repl, which I think is right given the native value-printing direction in compiler-research/CppInterOp#1100.
06c8899 to
40e8086
Compare
The in-process test52 crashed the llvm22 and cling CI lanes with a SIGSEGV, hiding the rest of the suite. Match compiler-research#56's head: the cling pretty-print path in op_str now compiles out entirely behind CPPJIT_USE_CLING on clang-repl, with the matching CMake compile-definition wiring, and the regression test runs its repro in a subprocess so a crash fails the assertion instead of the runner. Co-developed-with-the-help-of: Claude Code (Sonnet 5, human in the loop)
40e8086 to
e043129
Compare
| // reasons) | ||
| // reasons). Cling only: clang-repl has no cling namespace to look up, so the | ||
| // whole path compiles out and str() falls through to the generic repr. | ||
| #ifdef CPPJIT_USE_CLING |
There was a problem hiding this comment.
This should probably be required to CppInterOp’s toString interface which will still crash for clang-repl but this time we can fix it there.
There was a problem hiding this comment.
Happy to make that change here: route str() through Cpp::ObjToString and drop the cling-only block. clang-repl would then keep failing in toString until compiler-research/CppInterOp#1100 lands the fix there. Do you want that in this PR, or should we keep the short-circuit and switch once #1100 is fixed?
e043129 to
e5f3418
Compare
| // reasons) | ||
| // reasons). Cling only: clang-repl has no cling namespace to look up, so the | ||
| // whole path compiles out and str() falls through to the generic repr. | ||
| #ifdef CPPJIT_USE_CLING |
There was a problem hiding this comment.
I believe if we want to land this ifdef-based infrastructure we probably want to put it in a single compatibility-like file that expands the right ifdefs in one place.
There was a problem hiding this comment.
I went ahead and moved this into a compatibility header
Prevent a null dereference when str() cannot find cling.printValue. Keep the interpreter check and cached lookup in Compatibility.h. The helper returns nullptr on clang-repl and preserves the Cling fallback. Pass CPPJIT_USE_CLING from CMake to the sources. Run the regression in a subprocess and pass sys.path through PYTHONPATH. Retain the expected failure for the CppInterOp toString stub. Co-developed-with-the-help-of: Claude Code (Fable 5, human in the loop) Co-developed-with-the-help-of: OpenAI Codex (GPT-6, human in the loop)
e5f3418 to
53ae578
Compare
3 participants
str()on any instance without anostreaminserter segfaults on currentmain. Commit5a33027dropped the fakedcling::runtime::gCling, and thepretty-print fallback in
op_strthen dereferenced the failedcppjit.gbl.clinglookup. This guards both lookups, clears theAttributeError, and falls back to the generic repr.Repro:
A regression test is included. The crash kills the interpreter, so the test
asserts the fixed behaviour rather than the crash.