This is an archive of the discontinued LLVM Phabricator instance.

[Verifier] add invariant check for callbr
ClosedPublic

Authored by nickdesaulniers on Sep 4 2019, 3:25 PM.

Details

Summary

The list of indirect labels should ALWAYS have their blockaddresses as
argument operands to the callbr (but not necessarily the other way
around). Add an invariant that checks this.

The verifier catches a bad test case that was added recently in r368478.
I think that was a simple mistake, and the test was made less strict in
regards to the precise addresses (as those weren't specifically the
point of the test).

This invariant will be used to find a reported bug.

Link: https://www.spinics.net/lists/arm-kernel/msg753473.html
Link: https://github.com/ClangBuiltLinux/linux/issues/649

Event Timeline

nickdesaulniers created this revision.Sep 4 2019, 3:25 PM
  • rebase on master, run git-clang-format HEAD~

Is this already documented in LangRef?

llvm/test/CodeGen/AArch64/callbr-asm-obj-file.ll
1–4

Does update_llc_test_checks actually deal with this chaining?
I don't believe it does.

nickdesaulniers marked an inline comment as done.Sep 4 2019, 3:40 PM

Is this already documented in LangRef?

It is not!
https://llvm.org/docs/LangRef.html#callbr-instruction
Shall I add to that in this patch? (Assuming it's ok to explicitly state such an invariant?)

llvm/test/CodeGen/AArch64/callbr-asm-obj-file.ll
1–4

It does not, hence why I wasn't able to run ./llvm/utils/update_llc_test_checks.py llvm/test/CodeGen/AArch64/callbr-asm-obj-file.lland then observe any changes to the file (other than the header, which I see I committed accidentally). Do you know how I would fix the chaining related issue?

lebedev.ri added inline comments.Sep 4 2019, 3:47 PM
llvm/test/CodeGen/AArch64/callbr-asm-obj-file.ll
1–4

By manually updating the check lines i suppose. I hate doing that :/

srhines added inline comments.Sep 4 2019, 3:53 PM
llvm/test/Verifier/callbr.ll
3

Can you add a test case that shows the proper subset behavior (i.e. more blockaddress labels than are handled by the indirect label list)?

Can you also flip the labels in one test case so that you aren't just iterating over the same exact labels in the same exact order in both places (i.e. verify set behavior rather than just simple iteration).

  • add unit test, add invariant to LangRef, remove "or jump" from LangRef (was never there)
nickdesaulniers marked 3 inline comments as done.Sep 4 2019, 4:26 PM
  • fix my added unit test

LGTM, but please wait for someone more familiar with LangRef policies to review that part.

void added a comment.Sep 9 2019, 3:06 PM

The llvm/test/CodeGen/AArch64/callbr-asm-obj-file.ll change LGTM.

Bumping for code review

void accepted this revision.Sep 23 2019, 5:23 PM

LGTMing. Others please comment if you have objections.

This revision is now accepted and ready to land.Sep 23 2019, 5:23 PM
This revision was automatically updated to reflect the committed changes.