Add a safeguard when creating a MLIR operation with a null/invalid Type

[mehdi_amini]

Summary

This is protecting against cases where a dialect would not register a
type: the ODS generated parser would invoke this and it would "just
work" but yield unexpected results down the road.
This is caught with assertions in a different place, but this check here
seems more friendly to users.

[rriddle]

This seems like something that would be an assert? I have a somewhat growing concern over the inconsistent use of error handling mechanism (assert vs report_fatal_error).

mlir/lib/IR/Operation.cpp

178

This would only be necessary in the hasSingleResult branch given that ::get will never return null.

[mehdi_amini]

I have a somewhat growing concern over the inconsistent use of error handling mechanism (assert vs report_fatal_error).

That's true, I've been using more report_fatal_error where assert would be appropriate recently. I've been doing so in places which are not supposed to be "cheap" APIs already, but that may still bloat in other dimensions as well.

The LLVM guide only invites to "use asserts liberally but does not talk about tradeoffs between assert vs report_fatal_error runtime checks.
Should we try to revisit our practices on this and try to come up with more clear guidelines?

[stephenneuendorffer]

My rule of thumb is to use assert to check properties that should hold
based on local code postconditions (i.e. 'my' code),
and to use report_fatal_error to report failures in assumptions about what
other components produce (i.e. 'someone else's code').

So, IMO, errors for public APIs should always be fatal errors with good
messages, not assert()'s.
A pass validating some property of its input should be a fatal error.
A pass validating a property of its own internal data structures should be
an assert.

Some people probably think
that this is needlessly verbose and there are many places in LLVM that
assert() if an API is called with data that violates its
preconditions (which are rarely well documented). Personally, I hate this,
because I'm not an expert in any LLVM APIs, so I
endup firing up GDB or instrumenting the code in question to trace through
other people's code to understand a bug in *my*
code.

Steve

[stephenneuendorffer]

My rule of thumb is to use assert to check properties that should hold
based on local code postconditions (i.e. 'my' code),
and to use report_fatal_error to report failures in assumptions about what
other components produce (i.e. 'someone else's code').

So, IMO, errors for public APIs should always be fatal errors with good
messages, not assert()'s.
A pass validating some property of its input should be a fatal error.
A pass validating a property of its own internal data structures should be
an assert.

Some people probably think
that this is needlessly verbose and there are many places in LLVM that
assert() if an API is called with data that violates its
preconditions (which are rarely well documented). Personally, I hate this,
because I'm not an expert in any LLVM APIs, so I
endup firing up GDB or instrumenting the code in question to trace through
other people's code to understand a bug in *my*
code.

Steve

[mehdi_amini]

Yes this is more defensive than what is usually done in LLVM, for example we don't re-check everywhere the invariant that are supposed to be enforced by the verifier: even though you can invoke a pass on invalid IR.

[stephenneuendorffer]

I think there's a little bit of grey area here. What is 'my code' for
instance? In the case of IR invariants, I think there are two questions:

1. should there be a check? and 2) if there is a check, what form should it

take? In this case, I think that it makes sense for this kind of
verification to be essentially delegated and centralized in the verifier:
Although one *could* pass invalid IR to the verifier, in most usage, the
verifier is usually coupled together with the execution of passes.
However, verifier checks are not asserts, since they are not checking a
local invariant property of the code, but are checking a property which
should be guaraunteed by some other piece of code.

Steve.

[mehdi_amini]

Ultimately the only difference between assert vs report_fatal_error is whether it is checked in production and how much it'll be in the path of the runtime execution and binary size, otherwise neither is recoverable.
Some users of LLVM ship clang in production with asserts enabled, if you care about always having checks enabled, that's likely the way to go.

Developers should always have assertions on, so this shouldn't matter much for them (and I'm not sure how it addresses you're GDB question anyway).

[jpienaar]

There is also just returning an error too (it's not just a fatal error or
assert dichotomy here).

I agree with River here about an assert seeming more appropriate. But you
mention this is to guard a case in the generated parser, why can't we
insert verification & return there? Parsing returns an error and all, and
so if detected there then we already have error propagation path.

• smime.p7s3 KBDownload

[mehdi_amini]

But you mention this is to guard a case in the generated parser, why can't we
insert verification & return there? Parsing returns an error and all, and
so if detected there then we already have error propagation path.

Because this is fundamentally a fatal error indicating the project is misconfigured. Without this check it would either fail with an assertions (~easy to figure if you're familiar with it) or fail the verifier (in a very confusing way).

I hit this with a tool we ship internally where this error wasn't caught in testing somehow and the release mode lead to a confusing verifier issue.

That said I can probably improve the verifier instead (and we already have an assertion to catch this anyway).

[stephenneuendorffer]

The difference I see is that assets rarely have a useful message, because
they can't refer to the specific values they are comparing. The other
error reporting mechanisms make this possible. Or maybe it's the other way
around: people who think about nice error messages rarely use assert
because you can't create one and so use report fatal error instead. :)

So I almost always have to dig deeper with an assert, while a nice error
message makes my errors easy to find.

Steve

Steve

[mehdi_amini]

I suspect we're getting into "vocabulary" here, but I still call this "assert" even though they aren't technically: I actually write it this way https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/IR/OpDefinition.h#L1274-L1279

(I still have to create a wrapper to make it easier)

[stephenneuendorffer]

I dig it. Definitely not a case where the runtime overhead would be
desirable in a release build, and a something that should be easily caught
by development testing.

Steve

mlir/lib/IR/Operation.cpp

: location(location), numSuccs(numSuccessors), numRegions(numRegions),
       attrs(attributes) {
   if (!resultTypes.empty()) {
     // If there is a single result it is stored in-place, otherwise use a tuple.
     hasSingleResult = resultTypes.size() == 1;
     if (hasSingleResult)
       resultType = resultTypes.front();
     else
       resultType = TupleType::get(resultTypes, location->getContext());
+    if (!resultType)
+      llvm::report_fatal_error(
+          "Building an Operation with invalid/empty result Type");
   }
 }
 
 // Operations are deleted through the destroy() member because they are
 // allocated via malloc.
 Operation::~Operation() {
   assert(block == nullptr && "operation destroyed but still in a block");