This is an archive of the discontinued LLVM Phabricator instance.

[cfi-verify] Made FileAnalysis operate on a GraphResult rather than build one and validate it.
ClosedPublic

Authored by hctim on Nov 7 2017, 3:08 PM.

Details

Summary

Refactors the behaviour of building graphs out of FileAnalysis, allowing for analysis of the GraphResult by the callee without having to rebuild the graph. Means when we want to analyse the constructed graph (planned for later revisions), we don't do repeated work.

Also makes CFI verification in FileAnalysis now return an enum that allows us to differentiate why something failed, not just that it did/didn't fail.

Diff Detail

Repository
rL LLVM

Event Timeline

hctim updated this revision to Diff 122156.Nov 8 2017, 2:29 PM

Made unit tests not use a helper, they should explicitly individually check the result against the expected enum.

hctim updated this revision to Diff 122159.Nov 8 2017, 2:32 PM

Finally - rebase against the parent revision.

tools/llvm-cfi-verify/lib/FileAnalysis.cpp
70 ↗(On Diff #122159)

This should be an llvm_unreachable()

tools/llvm-cfi-verify/lib/FileAnalysis.h
52 ↗(On Diff #122159)

Could we move the comments above the entries?

59 ↗(On Diff #122159)

This is only returned in one place so why not make it a specific error, rather than a generic catch-all?

unittests/tools/llvm-cfi-verify/FileAnalysis.cpp
502 ↗(On Diff #122159)

Could we make this an easier to differentiate constant? Took me a second to understand why this was failing on not finding the instruction

hctim updated this revision to Diff 122332.Nov 9 2017, 2:18 PM
hctim marked 4 inline comments as done.

Updated with Vlad's comments, changed some enum values/comments, and merged in master.

tools/llvm-cfi-verify/lib/FileAnalysis.h
51 ↗(On Diff #122332)

Lets make this an enum class.

unittests/tools/llvm-cfi-verify/FileAnalysis.cpp
502 ↗(On Diff #122159)

This one still looks open.

hctim updated this revision to Diff 122476.Nov 10 2017, 10:49 AM
hctim marked 2 inline comments as done.

Updated with Vlad's comments, classified the enum, and changed the constant for the unknown instruction to be more obvious at a glance.

hctim added inline comments.Nov 10 2017, 10:52 AM
unittests/tools/llvm-cfi-verify/FileAnalysis.cpp
502 ↗(On Diff #122159)

Oh sorry, I thought you were referring to the enum's name...

Done.

This revision is now accepted and ready to land.Nov 10 2017, 11:16 AM
hctim updated this revision to Diff 122504.Nov 10 2017, 12:56 PM

Merged with master in preparation for submission.

This revision was automatically updated to reflect the committed changes.