Skip to content

[MLIR] Let AttrTypeReplacer prefer def builders (NFC) #87475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gysit
Copy link
Contributor

@gysit gysit commented Apr 3, 2024

This commit modifies the AttrTypeReplacer to prefer the default builders when creating attributes or types, avoiding the risk of failures associated with implicit casting. Previously, custom builders could lead to segfaults if a null StringAttr was automatically converted to a StringRef. By preferring the default builder, we avoid these potentially fatal conversions.

This commit modifies the AttrTypeReplacer to prefer the default
builders when creating attributes or types, avoiding the risk of
failures associated with implicit casting. Previously, custom builders
could lead to segfaults if a null StringAttr was automatically converted
to a StringRef. By preferring the default builder, we avoid these
potentially fatal conversions.
@gysit
Copy link
Contributor Author

gysit commented Apr 3, 2024

This issue came up in the context of #87295.

The AttrTypeReplacer may call the custom builder of attributes such as:

def LLVM_DISubprogramAttr : LLVM_Attr<"DISubprogram", "di_subprogram",
                                      /*traits=*/[], "DIScopeAttr"> {
  let parameters = (ins
    OptionalParameter<"DistinctAttr">:$id,
    OptionalParameter<"DICompileUnitAttr">:$compileUnit,
    "DIScopeAttr":$scope,
    OptionalParameter<"StringAttr">:$name,
    OptionalParameter<"StringAttr">:$linkageName,
    "DIFileAttr":$file,
    OptionalParameter<"unsigned">:$line,
    OptionalParameter<"unsigned">:$scopeLine,
    OptionalParameter<"DISubprogramFlags">:$subprogramFlags,
    OptionalParameter<"DISubroutineTypeAttr">:$type
  );
  let builders = [
    AttrBuilderWithInferredContext<(ins
      "DistinctAttr":$id, "DICompileUnitAttr":$compileUnit,
      "DIScopeAttr":$scope, "StringRef":$name, "StringRef":$linkageName,
      "DIFileAttr":$file, "unsigned":$line, "unsigned":$scopeLine,
      "DISubprogramFlags":$subprogramFlags, "DISubroutineTypeAttr":$type
    ), [{
      MLIRContext *ctx = file.getContext();
      return $_get(ctx, id, compileUnit, scope, StringAttr::get(ctx, name),
                   StringAttr::get(ctx, linkageName), file, line,
                   scopeLine, subprogramFlags, type);
    }]>
  ];

  let assemblyFormat = "`<` struct(params) `>`";
}

This is problematic since the name parameter is optional and hence can be nullptr. The custom builder is called since the StringAttr supports implicit conversion to a StringRef which is fatal in this case since we may try to convert a nullptr.

@gysit gysit requested a review from zyx-billy April 3, 2024 10:37
@gysit gysit changed the title [MLIR] Let AttrTypeReplacer prefer def builders [MLIR] Let AttrTypeReplacer prefer def builders (NFC) Apr 3, 2024
@gysit gysit requested a review from Mogball April 3, 2024 10:42
@gysit gysit marked this pull request as ready for review April 3, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant