[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

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

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

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

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

[tpopp]

Move assertions to NamedAttrList::push_back

mlir/include/mlir/IR/OperationSupport.h

bool operator==(const NamedAttrList &other) const {
     return attrs == other.attrs;
   }
 
   /// Add an attribute with the specified name.
   void append(StringRef name, Attribute attr);
 
   /// Add an attribute with the specified name.
   void append(Identifier name, Attribute attr) {
     append(NamedAttribute(name, attr));
   }
 
   /// Append the given named attribute.
   void append(NamedAttribute attr) { push_back(attr); }
 
   /// Add an array of named attributes.
   template <typename RangeT>
   void append(RangeT &&newAttributes) {
public:
 
   /// Add an attribute with the specified name.
   void addAttribute(StringRef name, Attribute attr) {
     addAttribute(Identifier::get(name, getContext()), attr);
   }
 
   /// Add an attribute with the specified name.
   void addAttribute(Identifier name, Attribute attr) {
+    assert(attr && "unexpected null attribute");
     attributes.append(name, attr);
   }
 
   /// Add an array of named attributes.
   void addAttributes(ArrayRef<NamedAttribute> newAttributes) {
     attributes.append(newAttributes);
   }
 

mlir/lib/IR/Operation.cpp

 Operation::Operation(Location location, OperationName name, unsigned numResults,
                      unsigned numSuccessors, unsigned numRegions,
                      DictionaryAttr attributes, bool hasOperandStorage)
     : location(location), numResults(numResults), numSuccs(numSuccessors),
       numRegions(numRegions), hasOperandStorage(hasOperandStorage), name(name),
       attrs(attributes) {
   assert(attributes && "unexpected null attribute dictionary");
 #ifndef NDEBUG
+  llvm::for_each(attributes.getValue(), [](NamedAttribute idAndAttr) {
+    assert(idAndAttr.second &&
+           "unexpected null attribute in attribute dictionary");
+  });
   if (!getDialect() && !getContext()->allowsUnregisteredDialects())
     llvm::report_fatal_error(
         name.getStringRef() +
         " created with unregistered dialect. If this is intended, please call "
         "allowUnregisteredDialects() on the MLIRContext, or use "
         "-allow-unregistered-dialect with the MLIR opt tool used");
 #endif
 }