🐛 Adopt recoverable jeff deserialization errors - #2451
Conversation
Update jeff-mlir to the merged upstream import-error fix and cover the former missing-functions abort through Core’s byte-import API. Assisted-by: GPT-6 via Codex
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
denialhaag
left a comment
There was a problem hiding this comment.
LGTM! ![]()
I just have one observation:
|
@denialhaag Seems like the Windows failures are separate: jeff’s deserializer enables exceptions, but KJ builds without them, causing the missing kj::getCaughtExceptionAsKj() symbol. This needs an upstream build-configuration fix. Will prepare one. |
Use the upstream follow-up that enables exceptions for the fetched Cap'n Proto dependency without changing Core's exception policy. Remove the unnecessary SetUp naming suppression. Assisted-by: GPT-6 via Codex
Restore the official repository now that jeff-mlir #56 has merged. Assisted-by: GPT-6 via Codex
Assisted-by: GPT-6 via Codex
|
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 AI text below 🤖
Description
Update jeff-mlir from
4732c4fto upstream commit316eab7fab26d80a38278c8bf2ac2d249bda81a1, adopting the merged deserializer fix (#55) and Windows build fix (#56). The dependency uses the officialunitaryfoundation/jeff-mlirrepository.Core already handles a null deserialization result. With the updated dependency, the previously fatal missing-functions input returns an import error instead of terminating the process. Add one in-process regression through
JeffProgram::fromBytesusing a serialized module without functions. Error handling stays upstream; no Core parser guards or exception-policy changes are added.The deserializer requires exception support in KJ. The Windows fix enables
/EHscwithin the fetched Cap'n Proto dependency's CMake scope, including KJ and parser frames. The setting does not change Core's compiler flags.Remove the unnecessary
SetUpnaming suppression. Its local warning came from incomplete SDK/header configuration; the override needs no source workaround.Addresses the jeff import-abort finding in #2287. This PR is independent of the other audit fixes and is based on
mainatd52b3028c.Validation
uvx nox -s lintpassed. A clean CMake FetchContent download from the official repository checked out the exact merged commit.3f1ea2eto29caf80#56 head exactly. Compared with the previously tested pin650973f, only the Windows workflow/preset relocation differs; runtime sources, tests, and the scoped exception fix are unchanged.eb654ca23, all 165 compiler tests and 151 jeff round-trip tests passed with LLVM/MLIR 23.1.0, including the missing-functions regression and successful imports. These tests were not rerun for the URL/tag-only update.uvx nox -s cpp-lintpassed ateb654ca23with local clang-tidy 23.0.0git and the installed macOS SDK headers configured. No C++ files changed in the upstream-pin update.eb654ca23: Windows x64 and ARM64 C++ tests, Windows Python tests, and C++ lint with clang-tidy 23.1.1 passed. CI for the upstream-pin update remains pending.GPT-6 via Codex assisted the dependency update, regression test, validation, and PR text.
Checklist
If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).