This is an archive of the discontinued LLVM Phabricator instance.

Don't count attributes when addressing operands.
ClosedPublic

Authored by ijsung on Nov 24 2020, 11:45 PM.

Details

Summary

Fixes out-of-bound access in generated nested DAG rewriter matching code.

Diff Detail

Event Timeline

ijsung created this revision.Nov 24 2020, 11:45 PM
ijsung requested review of this revision.Nov 24 2020, 11:45 PM

Could you add a test where this would have triggered OOB access?

tpopp added a subscriber: tpopp.Nov 25 2020, 7:52 AM
tpopp added inline comments.
mlir/tools/mlir-tblgen/RewriterGen.cpp
372

It might be slightly easier to read this by having a nextOperand++ instead of this numPrevAttrs computation because then it's more like an iterator than arithmetic to understand and the value is only used in one place.

ijsung updated this revision to Diff 307765.Nov 25 2020, 11:16 PM
This comment was removed by ijsung.
ijsung updated this revision to Diff 307766.Nov 25 2020, 11:23 PM
This comment was removed by ijsung.
ijsung updated this revision to Diff 307767.Nov 25 2020, 11:26 PM

Addressed comments and added a test.

PTAL

ijsung updated this revision to Diff 307768.Nov 25 2020, 11:33 PM

Small test case indenting fix.

Harbormaster completed remote builds in B80185: Diff 307768.
ijsung marked an inline comment as done.Nov 26 2020, 2:24 AM
tpopp added inline comments.Nov 27 2020, 2:27 PM
mlir/test/mlir-tblgen/rewriter-indexing.td
2

This is an internal detail. Can you run this test through presubmit in the work codebase? We sometimes have to work around lit implementation details and it can take time, so if you find the test failing, I can start to work on the fix in advance.

40

Does this one gain anything? Ideally we avoid these ifdefs because they make it more confusing.

mlir/tools/mlir-tblgen/RewriterGen.cpp
383

Arg. I didn't realize you would have to increment nextOperand in two places anyway. Sorry for making you turn a 2 line change into a different 2 line change.

ijsung updated this revision to Diff 308129.Nov 27 2020, 6:26 PM

Addressed more comments on the test.

ijsung marked an inline comment as done.Nov 27 2020, 6:30 PM

Thanks & PTAL

mlir/test/mlir-tblgen/rewriter-indexing.td
2

Ran through

blaze test //third_party/llvm/llvm-project/mlir/test/mlir-tblgen/...

and verified test working as expected.

40

Replaced with just one FileCheck run.

mlir/tools/mlir-tblgen/RewriterGen.cpp
383

Ack. It looks slightly better than old version (as there's no confusing subtraction) anyways.

tpopp accepted this revision.Nov 30 2020, 6:31 AM
tpopp added inline comments.
mlir/test/mlir-tblgen/rewriter-indexing.td
12–13

Maybe move these 2 lines to below the COp definition to make it clear that all the actual tests are only below that point.

46

Delete the newline to be consistent with spacing above.

This revision is now accepted and ready to land.Nov 30 2020, 6:31 AM
ijsung updated this revision to Diff 308490.Nov 30 2020, 3:09 PM
ijsung marked an inline comment as done.

Addressed comments on the test.

ijsung marked 2 inline comments as done.Nov 30 2020, 3:10 PM

Thanks for reviewing!

For the Author property please use the following:

Name: Ray (I-Jui) Sung
Email: ijsung@google.com

Thanks!

jpienaar accepted this revision.Nov 30 2020, 4:46 PM

Thanks!

This revision was automatically updated to reflect the committed changes.