This is an archive of the discontinued LLVM Phabricator instance.

Classify llvm-cfi-verify.
ClosedPublic

Authored by hctim on Sep 28 2017, 1:05 PM.

Event Timeline

hctim created this revision.Sep 28 2017, 1:05 PM
hctim updated this revision to Diff 117043.Sep 28 2017, 1:07 PM
hctim added a comment.Sep 28 2017, 1:07 PM

Removed commented-out line from unittests/CMakeListst.txt

vlad.tsyrklevich requested changes to this revision.Sep 28 2017, 3:10 PM

This commit is hard to understand in it's totality and seems like it could be broken up into a couple of smaller changes. First off, the current llvm-verify-cfi.cpp could be refactored into the current FileVerifier interface without any of the new functionality. Next you could add functionality and tests for building CFGs, and then finally the CFI functionality built on top of that. Also, with that in mind, does it make sense to mix-up FileVerifier with the CFG construction logic? Those seem like they should be logically separate, possibly with some form state object passed between them to encapsulate the private members of FileVerifier?

This revision now requires changes to proceed.Sep 28 2017, 3:10 PM
hctim updated this revision to Diff 117080.Sep 28 2017, 5:59 PM
hctim edited edge metadata.

Changed this update to simply be an 'objectification' of the previous llvm-cfi-verify tool. This simply allows for a user to create a binary, disassemble it into the object, and print the indirect CFI instructions.

hctim updated this revision to Diff 117081.Sep 28 2017, 5:59 PM

Readded <cstdlib> as an include for EXIT_SUCCESS;

hctim retitled this revision from First (real) implementation of llvm-cfi-verify. to Classify llvm-cfi-verify..Sep 28 2017, 6:01 PM
hctim edited the summary of this revision. (Show Details)
hctim updated this revision to Diff 117176.Sep 29 2017, 10:17 AM

Re-added basic disassembly traversal unit tests.

hctim updated this revision to Diff 117211.Sep 29 2017, 1:08 PM

Added prev and next tests from bad instructions.

hctim updated this revision to Diff 117425.Oct 2 2017, 2:40 PM

Updating base.

hctim updated this revision to Diff 117444.Oct 2 2017, 4:20 PM
  • fixed warnings
tools/llvm-cfi-verify/FileVerifier.cpp
2

Missing header

74

Seems clearer to specify Address' type here (e.g. is a reference needed given that it's a uint64_t?)

109

I think this would be clearer if you prefix incremented the second term instead of the first.

234

231-239 can be simplified to InstrMeta.Bad = BadInstruction; addInstruction(InstrMeta); if (BadInstruction) continue;

tools/llvm-cfi-verify/FileVerifier.h
46

This class seems like it's a whole program disassembly with a print method attached to report CFI status, does that seem fair? Would it make sense to rename it and split out the print method?

54

Should this be a ptr/ref to avoid duplicating memory? Or should there just be a method that gets the SectionName by iterating through the Sections instead of storing it for every Instr?

55

What is bad?

66

Could we also include copy & move assignment constructors

80

typo: Retusn

tools/llvm-cfi-verify/unittests/FileVerifier.cpp
66

This is run before every test, is that what you intended or is this meant to run only once (e.g. SetUpTestCase)?

68

Is it possible this will mask failures? Other unit tests seem to eschew error checking here, presumably allowing unit tests to fail later.

hctim updated this revision to Diff 117715.Oct 4 2017, 12:47 PM
hctim marked 8 inline comments as done.
  • /s/FileVerifier/FileAnalysis, removed Instr::SectionName, other comments from vlad.tsyrklevich
hctim added inline comments.Oct 4 2017, 12:47 PM
tools/llvm-cfi-verify/FileVerifier.h
54

Removed (was useful in debugging).

55

Changed to Instr::Valid.

66

Move assignment is implemented (=default), copy assignment is deleted as each FileVerifier::Object is actually owned by the parent FileVerifier::Binary, and there is no copy construction allowed of object::Binary.

hctim updated this revision to Diff 117726.Oct 4 2017, 1:17 PM
  • Added file header.

Also waiting to hear your response on the 2 unit tests questions.

