This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Fix two AttributeParser aborts
ClosedPublic

Authored by jpienaar on Mar 22 2022, 10:41 AM.

Details

Summary

Reproducers that resulted in triggering the following asserts

mlir::NamedAttribute::NamedAttribute(mlir::StringAttr, mlir::Attribute)
mlir/lib/IR/Attributes.cpp:29:3
consumeToken
mlir/lib/Parser/Parser.h:126

Diff Detail

Event Timeline

jpienaar created this revision.Mar 22 2022, 10:41 AM
Herald added a project: Restricted Project. · View Herald Transcript
jpienaar requested review of this revision.Mar 22 2022, 10:41 AM
Mogball added inline comments.Mar 23 2022, 1:04 AM
mlir/lib/Parser/AttributeParser.cpp
274–290

This should be moved before uses of the empty name.

mlir/test/IR/invalid.mlir
1649

Wasn't it the convention to note have these in-line? @rriddle

1653
jpienaar updated this revision to Diff 418097.Mar 24 2022, 5:53 PM
jpienaar marked 2 inline comments as done.

Rebase post EOF fix landing & move valid identifier check earlier before first
string use.

jpienaar marked an inline comment as done.Mar 24 2022, 5:53 PM
jpienaar added inline comments.
mlir/test/IR/invalid.mlir
1649

This file is about 50-50 (84 out of line, 100 in-line), but I'll make it out of line and could make it consistent in pure NFC follow up.

rriddle added inline comments.Mar 28 2022, 7:59 AM
mlir/lib/Parser/AttributeParser.cpp
172–175

Why is this necessary? Which crash is this hitting?

277

Do we not have an empty method?

mlir/test/IR/invalid.mlir
1649

Yeah, we should clean these up.

Mogball accepted this revision.Mar 28 2022, 9:05 AM
Mogball added inline comments.
mlir/lib/Parser/AttributeParser.cpp
172–175

":<eof>" is crashing here because it reads past EOF.

277

we do not

This revision is now accepted and ready to land.Mar 28 2022, 9:05 AM
jpienaar marked an inline comment as done.Mar 28 2022, 7:59 PM
jpienaar added inline comments.
mlir/lib/Parser/AttributeParser.cpp
172–175

Yes, consumeToken inside crashes. I add error too as that would trigger assert there too. I'm not 100% sure what cases the reset & consume here wants to support/won't error on so I can adjust this if there is something better fitting.

This revision was automatically updated to reflect the committed changes.