This is an archive of the discontinued LLVM Phabricator instance.

Add option -verify-cfiinstrs to run verifier in CFIInstrInserter
ClosedPublic

Authored by petarj on May 4 2018, 11:50 AM.

Details

Summary

Instead of enabling it for non NDEBUG builds, use -verify-cfiinstrs to
run verifier in CFIInstrInserter. It defaults to false.

Diff Detail

Repository
rL LLVM

Event Timeline

petarj created this revision.May 4 2018, 11:50 AM
vlad.tsyrklevich accepted this revision.May 4 2018, 1:38 PM

Seems like there should also be a test case that triggers the verify failure.

This revision is now accepted and ready to land.May 4 2018, 1:38 PM
petarj added a comment.EditedMay 4 2018, 2:20 PM

Seems like there should also be a test case that triggers the verify failure.

If you are referring to the llvm.trap() issue, that will be part of a separate patch and a test case.
There appears to be an issue with inconsistent incoming offsets for 'noreturn' basic blocks.

But also makes sense to have a test case that uses the option and triggers the failure.
Will do that.

Instead of adding a new flag, would it be possible to make this part of the machine verifier? (-verify-machineinstrs)

petarj added a comment.May 4 2018, 2:55 PM

Instead of adding a new flag, would it be possible to make this part of the machine verifier? (-verify-machineinstrs)

Good idea to try to merge it into Machine Verifier later. Right now, we need to make it optional so it does not break anyone's code.

petarj updated this revision to Diff 145454.May 7 2018, 6:12 AM

Adding the test cases for -verify-cfiinstrs.

violetav accepted this revision.May 7 2018, 6:33 AM

LGTM

This revision was automatically updated to reflect the committed changes.