Classify llvm-cfi-verify.

[hctim]

Summary

Move llvm-cfi-verify into a class in preparation for CFI analysis to come.

[hctim]

Removed commented-out line from unittests/CMakeListst.txt

[vlad.tsyrklevich]

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?

[hctim]

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]

Readded <cstdlib> as an include for EXIT_SUCCESS;

[hctim]

Re-added basic disassembly traversal unit tests.

[hctim]

Added prev and next tests from bad instructions.

[hctim]

Updating base.

[hctim]

• fixed warnings

[vlad.tsyrklevich]

tools/llvm-cfi-verify/FileVerifier.cpp

1 ↗

(On Diff #117444)

Missing header

73 ↗

(On Diff #117444)

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

108 ↗

(On Diff #117211)

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

233 ↗

(On Diff #117211)

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

tools/llvm-cfi-verify/FileVerifier.h

45 ↗

(On Diff #117444)

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?

53 ↗

(On Diff #117444)

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?

54 ↗

(On Diff #117444)

What is bad?

65 ↗

(On Diff #117211)

Could we also include copy & move assignment constructors

79 ↗

(On Diff #117211)

typo: Retusn

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

65 ↗

(On Diff #117444)

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

67 ↗

(On Diff #117444)

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

[hctim]

• /s/FileVerifier/FileAnalysis, removed Instr::SectionName, other comments from vlad.tsyrklevich

[hctim]

tools/llvm-cfi-verify/FileVerifier.h

53 ↗

(On Diff #117444)

Removed (was useful in debugging).

54 ↗

(On Diff #117444)

Changed to Instr::Valid.

65 ↗

(On Diff #117211)

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]

• Added file header.

[vlad.tsyrklevich]

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]

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

65 ↗

(On Diff #117444)

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.

67 ↗

(On Diff #117444)

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 for Vlad's comments round 2.

[hctim]

Backported usesRegisterOperand.

[vlad.tsyrklevich]

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.

[hctim]

Added assertions on instruction insertion.

[hctim]

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).

[hctim]

[code diff updated]

[hctim]

/s/Verifier/Analysis

[hctim]

clang-format over everything.

[skatkov]

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?

[skatkov]

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
....

[hctim]

In D38379#898339, @skatkov wrote:

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.

[skatkov]

In D38379#898719, @hctim wrote:

In D38379#898339, @skatkov wrote:

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]

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.

[hctim]

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]

Thank you very much!

[aheejin]

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

[hctim]

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]

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

llvm/trunk/tools/llvm-cfi-verify/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   AllTargetsAsmPrinters
   AllTargetsAsmParsers
   AllTargetsDescs
   AllTargetsDisassemblers
   AllTargetsInfos
   MC
-  Object
   MCParser
   Object
   Support
   )
 
 add_llvm_tool(llvm-cfi-verify
   llvm-cfi-verify.cpp
+  lib/FileAnalysis.cpp
   )
+
+add_subdirectory(lib)

llvm/trunk/tools/llvm-cfi-verify/LLVMBuild.txt

 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
 [component_0]
 type = Tool
 name = llvm-cfi-verify
 parent = Tools
-required_libraries = MC MCDisassembler MCParser Support all-targets
+required_libraries = all-targets MC MCDisassembler MCParser Support

llvm/trunk/tools/llvm-cfi-verify/lib/CMakeLists.txt

+add_llvm_library(LLVMCFIVerify
+  FileAnalysis.cpp
+  FileAnalysis.h
+
+  LINK_COMPONENTS
+  MC
+  MCParser
+  Object
+  Support)

llvm/trunk/tools/llvm-cfi-verify/lib/FileAnalysis.h

+//===- FileAnalysis.h -------------------------------------------*- C++ -*-===//
+//
+//                      The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CFI_VERIFY_FILE_ANALYSIS_H
+#define LLVM_CFI_VERIFY_FILE_ANALYSIS_H
+
+#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCDisassembler/MCDisassembler.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstPrinter.h"
+#include "llvm/MC/MCInstrAnalysis.h"
+#include "llvm/MC/MCInstrDesc.h"
+#include "llvm/MC/MCInstrInfo.h"
+#include "llvm/MC/MCObjectFileInfo.h"
+#include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/Object/Binary.h"
+#include "llvm/Object/COFF.h"
+#include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Object/ObjectFile.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include <functional>
+#include <set>
+#include <string>
+#include <unordered_map>
+
+namespace llvm {
+namespace cfi_verify {
+
+// Disassembler and analysis tool for machine code files. Keeps track of non-
+// sequential control flows, including indirect control flow instructions.
+class FileAnalysis {
+public:
+  // A metadata struct for an instruction.
+  struct Instr {
+    uint64_t VMAddress;       // Virtual memory address of this instruction.
+    MCInst Instruction;       // Instruction.
+    uint64_t InstructionSize; // Size of this instruction.
+    bool Valid; // Is this a valid instruction? If false, Instr::Instruction is
+                // undefined.
+  };
+
+  // Construct a FileAnalysis from a file path.
+  static Expected<FileAnalysis> Create(StringRef Filename);
+
+  // Construct and take ownership of the supplied object. Do not use this
+  // constructor, prefer to use FileAnalysis::Create instead.
+  FileAnalysis(object::OwningBinary<object::Binary> Binary);
+  FileAnalysis() = delete;
+  FileAnalysis(const FileAnalysis &) = delete;
+  FileAnalysis(FileAnalysis &&Other) = default;
+
+  // Returns the instruction at the provided address. Returns nullptr if there
+  // is no instruction at the provided address.
+  const Instr *getInstruction(uint64_t Address) const;
+
+  // Returns the instruction at the provided adress, dying if the instruction is
+  // not found.
+  const Instr &getInstructionOrDie(uint64_t Address) const;
+
+  // Returns a pointer to the previous/next instruction in sequence,
+  // respectively. Returns nullptr if the next/prev instruction doesn't exist,
+  // or if the provided instruction doesn't exist.
+  const Instr *getPrevInstructionSequential(const Instr &InstrMeta) const;
+  const Instr *getNextInstructionSequential(const Instr &InstrMeta) const;
+
+  // Returns whether this instruction uses a register operand.
+  bool usesRegisterOperand(const Instr &InstrMeta) const;
+
+  // Returns the list of indirect instructions.
+  const std::set<uint64_t> &getIndirectInstructions() const;
+
+  const MCRegisterInfo *getRegisterInfo() const;
+  const MCInstrInfo *getMCInstrInfo() const;
+  const MCInstrAnalysis *getMCInstrAnalysis() const;
+
+protected:
+  // Construct a blank object with the provided triple and features. Used in
+  // testing, where a sub class will dependency inject protected methods to
+  // allow analysis of raw binary, without requiring a fully valid ELF file.
+  FileAnalysis(const Triple &ObjectTriple, const SubtargetFeatures &Features);
+
+  // Add an instruction to this object.
+  void addInstruction(const Instr &Instruction);
+
+  // Disassemble and parse the provided bytes into this object. Instruction
+  // address calculation is done relative to the provided SectionAddress.
+  void parseSectionContents(ArrayRef<uint8_t> SectionBytes,
+                            uint64_t SectionAddress);
+
+  // Constructs and initialises members required for disassembly.
+  Error initialiseDisassemblyMembers();
+
+  // Parses code sections from the internal object file. Saves them into the
+  // internal members. Should only be called once by Create().
+  Error parseCodeSections();
+
+private:
+  // Members that describe the input file.
+  object::OwningBinary<object::Binary> Binary;
+  const object::ObjectFile *Object = nullptr;
+  Triple ObjectTriple;
+  std::string ArchName;
+  std::string MCPU;
+  const Target *ObjectTarget = nullptr;
+  SubtargetFeatures Features;
+
+  // Members required for disassembly.
+  std::unique_ptr<const MCRegisterInfo> RegisterInfo;
+  std::unique_ptr<const MCAsmInfo> AsmInfo;
+  std::unique_ptr<MCSubtargetInfo> SubtargetInfo;
+  std::unique_ptr<const MCInstrInfo> MII;
+  MCObjectFileInfo MOFI;
+  std::unique_ptr<MCContext> Context;
+  std::unique_ptr<const MCDisassembler> Disassembler;
+  std::unique_ptr<const MCInstrAnalysis> MIA;
+  std::unique_ptr<MCInstPrinter> Printer;
+
+  // A mapping between the virtual memory address to the instruction metadata
+  // struct.
+  std::map<uint64_t, Instr> Instructions;
+
+  // Contains a mapping between a specific address, and a list of instructions
+  // that use this address as a branch target (including call instructions).
+  std::unordered_map<uint64_t, std::vector<uint64_t>> StaticBranchTargetings;
+
+  // A list of addresses of indirect control flow instructions.
+  std::set<uint64_t> IndirectInstructions;
+};
+
+class UnsupportedDisassembly : public ErrorInfo<UnsupportedDisassembly> {
+public:
+  static char ID;
+
+  void log(raw_ostream &OS) const override;
+  std::error_code convertToErrorCode() const override;
+};
+
+} // namespace cfi_verify
+} // namespace llvm
+
+#endif // LLVM_CFI_VERIFY_FILE_ANALYSIS_H

llvm/trunk/tools/llvm-cfi-verify/lib/FileAnalysis.cpp

+//===- FileAnalysis.cpp -----------------------------------------*- C++ -*-===//
+//
+//                      The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "FileAnalysis.h"
+
+#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCDisassembler/MCDisassembler.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstPrinter.h"
+#include "llvm/MC/MCInstrAnalysis.h"
+#include "llvm/MC/MCInstrDesc.h"
+#include "llvm/MC/MCInstrInfo.h"
+#include "llvm/MC/MCObjectFileInfo.h"
+#include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/Object/Binary.h"
+#include "llvm/Object/COFF.h"
+#include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Object/ObjectFile.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include <functional>
+
+using Instr = llvm::cfi_verify::FileAnalysis::Instr;
+
+namespace llvm {
+namespace cfi_verify {
+
+Expected<FileAnalysis> FileAnalysis::Create(StringRef Filename) {
+  // Open the filename provided.
+  Expected<object::OwningBinary<object::Binary>> BinaryOrErr =
+      object::createBinary(Filename);
+  if (!BinaryOrErr)
+    return BinaryOrErr.takeError();
+
+  // Construct the object and allow it to take ownership of the binary.
+  object::OwningBinary<object::Binary> Binary = std::move(BinaryOrErr.get());
+  FileAnalysis Analysis(std::move(Binary));
+
+  Analysis.Object = dyn_cast<object::ObjectFile>(Analysis.Binary.getBinary());
+  if (!Analysis.Object)
+    return make_error<UnsupportedDisassembly>();
+
+  Analysis.ObjectTriple = Analysis.Object->makeTriple();
+  Analysis.Features = Analysis.Object->getFeatures();
+
+  // Init the rest of the object.
+  if (auto InitResponse = Analysis.initialiseDisassemblyMembers())
+    return std::move(InitResponse);
+
+  if (auto SectionParseResponse = Analysis.parseCodeSections())
+    return std::move(SectionParseResponse);
+
+  return std::move(Analysis);
+}
+
+FileAnalysis::FileAnalysis(object::OwningBinary<object::Binary> Binary)
+    : Binary(std::move(Binary)) {}
+
+FileAnalysis::FileAnalysis(const Triple &ObjectTriple,
+                           const SubtargetFeatures &Features)
+    : ObjectTriple(ObjectTriple), Features(Features) {}
+
+const Instr *
+FileAnalysis::getPrevInstructionSequential(const Instr &InstrMeta) const {
+  std::map<uint64_t, Instr>::const_iterator KV =
+      Instructions.find(InstrMeta.VMAddress);
+  if (KV == Instructions.end() || KV == Instructions.begin())
+    return nullptr;
+
+  if (!(--KV)->second.Valid)
+    return nullptr;
+
+  return &KV->second;
+}
+
+const Instr *
+FileAnalysis::getNextInstructionSequential(const Instr &InstrMeta) const {
+  std::map<uint64_t, Instr>::const_iterator KV =
+      Instructions.find(InstrMeta.VMAddress);
+  if (KV == Instructions.end() || ++KV == Instructions.end())
+    return nullptr;
+
+  if (!KV->second.Valid)
+    return nullptr;
+
+  return &KV->second;
+}
+
+bool FileAnalysis::usesRegisterOperand(const Instr &InstrMeta) const {
+  for (const auto &Operand : InstrMeta.Instruction) {
+    if (Operand.isReg())
+      return true;
+  }
+  return false;
+}
+
+const Instr *FileAnalysis::getInstruction(uint64_t Address) const {
+  const auto &InstrKV = Instructions.find(Address);
+  if (InstrKV == Instructions.end())
+    return nullptr;
+
+  return &InstrKV->second;
+}
+
+const Instr &FileAnalysis::getInstructionOrDie(uint64_t Address) const {
+  const auto &InstrKV = Instructions.find(Address);
+  assert(InstrKV != Instructions.end() && "Address doesn't exist.");
+  return InstrKV->second;
+}
+
+const std::set<uint64_t> &FileAnalysis::getIndirectInstructions() const {
+  return IndirectInstructions;
+}
+
+const MCRegisterInfo *FileAnalysis::getRegisterInfo() const {
+  return RegisterInfo.get();
+}
+
+const MCInstrInfo *FileAnalysis::getMCInstrInfo() const { return MII.get(); }
+
+const MCInstrAnalysis *FileAnalysis::getMCInstrAnalysis() const {
+  return MIA.get();
+}
+
+Error FileAnalysis::initialiseDisassemblyMembers() {
+  std::string TripleName = ObjectTriple.getTriple();
+  ArchName = "";
+  MCPU = "";
+  std::string ErrorString;
+
+  ObjectTarget =
+      TargetRegistry::lookupTarget(ArchName, ObjectTriple, ErrorString);
+  if (!ObjectTarget)
+    return make_error<StringError>(Twine("Couldn't find target \"") +
+                                       ObjectTriple.getTriple() +
+                                       "\", failed with error: " + ErrorString,
+                                   inconvertibleErrorCode());
+
+  RegisterInfo.reset(ObjectTarget->createMCRegInfo(TripleName));
+  if (!RegisterInfo)
+    return make_error<StringError>("Failed to initialise RegisterInfo.",
+                                   inconvertibleErrorCode());
+
+  AsmInfo.reset(ObjectTarget->createMCAsmInfo(*RegisterInfo, TripleName));
+  if (!AsmInfo)
+    return make_error<StringError>("Failed to initialise AsmInfo.",
+                                   inconvertibleErrorCode());
+
+  SubtargetInfo.reset(ObjectTarget->createMCSubtargetInfo(
+      TripleName, MCPU, Features.getString()));
+  if (!SubtargetInfo)
+    return make_error<StringError>("Failed to initialise SubtargetInfo.",
+                                   inconvertibleErrorCode());
+
+  MII.reset(ObjectTarget->createMCInstrInfo());
+  if (!MII)
+    return make_error<StringError>("Failed to initialise MII.",
+                                   inconvertibleErrorCode());
+
+  Context.reset(new MCContext(AsmInfo.get(), RegisterInfo.get(), &MOFI));
+
+  Disassembler.reset(
+      ObjectTarget->createMCDisassembler(*SubtargetInfo, *Context));
+
+  if (!Disassembler)
+    return make_error<StringError>("No disassembler available for target",
+                                   inconvertibleErrorCode());
+
+  MIA.reset(ObjectTarget->createMCInstrAnalysis(MII.get()));
+
+  Printer.reset(ObjectTarget->createMCInstPrinter(
+      ObjectTriple, AsmInfo->getAssemblerDialect(), *AsmInfo, *MII,
+      *RegisterInfo));
+
+  return Error::success();
+}
+
+Error FileAnalysis::parseCodeSections() {
+  for (const object::SectionRef &Section : Object->sections()) {
+    // Ensure only executable sections get analysed.
+    if (!(object::ELFSectionRef(Section).getFlags() & ELF::SHF_EXECINSTR))
+      continue;
+
+    StringRef SectionContents;
+    if (Section.getContents(SectionContents))
+      return make_error<StringError>("Failed to retrieve section contents",
+                                     inconvertibleErrorCode());
+
+    ArrayRef<uint8_t> SectionBytes((const uint8_t *)SectionContents.data(),
+                                   Section.getSize());
+    parseSectionContents(SectionBytes, Section.getAddress());
+  }
+  return Error::success();
+}
+
+void FileAnalysis::parseSectionContents(ArrayRef<uint8_t> SectionBytes,
+                                        uint64_t SectionAddress) {
+  MCInst Instruction;
+  Instr InstrMeta;
+  uint64_t InstructionSize;
+
+  for (uint64_t Byte = 0; Byte < SectionBytes.size();) {
+    bool ValidInstruction =
+        Disassembler->getInstruction(Instruction, InstructionSize,
+                                     SectionBytes.drop_front(Byte), 0, nulls(),
+                                     outs()) == MCDisassembler::Success;
+
+    Byte += InstructionSize;
+
+    uint64_t VMAddress = SectionAddress + Byte - InstructionSize;
+    InstrMeta.Instruction = Instruction;
+    InstrMeta.VMAddress = VMAddress;
+    InstrMeta.InstructionSize = InstructionSize;
+    InstrMeta.Valid = ValidInstruction;
+    addInstruction(InstrMeta);
+
+    if (!ValidInstruction)
+      continue;
+
+    // Skip additional parsing for instructions that do not affect the control
+    // flow.
+    const auto &InstrDesc = MII->get(Instruction.getOpcode());
+    if (!InstrDesc.mayAffectControlFlow(Instruction, *RegisterInfo))
+      continue;
+
+    uint64_t Target;
+    if (MIA->evaluateBranch(Instruction, VMAddress, InstructionSize, Target)) {
+      // If the target can be evaluated, it's not indirect.
+      StaticBranchTargetings[Target].push_back(VMAddress);
+      continue;
+    }
+
+    if (!usesRegisterOperand(InstrMeta))
+      continue;
+
+    IndirectInstructions.insert(VMAddress);
+  }
+}
+
+void FileAnalysis::addInstruction(const Instr &Instruction) {
+  const auto &KV =
+      Instructions.insert(std::make_pair(Instruction.VMAddress, Instruction));
+  if (!KV.second) {
+    errs() << "Failed to add instruction at address "
+           << format_hex(Instruction.VMAddress, 2)
+           << ": Instruction at this address already exists.\n";
+    exit(EXIT_FAILURE);
+  }
+}
+
+char UnsupportedDisassembly::ID;
+void UnsupportedDisassembly::log(raw_ostream &OS) const {
+  OS << "Dissassembling of non-objects not currently supported.\n";
+}
+
+std::error_code UnsupportedDisassembly::convertToErrorCode() const {
+  return std::error_code();
+}
+
+} // namespace cfi_verify
+} // namespace llvm

llvm/trunk/tools/llvm-cfi-verify/lib/LLVMBuild.txt

+;===- ./tools/llvm-cfi-verify/lib/LLVMBuild.txt ----------------*- Conf -*--===;
+;
+;                     The LLVM Compiler Infrastructure
+;
+; This file is distributed under the University of Illinois Open Source
+; License. See LICENSE.TXT for details.
+;
+;===------------------------------------------------------------------------===;
+;
+; This is an LLVMBuild description file for the components in this subdirectory.
+;
+; For more information on the LLVMBuild system, please see:
+;
+;   http://llvm.org/docs/LLVMBuild.html
+;
+;===------------------------------------------------------------------------===;
+
+[component_0]
+type = Library
+name = CFIVerify
+parent = Libraries
+required_libraries = MC MCDisassembler MCParser Support

llvm/trunk/tools/llvm-cfi-verify/llvm-cfi-verify.cpp

 // information.
 //
 // This tool is currently incomplete. It currently only does disassembly for
 // object files, and searches through the code for indirect control flow
 // instructions, printing them once found.
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/MC/MCAsmInfo.h"
-#include "llvm/MC/MCContext.h"
-#include "llvm/MC/MCDisassembler/MCDisassembler.h"
-#include "llvm/MC/MCInst.h"
-#include "llvm/MC/MCInstPrinter.h"
-#include "llvm/MC/MCInstrAnalysis.h"
-#include "llvm/MC/MCInstrDesc.h"
-#include "llvm/MC/MCInstrInfo.h"
-#include "llvm/MC/MCObjectFileInfo.h"
-#include "llvm/MC/MCRegisterInfo.h"
-#include "llvm/MC/MCSubtargetInfo.h"
-#include "llvm/Object/Binary.h"
-#include "llvm/Object/COFF.h"
-#include "llvm/Object/ObjectFile.h"
-#include "llvm/Support/Casting.h"
+#include "lib/FileAnalysis.h"
+
+#include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Support/CommandLine.h"
-#include "llvm/Support/MemoryBuffer.h"
-#include "llvm/Support/TargetRegistry.h"
-#include "llvm/Support/TargetSelect.h"
-#include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/Error.h"
 
-#include <cassert>
 #include <cstdlib>
 
 using namespace llvm;
 using namespace llvm::object;
+using namespace llvm::cfi_verify;
 
-cl::opt<bool> ArgDumpSymbols("sym", cl::desc("Dump the symbol table."));
 cl::opt<std::string> InputFilename(cl::Positional, cl::desc("<input file>"),
                                    cl::Required);
 
-static void printSymbols(const ObjectFile *Object) {
-  for (const SymbolRef &Symbol : Object->symbols()) {
-    outs() << "Symbol [" << format_hex_no_prefix(Symbol.getValue(), 2)
-           << "] = ";
-
-    auto SymbolName = Symbol.getName();
-    if (SymbolName)
-      outs() << *SymbolName;
-    else
-      outs() << "UNKNOWN";
-
-    if (Symbol.getFlags() & SymbolRef::SF_Hidden)
-      outs() << " .hidden";
-
-    outs() << " (Section = ";
-
-    auto SymbolSection = Symbol.getSection();
-    if (SymbolSection) {
-      StringRef SymbolSectionName;
-      if ((*SymbolSection)->getName(SymbolSectionName))
-        outs() << "UNKNOWN)";
-      else
-        outs() << SymbolSectionName << ")";
-    } else {
-      outs() << "N/A)";
-    }
+ExitOnError ExitOnErr;
 
+void printIndirectCFInstructions(const FileAnalysis &Verifier) {
+  for (uint64_t Address : Verifier.getIndirectInstructions()) {
+    const auto &InstrMeta = Verifier.getInstructionOrDie(Address);
+    outs() << format_hex(Address, 2) << " |"
+           << Verifier.getMCInstrInfo()->getName(
+                  InstrMeta.Instruction.getOpcode())
+           << " ";
+    InstrMeta.Instruction.print(outs());
     outs() << "\n";
   }
 }
 
 int main(int argc, char **argv) {
   cl::ParseCommandLineOptions(argc, argv);
 
   InitializeAllTargetInfos();
   InitializeAllTargetMCs();
   InitializeAllAsmParsers();
   InitializeAllDisassemblers();
 
-  Expected<OwningBinary<Binary>> BinaryOrErr = createBinary(InputFilename);
-  if (!BinaryOrErr) {
-    errs() << "Failed to open file.\n";
-    return EXIT_FAILURE;
-  }
-
-  Binary &Binary = *BinaryOrErr.get().getBinary();
-  ObjectFile *Object = dyn_cast<ObjectFile>(&Binary);
-  if (!Object) {
-    errs() << "Disassembling of non-objects not currently supported.\n";
-    return EXIT_FAILURE;
-  }
-
-  Triple TheTriple = Object->makeTriple();
-  std::string TripleName = TheTriple.getTriple();
-  std::string ArchName = "";
-  std::string ErrorString;
-
-  const Target *TheTarget =
-      TargetRegistry::lookupTarget(ArchName, TheTriple, ErrorString);
-
-  if (!TheTarget) {
-    errs() << "Couldn't find target \"" << TheTriple.getTriple()
-           << "\", failed with error: " << ErrorString << ".\n";
-    return EXIT_FAILURE;
-  }
-
-  SubtargetFeatures Features = Object->getFeatures();
-
-  std::unique_ptr<const MCRegisterInfo> RegisterInfo(
-      TheTarget->createMCRegInfo(TripleName));
-  if (!RegisterInfo) {
-    errs() << "Failed to initialise RegisterInfo.\n";
-    return EXIT_FAILURE;
-  }
-
-  std::unique_ptr<const MCAsmInfo> AsmInfo(
-      TheTarget->createMCAsmInfo(*RegisterInfo, TripleName));
-  if (!AsmInfo) {
-    errs() << "Failed to initialise AsmInfo.\n";
-    return EXIT_FAILURE;
-  }
-
-  std::string MCPU = "";
-  std::unique_ptr<MCSubtargetInfo> SubtargetInfo(
-      TheTarget->createMCSubtargetInfo(TripleName, MCPU, Features.getString()));
-  if (!SubtargetInfo) {
-    errs() << "Failed to initialise SubtargetInfo.\n";
-    return EXIT_FAILURE;
-  }
-
-  std::unique_ptr<const MCInstrInfo> MII(TheTarget->createMCInstrInfo());
-  if (!MII) {
-    errs() << "Failed to initialise MII.\n";
-    return EXIT_FAILURE;
-  }
-
-  MCObjectFileInfo MOFI;
-  MCContext Context(AsmInfo.get(), RegisterInfo.get(), &MOFI);
-
-  std::unique_ptr<const MCDisassembler> Disassembler(
-      TheTarget->createMCDisassembler(*SubtargetInfo, Context));
-
-  if (!Disassembler) {
-    errs() << "No disassembler available for target.";
-    return EXIT_FAILURE;
-  }
-
-  std::unique_ptr<const MCInstrAnalysis> MIA(
-      TheTarget->createMCInstrAnalysis(MII.get()));
-
-  std::unique_ptr<MCInstPrinter> Printer(
-      TheTarget->createMCInstPrinter(TheTriple, AsmInfo->getAssemblerDialect(),
-                                     *AsmInfo, *MII, *RegisterInfo));
-
-  if (ArgDumpSymbols)
-    printSymbols(Object);
-
-  for (const SectionRef &Section : Object->sections()) {
-    outs() << "Section [" << format_hex_no_prefix(Section.getAddress(), 2)
-           << "] = ";
-    StringRef SectionName;
-
-    if (Section.getName(SectionName))
-      outs() << "UNKNOWN.\n";
-    else
-      outs() << SectionName << "\n";
-
-    StringRef SectionContents;
-    if (Section.getContents(SectionContents)) {
-      errs() << "Failed to retrieve section contents.\n";
-      return EXIT_FAILURE;
-    }
-
-    MCInst Instruction;
-    uint64_t InstructionSize;
-
-    ArrayRef<uint8_t> SectionBytes((const uint8_t *)SectionContents.data(),
-                                   Section.getSize());
-
-    for (uint64_t Byte = 0; Byte < Section.getSize();) {
-      bool BadInstruction = false;
-
-      // Disassemble the instruction.
-      if (Disassembler->getInstruction(
-              Instruction, InstructionSize, SectionBytes.drop_front(Byte), 0,
-              nulls(), outs()) != MCDisassembler::Success) {
-        BadInstruction = true;
-      }
-
-      Byte += InstructionSize;
-
-      if (BadInstruction)
-        continue;
-
-      // Skip instructions that do not affect the control flow.
-      const auto &InstrDesc = MII->get(Instruction.getOpcode());
-      if (!InstrDesc.mayAffectControlFlow(Instruction, *RegisterInfo))
-        continue;
-
-      // Skip instructions that do not operate on register operands.
-      bool UsesRegisterOperand = false;
-      for (const auto &Operand : Instruction) {
-        if (Operand.isReg())
-          UsesRegisterOperand = true;
-      }
-
-      if (!UsesRegisterOperand)
-        continue;
-
-      // Print the instruction address.
-      outs() << "    "
-             << format_hex(Section.getAddress() + Byte - InstructionSize, 2)
-             << ": ";
-
-      // Print the instruction bytes.
-      for (uint64_t i = 0; i < InstructionSize; ++i) {
-        outs() << format_hex_no_prefix(SectionBytes[Byte - InstructionSize + i],
-                                       2)
-               << " ";
-      }
-
-      // Print the instruction.
-      outs() << " | " << MII->getName(Instruction.getOpcode()) << " ";
-      Instruction.dump_pretty(outs(), Printer.get());
-
-      outs() << "\n";
-    }
-  }
+  FileAnalysis Verifier = ExitOnErr(FileAnalysis::Create(InputFilename));
+  printIndirectCFInstructions(Verifier);
 
   return EXIT_SUCCESS;
 }

llvm/trunk/unittests/CMakeLists.txt

 add_subdirectory(BinaryFormat)
 add_subdirectory(ObjectYAML)
 add_subdirectory(Option)
 add_subdirectory(ProfileData)
 add_subdirectory(Support)
 add_subdirectory(Target)
 add_subdirectory(Transforms)
 add_subdirectory(XRay)
+add_subdirectory(tools)

llvm/trunk/unittests/tools/CMakeLists.txt

+add_subdirectory(llvm-cfi-verify)

llvm/trunk/unittests/tools/llvm-cfi-verify/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  AllTargetsAsmPrinters
+  AllTargetsAsmParsers
+  AllTargetsDescs
+  AllTargetsDisassemblers
+  AllTargetsInfos
+  CFIVerify
+  MC
+  MCParser
+  Object
+  Support
+  )
+
+add_llvm_unittest(CFIVerifyTests
+  FileAnalysis.cpp)

llvm/trunk/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp

+//===- llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp --------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "../tools/llvm-cfi-verify/lib/FileAnalysis.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCDisassembler/MCDisassembler.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstPrinter.h"
+#include "llvm/MC/MCInstrAnalysis.h"
+#include "llvm/MC/MCInstrDesc.h"
+#include "llvm/MC/MCInstrInfo.h"
+#include "llvm/MC/MCObjectFileInfo.h"
+#include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/Object/Binary.h"
+#include "llvm/Object/COFF.h"
+#include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Object/ObjectFile.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include <cstdlib>
+
+using Instr = ::llvm::cfi_verify::FileAnalysis::Instr;
+using ::testing::Eq;
+
+namespace llvm {
+namespace cfi_verify {
+namespace {
+class ELFx86TestFileAnalysis : public FileAnalysis {
+public:
+  ELFx86TestFileAnalysis()
+      : FileAnalysis(Triple("x86_64--"), SubtargetFeatures()) {}
+
+  // Expose this method publicly for testing.
+  void parseSectionContents(ArrayRef<uint8_t> SectionBytes,
+                            uint64_t SectionAddress) {
+    FileAnalysis::parseSectionContents(SectionBytes, SectionAddress);
+  }
+
+  Error initialiseDisassemblyMembers() {
+    return FileAnalysis::initialiseDisassemblyMembers();
+  }
+};
+
+class BasicFileAnalysisTest : public ::testing::Test {
+protected:
+  virtual void SetUp() {
+    if (Analysis.initialiseDisassemblyMembers()) {
+      FAIL() << "Failed to initialise FileAnalysis.";
+    }
+  }
+
+  ELFx86TestFileAnalysis Analysis;
+};
+
+TEST_F(BasicFileAnalysisTest, BasicDisassemblyTraversalTest) {
+  Analysis.parseSectionContents(
+      {
+          0x90,                   // 0: nop
+          0xb0, 0x00,             // 1: mov $0x0, %al
+          0x48, 0x89, 0xe5,       // 3: mov %rsp, %rbp
+          0x48, 0x83, 0xec, 0x18, // 6: sub $0x18, %rsp
+          0x48, 0xbe, 0xc4, 0x07, 0x40,
+          0x00, 0x00, 0x00, 0x00, 0x00, // 10: movabs $0x4007c4, %rsi
+          0x2f,                         // 20: (bad)
+          0x41, 0x0e,                   // 21: rex.B (bad)
+          0x62, 0x72, 0x65, 0x61, 0x6b, // 23: (bad) {%k1}
+      },
+      0xDEADBEEF);
+
+  EXPECT_EQ(nullptr, Analysis.getInstruction(0x0));
+  EXPECT_EQ(nullptr, Analysis.getInstruction(0x1000));
+
+  // 0xDEADBEEF: nop
+  const auto *InstrMeta = Analysis.getInstruction(0xDEADBEEF);
+  EXPECT_NE(nullptr, InstrMeta);
+  EXPECT_EQ(0xDEADBEEF, InstrMeta->VMAddress);
+  EXPECT_EQ(1u, InstrMeta->InstructionSize);
+  EXPECT_TRUE(InstrMeta->Valid);
+
+  const auto *NextInstrMeta = Analysis.getNextInstructionSequential(*InstrMeta);
+  EXPECT_EQ(nullptr, Analysis.getPrevInstructionSequential(*InstrMeta));
+  const auto *PrevInstrMeta = InstrMeta;
+
+  // 0xDEADBEEF + 1: mov $0x0, %al
+  InstrMeta = Analysis.getInstruction(0xDEADBEEF + 1);
+  EXPECT_NE(nullptr, InstrMeta);
+  EXPECT_EQ(NextInstrMeta, InstrMeta);
+  EXPECT_EQ(0xDEADBEEF + 1, InstrMeta->VMAddress);
+  EXPECT_EQ(2u, InstrMeta->InstructionSize);
+  EXPECT_TRUE(InstrMeta->Valid);
+
+  NextInstrMeta = Analysis.getNextInstructionSequential(*InstrMeta);
+  EXPECT_EQ(PrevInstrMeta, Analysis.getPrevInstructionSequential(*InstrMeta));
+  PrevInstrMeta = InstrMeta;
+
+  // 0xDEADBEEF + 3: mov %rsp, %rbp
+  InstrMeta = Analysis.getInstruction(0xDEADBEEF + 3);
+  EXPECT_NE(nullptr, InstrMeta);
+  EXPECT_EQ(NextInstrMeta, InstrMeta);
+  EXPECT_EQ(0xDEADBEEF + 3, InstrMeta->VMAddress);
+  EXPECT_EQ(3u, InstrMeta->InstructionSize);
+  EXPECT_TRUE(InstrMeta->Valid);
+
+  NextInstrMeta = Analysis.getNextInstructionSequential(*InstrMeta);
+  EXPECT_EQ(PrevInstrMeta, Analysis.getPrevInstructionSequential(*InstrMeta));
+  PrevInstrMeta = InstrMeta;
+
+  // 0xDEADBEEF + 6: sub $0x18, %rsp
+  InstrMeta = Analysis.getInstruction(0xDEADBEEF + 6);
+  EXPECT_NE(nullptr, InstrMeta);
+  EXPECT_EQ(NextInstrMeta, InstrMeta);
+  EXPECT_EQ(0xDEADBEEF + 6, InstrMeta->VMAddress);
+  EXPECT_EQ(4u, InstrMeta->InstructionSize);
+  EXPECT_TRUE(InstrMeta->Valid);
+
+  NextInstrMeta = Analysis.getNextInstructionSequential(*InstrMeta);
+  EXPECT_EQ(PrevInstrMeta, Analysis.getPrevInstructionSequential(*InstrMeta));
+  PrevInstrMeta = InstrMeta;
+
+  // 0xDEADBEEF + 10: movabs $0x4007c4, %rsi
+  InstrMeta = Analysis.getInstruction(0xDEADBEEF + 10);
+  EXPECT_NE(nullptr, InstrMeta);
+  EXPECT_EQ(NextInstrMeta, InstrMeta);
+  EXPECT_EQ(0xDEADBEEF + 10, InstrMeta->VMAddress);
+  EXPECT_EQ(10u, InstrMeta->InstructionSize);
+  EXPECT_TRUE(InstrMeta->Valid);
+
+  EXPECT_EQ(nullptr, Analysis.getNextInstructionSequential(*InstrMeta));
+  EXPECT_EQ(PrevInstrMeta, Analysis.getPrevInstructionSequential(*InstrMeta));
+  PrevInstrMeta = InstrMeta;
+
+  // 0xDEADBEEF + 20: (bad)
+  InstrMeta = Analysis.getInstruction(0xDEADBEEF + 20);
+  EXPECT_NE(nullptr, InstrMeta);
+  EXPECT_EQ(0xDEADBEEF + 20, InstrMeta->VMAddress);
+  EXPECT_EQ(1u, InstrMeta->InstructionSize);
+  EXPECT_FALSE(InstrMeta->Valid);
+
+  EXPECT_EQ(nullptr, Analysis.getNextInstructionSequential(*InstrMeta));
+  EXPECT_EQ(PrevInstrMeta, Analysis.getPrevInstructionSequential(*InstrMeta));
+
+  // 0xDEADBEEF + 21: rex.B (bad)
+  InstrMeta = Analysis.getInstruction(0xDEADBEEF + 21);
+  EXPECT_NE(nullptr, InstrMeta);
+  EXPECT_EQ(0xDEADBEEF + 21, InstrMeta->VMAddress);
+  EXPECT_EQ(2u, InstrMeta->InstructionSize);
+  EXPECT_FALSE(InstrMeta->Valid);
+
+  EXPECT_EQ(nullptr, Analysis.getNextInstructionSequential(*InstrMeta));
+  EXPECT_EQ(nullptr, Analysis.getPrevInstructionSequential(*InstrMeta));
+
+  // 0xDEADBEEF + 6: (bad) {%k1}
+  InstrMeta = Analysis.getInstruction(0xDEADBEEF + 23);
+  EXPECT_NE(nullptr, InstrMeta);
+  EXPECT_EQ(0xDEADBEEF + 23, InstrMeta->VMAddress);
+  EXPECT_EQ(5u, InstrMeta->InstructionSize);
+  EXPECT_FALSE(InstrMeta->Valid);
+
+  EXPECT_EQ(nullptr, Analysis.getNextInstructionSequential(*InstrMeta));
+  EXPECT_EQ(nullptr, Analysis.getPrevInstructionSequential(*InstrMeta));
+}
+
+TEST_F(BasicFileAnalysisTest, PrevAndNextFromBadInst) {
+  Analysis.parseSectionContents(
+      {
+          0x90, // 0: nop
+          0x2f, // 1: (bad)
+          0x90  // 2: nop
+      },
+      0xDEADBEEF);
+  const auto &BadInstrMeta = Analysis.getInstructionOrDie(0xDEADBEEF + 1);
+  const auto *GoodInstrMeta =
+      Analysis.getPrevInstructionSequential(BadInstrMeta);
+  EXPECT_NE(nullptr, GoodInstrMeta);
+  EXPECT_EQ(0xDEADBEEF, GoodInstrMeta->VMAddress);
+  EXPECT_EQ(1u, GoodInstrMeta->InstructionSize);
+
+  GoodInstrMeta = Analysis.getNextInstructionSequential(BadInstrMeta);
+  EXPECT_NE(nullptr, GoodInstrMeta);
+  EXPECT_EQ(0xDEADBEEF + 2, GoodInstrMeta->VMAddress);
+  EXPECT_EQ(1u, GoodInstrMeta->InstructionSize);
+}
+
+} // anonymous namespace
+} // end namespace cfi_verify
+} // end namespace llvm
+
+int main(int argc, char **argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  llvm::cl::ParseCommandLineOptions(argc, argv);
+
+  llvm::InitializeAllTargetInfos();
+  llvm::InitializeAllTargetMCs();
+  llvm::InitializeAllAsmParsers();
+  llvm::InitializeAllDisassemblers();
+
+  return RUN_ALL_TESTS();
+}