tools/llvm-cfi-verify/FileAnalysis.cpp
63 ↗(On Diff #117726)

nit: if (auto InitResponse = ...) here? seems a little cleaner to me

67 ↗(On Diff #117726)

ditto

98 ↗(On Diff #117726)

nit: remove line

145 ↗(On Diff #117726)

nit:get rid of line

tools/llvm-cfi-verify/FileAnalysis.h
140 ↗(On Diff #117726)

I kept coming back to this trying to figure out what IndirectInstructions were, rename to IndirectBranches/getIndirectBranches()?

hctim marked 7 inline comments as done.Oct 9 2017, 9:56 AM
hctim added inline comments.
tools/llvm-cfi-verify/FileAnalysis.h
140 ↗(On Diff #117726)

I've been trying to keep the rhetoric 'indirect instructions' to cover both indirect branches and indirect calls.

Would you prefer IndirectCallsAndJumps?

tools/llvm-cfi-verify/unittests/FileVerifier.cpp
66

Yep, was intended. Setup of the class via the test constructor is pretty cheap and an easy way to reset the internal members to start with a clean slate each time.

68

I don't think so, given that we're exiting on a nonzero error code. I've changed this to gtest's FAIL() macro instead though so GTEST can clean up after itself and show the problems in a pretty red colour.

hctim updated this revision to Diff 118222.Oct 9 2017, 9:56 AM
hctim marked 3 inline comments as done.

Updated for Vlad's comments round 2.

hctim updated this revision to Diff 118253.Oct 9 2017, 12:36 PM

Backported usesRegisterOperand.

vlad.tsyrklevich accepted this revision.Oct 10 2017, 8:11 AM
vlad.tsyrklevich added inline comments.
tools/llvm-cfi-verify/FileAnalysis.cpp
257 ↗(On Diff #118253)

Is it useful to assert() that an instruction isn't being overwritten here?

tools/llvm-cfi-verify/FileAnalysis.h
140 ↗(On Diff #117726)

It's a bit unwieldy, lets leave as is.

This revision is now accepted and ready to land.Oct 10 2017, 8:11 AM
hctim updated this revision to Diff 118421.Oct 10 2017, 10:42 AM
hctim marked 2 inline comments as done.

Added assertions on instruction insertion.

This revision was automatically updated to reflect the committed changes.
hctim reopened this revision.Oct 11 2017, 11:38 AM

Reopened for resubmission as it was rolled back in rL315370.

Fixed the unused variable warning when assertions are disabled.

Also realised that unit tests were being built as part of check-* but not executed. Have integrated the unit tests into check-* build targets (required a reasonable refactor of file locations and the associated build file changes).

This revision is now accepted and ready to land.Oct 11 2017, 11:38 AM
hctim updated this revision to Diff 118658.Oct 11 2017, 11:38 AM

[code diff updated]

hctim updated this revision to Diff 118659.Oct 11 2017, 11:39 AM

/s/Verifier/Analysis

hctim updated this revision to Diff 118662.Oct 11 2017, 11:43 AM

clang-format over everything.

This revision was automatically updated to reflect the committed changes.

Please note that this patch causes a build failure to me in case of usage -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON.
I'm trying to figure out whether it is my configuration problem or patch problem.

Could you please try to run LLVM build with -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON? May be it is reproducible on your side?

Just to clarify what error I get for the command make check:
CMakeFiles/CFIVerifyTests.dir/FileAnalysis.cpp.o: In function `llvm::cfi_verify::(anonymous namespace)::ELFx86TestFileAnalysis::ELFx86TestFileAnalysis()':
/home/skatkov/work/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp:50: undefined reference to `llvm::cfi_verify::FileAnalysis::FileAnalysis(llvm::Triple const&, llvm::SubtargetFeatures const&)'
CMakeFiles/CFIVerifyTests.dir/FileAnalysis.cpp.o: In function `llvm::cfi_verify::(anonymous namespace)::ELFx86TestFileAnalysis::parseSectionContents(llvm::ArrayRef<unsigned char>, unsigned long)':
/home/skatkov/work/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp:55: undefined reference to `llvm::cfi_verify::FileAnalysis::parseSectionContents(llvm::ArrayRef<unsigned char>, unsigned long)'
CMakeFiles/CFIVerifyTests.dir/FileAnalysis.cpp.o: In function `llvm::cfi_verify::(anonymous namespace)::ELFx86TestFileAnalysis::initialiseDisassemblyMembers()':
/home/skatkov/work/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp:59: undefined reference to `llvm::cfi_verify::FileAnalysis::initialiseDisassemblyMembers()'
CMakeFiles/CFIVerifyTests.dir/FileAnalysis.cpp.o: In function `llvm::cfi_verify::(anonymous namespace)::BasicFileAnalysisTest_BasicDisassemblyTraversalTest_Test::TestBody()':
/home/skatkov/work/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp:89: undefined reference to `llvm::cfi_verify::FileAnalysis::getInstruction(unsigned long) const'
/home/skatkov/work/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp:90: undefined reference to `llvm::cfi_verify::FileAnalysis::getInstruction(unsigned long) const
....

Please note that this patch causes a build failure to me in case of usage -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON.
I'm trying to figure out whether it is my configuration problem or patch problem.

Could you please try to run LLVM build with -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON? May be it is reproducible on your side?

Hi Serguei,

I can't seem to repro with those two flags on. Is it possible for you to attack your CMakeCache.txt? What commit # are you at please?

Thanks.

Please note that this patch causes a build failure to me in case of usage -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON.
I'm trying to figure out whether it is my configuration problem or patch problem.

Could you please try to run LLVM build with -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON? May be it is reproducible on your side?

Hi Serguei,

I can't seem to repro with those two flags on. Is it possible for you to attack your CMakeCache.txt? What commit # are you at please?

Thanks.

Hi Mitch,

I'm on HEAD, so the most fresh version of LLVM trunk.

My current investigation shows the following. The issue is not reproducible on some hosts while is reproducible on many others.
I guess there is a race condition in a build. Specifically,

the unit test requires CFIVerify library be available. When we use -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON the test expects to find the required symbols in LLVM shared library.
In case make check passes (unit test is compiled) I see these symbols in LLVM shared libraries while when test compilation fails there is no these symbols in shared LLVM library.

The difference I see between these cases is that LLVM library is added to global property LLVM_LIBS before or after CFIVerify library.
In case LLVM shared library is added later, cmake knows about CFIVerify and generates dependency betweem LLVM and CFIVerify, so CFIVerify goes to LLVM shared library and compilation of unittest passes.
In case LLVM shared library is added earlier than CFIVerify, cmake does not know about CFIVerify and I've got a failure.

So it seems there is a some race condition of traversing libraries and I do not know why.

I guess that movement of CFIVerify library to lib directory should resolve the issue.

I want to understand how the order of traverse is defined to understand conditions to reproduce the issue.

For a while, does not it make sense to move CFI library to lib directory?

skatkov added a comment.EditedOct 16 2017, 10:59 PM

I confirm that sub-directory traverse order is the reason of unittest compilation failure.
In good case llvm-cfi-verify is handled earlier than llvm-shlib while in bad case otherwise.

The traverse order depends on this command:

file(GLOB sub-dirs "${CMAKE_CURRENT_SOURCE_DIR}/*")

in function(llvm_add_implicit_projects project)

The following workaround

diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index b654b8c..da62e35 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -47,6 +47,7 @@ add_llvm_external_project(llgo)
 add_llvm_external_project(lld)
 add_llvm_external_project(lldb)

+add_llvm_subdirectory(LLVM TOOL llvm-cfi-verify llvm-cfi-verify)
 # Automatically add remaining sub-directories containing a 'CMakeLists.txt'
 # file as external projects.
 add_llvm_implicit_projects()

fixes the issue for me because I force handling of CFIVerify earlier than LLVM shared library.

Please fix this asap, it really blocks me. I see three ways to fix it:

  1. Move CFIVerify library to top-level lib directory
  2. Find a way to prefer CFIVerify library to LLVM shared library
  3. Revert the patch.

Hey Serguei,

Unfortunately I don't have access to a computer to update this patch currently, but will look at resolving it first thing tomorrow.

Cheers,
Mitch.

Thank you very much!

@skatkov FYI I had the same problem and updating cmake to the latest version solved the issue.

hctim added a comment.EditedOct 17 2017, 11:37 AM

Just confirming that I can now reproduce this by forcing llvm-cfi-verify to build last in the provided glob of tools (simply add 'zzzzz' as a prefix to the directory name and boom!).

I'm a little confused how this causes cmake to fail, given that glob order is seemingly defined to be lexicographically descending, but given @aheejin 's comment I think it might be something to do with different cmake versions treating subdirectories differently.

I've tried to avoid putting the libraries for llvm-cfi-verify into lib/ as they should only be used by this tool. The only reason why they're a library in the first place is they need to be accessed from unittests/, as previously when the unit tests were in the tools/llvm-cfi-verify directory they were built but not executed as part of check-all.

My thoughts are to basically have a similar solution to your current workaround, with a bit more context in a comment above. I'll send it through to you in a moment for review, I'm just doing some verification on my end that this resolves the issue.

hctim added a comment.Oct 17 2017, 2:17 PM

Just for closure, the fix for this build issue is located over at D39020. Please direct all discussion over there :)