Move llvm-cfi-verify into a class in preparation for CFI analysis to come.
Details
Diff Detail
- Build Status
Buildable 10689 Build 10689: arc lint + arc unit
Event Timeline
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?
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.
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. |
- /s/FileVerifier/FileAnalysis, removed Instr::SectionName, other comments from vlad.tsyrklevich
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. |
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()? |
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. |
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).
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
....
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?
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:
- Move CFIVerify library to top-level lib directory
- Find a way to prefer CFIVerify library to LLVM shared library
- 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.
@skatkov FYI I had the same problem and updating cmake to the latest version solved the issue.
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.
Just for closure, the fix for this build issue is located over at D39020. Please direct all discussion over there :)
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?