[mlir] Add assertions to prevent adding null attributes

[tpopp]

Summary

Setting attributes on operations results in an assertion failure, but
this could be missed currently by adding a null attribute to an
OperationState and building an Operation with that. This change adds
some additional assertions to make these different locations consistent
with each other.

[rriddle]

mlir/include/mlir/IR/OperationSupport.h

486 ↗

(On Diff #368159)

If we are going to assert when adding attributes, I would instead add asserts to NamedAttrList. Asserts there would catch more cases.

mlir/lib/IR/Operation.cpp

179 ↗

(On Diff #368159)

This should be guaranteed on construction by DictionaryAttr. Is there a case that isn't being caught?

[rriddle]

Thanks for the patch!

[tpopp]

mlir/lib/IR/Operation.cpp

179 ↗

(On Diff #368159)

I'm removing this one. The case that was not handled was appending to a NamedAttrList and then calling NamedAttrList::getDictionary(MLIRContext *context). That is handled by your suggestion above though, so I'll remove this one.

[tpopp]

Move the assertion logic to NamedAttrList.

[rriddle]

mlir/include/mlir/IR/OperationSupport.h

275 ↗

(On Diff #368330)

Should we move this to push_back instead? (The method that this bottoms out to)

[tpopp]

Move assertions to NamedAttrList::push_back

mlir/lib/IR/OperationSupport.cpp

 
 /// Replaces the attributes with new list of attributes.
 void NamedAttrList::assign(const_iterator in_start, const_iterator in_end) {
   DictionaryAttr::sort(ArrayRef<NamedAttribute>{in_start, in_end}, attrs);
   dictionarySorted.setPointerAndInt(nullptr, true);
 }
 
 void NamedAttrList::push_back(NamedAttribute newAttribute) {
+  assert(newAttribute.second && "unexpected null attribute");
   if (isSorted())
     dictionarySorted.setInt(
         attrs.empty() ||
         strcmp(attrs.back().first.data(), newAttribute.first.data()) < 0);
   dictionarySorted.setPointer(nullptr);
   attrs.push_back(newAttribute);
 }