[Mlir-commits] [mlir] [MLIR] Fix crash when parsing typed attribute via parseOptionalAttribute (PR #186192)

[Andy Kaylor]

andykaylor wrote:

This change caused several regressions in CIR. Two of the regressions were caused by code that did this:

```
  // Parse CXXSpecialMember attribute
  if (parser.parseOptionalKeyword("special_member").succeeded()) {
    cir::CXXCtorAttr ctorAttr;
    cir::CXXDtorAttr dtorAttr;
    cir::CXXAssignAttr assignAttr;
    if (parser.parseLess().failed())
      return failure();
    if (parser.parseOptionalAttribute(ctorAttr).has_value())
      state.addAttribute(specialMemberAttr, ctorAttr);
    else if (parser.parseOptionalAttribute(dtorAttr).has_value())
      state.addAttribute(specialMemberAttr, dtorAttr);
    else if (parser.parseOptionalAttribute(assignAttr).has_value())
      state.addAttribute(specialMemberAttr, assignAttr);
    if (parser.parseGreater().failed())
      return failure();
  }
```

Here the attribute we're trying to parse wasn't actually optional, but we didn't know which of several attribute types it would be and we were using `parseOptionalAttribute` to try them sequentially. This is a pretty ugly idiom, and I'm happy to change this code to parse the attribute non-optionally and check the type.

The other failure we encountered is a bit more complicated. In that case we had an operation with a `DefaultValuedAttr` using a dialect-specific enum type (`global_visibility`) followed by a required attribute that also used a a dialect-specific enum type (`linkage`).

```
  let arguments = (ins SymbolNameAttr:$sym_name,
                       DefaultValuedAttr<
                        CIR_VisibilityAttr,
                        "VisibilityKind::Default"
                       >:$global_visibility,
                       OptionalAttr<StrAttr>:$sym_visibility,
                       TypeAttr:$sym_type,
                       CIR_GlobalLinkageKind:$linkage,
                       OptionalAttr<MemorySpaceAttrInterface>:$addr_space,
                       OptionalAttr<CIR_TLSModel>:$tls_model,
                       OptionalAttr<AnyAttr>:$initial_value,
                       UnitAttr:$comdat,
                       UnitAttr:$constant,
                       UnitAttr:$dso_local,
                       OptionalAttr<CIR_StaticLocalGuardAttr>:$static_local_guard,
                       OptionalAttr<I64Attr>:$alignment,
                       OptionalAttr<ASTVarDeclInterface>:$ast);
```
We also have a custom assembly format for this operation:
```
  let assemblyFormat = [{
    ($sym_visibility^)?
    (`` $global_visibility^)?
    (`constant` $constant^)?
    $linkage
    (`comdat` $comdat^)?
    ($tls_model^)?
    (`dso_local` $dso_local^)?
    (`static_local_guard` `` $static_local_guard^)?
    (` ` custom<GlobalAddressSpaceValue>($addr_space)^ )?
    $sym_name
    custom<GlobalOpTypeAndInitialValue>($sym_type, $initial_value,
                                        $ctorRegion, $dtorRegion)
    attr-dict
  }];
```
In this case, our test for the missing non-optional attribute failed with a different message than it had before the `parseOptionalAttribute` change. Our test is looking for a message saying that `linkage` wasn't found with one of the expected values ("expected-error at +1 {{expected string or keyword containing one of the following enum values for attribute 'linkage' [external, available_externally, linkonce, linkonce_odr, weak, weak_odr, appending, internal, cir_private, extern_weak, common]}}") instead, we now get "error: unexpected error: custom op 'cir.global' expected attribute
 of a different type". 

I'll admit, what we're doing here feels like kind of a mess, but it worked before and now it doesn't. Claude tells me that the problem is that we weren't able to properly distinguish the missing optional attribute from the required attribute. Its suggested fix was to have `parseOptionalAttribute` look for the `#dialect.mnemonic<>` form of the attribute to avoid inadvertantly consuming parts of the custom assembly format that weren't involved in either of these attributes. I'm posting Claude's suggested change as a PR, but I'm open to other suggestions.

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