[Mlir-commits] [mlir] [mlir][bytecode] Avoid crash when test dialect version is missing (PR #170173)

[llvmlistbot at llvm.org]

================
@@ -166,8 +166,8 @@ struct TestBytecodeRoundtripPass
             Type &entry) -> LogicalResult {
           // Get test dialect version from the version map.
           auto versionOr = reader.getDialectVersion<test::TestDialect>();
-          assert(succeeded(versionOr) && "expected reader to be able to access "
-                                         "the version for test dialect");
+          if (failed(versionOr))
+            return success();
----------------
Men-cotton wrote:

Thanks for the comment!
I’d prefer to keep this as `return success();` rather than `failure()`.

My understanding is that for the Reader callbacks, returning `failure()` indicates a hard error and aborts the reading process immediately, whereas returning `success()` with `entry` left unset means “I didn’t handle this, please fall back to the default path”.

Since the goal here is to gracefully fallback (skip this callback) when the version is missing, I believe `return success()` is the correct return value for the Reader side (in contrast to the Writer side, where `failure()` triggers a fallback). This also matches the overall design goal described in [this comment](https://github.com/llvm/llvm-project/blob/af27159e17b91cc5f49c2b6ed24f3d422312d6ec/mlir/include/mlir/Bytecode/BytecodeWriter.h#L191), that bytecode handling should be robust and not fail in such cases.

If I’ve misunderstood the intended reader contract, I’m happy to adjust.

https://github.com/llvm/llvm-project/pull/170173