This is an archive of the discontinued LLVM Phabricator instance.

[cfi-verify] Support AArch64.
ClosedPublic

Authored by jgalenson on Jul 2 2018, 9:19 AM.

Details

Summary

This patch adds support for AArch64 to cfi-verify.

This required three changes to cfi-verify. First, it generalizes checking if an instruction is a trap by adding a new isTrap flag to TableGen (and defining it for x86 and AArch64).

Second, the code that ensures that the operand register is not clobbered between the CFI check and the indirect call needs to allow a single dereference (in x86 this happens as part of the jump instruction, but not in AArch64). So I modified the code to walk backwards from the indirect call and look for multiple loads.

Third, we needed to ensure that return instructions are not counted as indirect branches. Technically, returns are indirect branches and can be covered by CFI, but LLVM's forward-edge CFI does not protect them, and x86 does not consider them, so we keep that behavior (this might also be worth discussing, however).

In addition, we had to improve AArch64's code to evaluate the branch target of a MCInst to handle calls where the destination is not the first operand (which it often is not).

Diff Detail

Event Timeline

jgalenson created this revision.Jul 2 2018, 9:19 AM
vlad.tsyrklevich added subscribers: yursha, kcc.

Hi Joel, thanks for taking this on! Other than the comments mentioned inline, I'd also like to see a couple more tests, in particular:

  • Currently there is a CFI-protected tiny.cc example, you should also an unprotected example.
  • Given the special handling for vtable loads, it would be good to add a similar simple 'vtable.cc' example with a protected and an unprotected test to make sure that logic doesn't regress.

There are also unittests for llvm-cfi-verify. It would be worth adding a couple of tests for the 'can load' vtable logic to the FileAnalysis tests.

tools/llvm-cfi-verify/lib/FileAnalysis.cpp
157–158

Instead of doing string comparisons here for every valid value, lets define a TrapOpcode variable during initialization to X86::TRAP / AArch64::BRK depending on the target. (We can bail on other architectures that we haven't explicitly tested on to avoid confusing users who might otherwise believe the results.)

280

Update the header comment for this to mention the new clobber evaluation logic.

301

I believe the loop to compute this below can be replaced by Graph.flattenAddress(Node)

313–315

Any reason to not use a range-for here?

318

Invert the if condition here so that it's

if (...)
  return
....
319

This variable is set outside of the for loop so it will still be false for the next conditional branch node we iterate over. Please add a test for this case.

pcc added inline comments.Jul 3 2018, 2:37 PM
tools/llvm-cfi-verify/lib/FileAnalysis.cpp
157–158

I don't think we can access backend-specific things like X86::TRAP here though. It might be worth adding an interface to MCInstrAnalysis for asking what the trap instruction is and have that assert on architectures that we don't support yet.

Thanks for the comments! I'll try to update the patch on Friday.

Sorry for the lack of tests; I was having trouble compiling testcases the right way, but I think I have it figured out now.

tools/llvm-cfi-verify/lib/FileAnalysis.cpp
157–158

I had envisioned adding a new entry to MCID::Flag so that instruction definitions in .td files could contain IsTrap = 1 on the trap instructions. And as I said, that seemed the cleanest and most correct option but was quite a large change, especially if we only ever need two architectures. Adding a new method to MCInstrAnalysis seems a lot simpler, although I like it slightly less than my way. Any preferences?

301

Ah, that looks like just what I want. Thanks!

313–315

The reason is that on line 326 when I removed the defined register from the target set I needed an iterator not the value, and it seemed more efficient to reuse an iterator than call find. I'd be happy to lose a bit of performance to improve readability if you'd prefer me to use a foreach loop.

319

Good point, I'll move the definition inside the outermost for loop.

The reason I didn't write a testcase for this was that I wasn't sure how to construct one. Are there existing tests that create this pattern, or should I write a unit test, which is presumably simpler to create?

tools/llvm-cfi-verify/lib/FileAnalysis.cpp
157–158

I don't have a preference between the two.

313–315

Ah, I missed that. That's fine then.

319

I think this would make sense for either a unit or a lit test, either way you'll need to either hand-write assembly cases or reuse cases from a CFI-verified binary that this logic fixes.

jgalenson updated this revision to Diff 154459.Jul 6 2018, 2:44 PM
jgalenson marked 5 inline comments as done.

I believe this addresses all of the existing comments except for how to handle detecting traps. I see Vlad just commented that he doesn't have an opinion, so if there are no further opinions I'll just play around and do what I feel is best. The current approach should still work, of course.

However, I was not sure how to create a vtable.cc lit-based regression test. When I compile my simple virtual call test, it creates separate sections for main and each virtual method, each of which starts at 0. This means that FileAnalysis::parseSectionContents will complain because multiple instructions have address 0. This happens even on x86, so I don't think it's AArch64-specific. I think the problem is that llvm-mc isn't producing a fully valid ELF file, but I'm not sure how to do that without a linker. For now I added a number of unit tests to test this case instead.

jgalenson updated this revision to Diff 154618.Jul 9 2018, 8:57 AM
jgalenson edited the summary of this revision. (Show Details)
jgalenson added inline comments.
tools/llvm-cfi-verify/lib/FileAnalysis.cpp
157–158

I ended up going the TableGen route since it seemed more correct to me (all the other MCInstrAnalysis methods default to calling the MCInstrDesc method anyway). I'm not sure how to test it by itself, but I couldn't find tests for another of the flags, so hopefully this is good enough.

vlad.tsyrklevich accepted this revision.Jul 9 2018, 4:36 PM

Changes seem reasonable to me modulo the one nit, though I'd give @pcc a chance to look at it as well if he has any feedback.

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

This is true for AArch64 but not x86.

This revision is now accepted and ready to land.Jul 9 2018, 4:36 PM

Thanks. I'll wait a bit to see if there are any more comments.

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

The way I meant it, this actually included x86 since there the load happens on the indirect branch itself. I can change this to make that more clear, though.

jgalenson updated this revision to Diff 154817.Jul 10 2018, 8:42 AM
jgalenson marked an inline comment as done.
jgalenson edited the summary of this revision. (Show Details)

Updated a comment.

pcc accepted this revision.Jul 12 2018, 11:13 AM

LGTM

tools/llvm-cfi-verify/lib/FileAnalysis.cpp
157–158

Well, if this will only ever work on two architectures, wouldn't it be better to implement it on MCInstrAnalysis rather than in tablegen so that we will assert on the unsupported architectures instead of potentially giving an incorrect result? I don't feel too strongly about that, though.

jgalenson added inline comments.Jul 12 2018, 12:33 PM
tools/llvm-cfi-verify/lib/FileAnalysis.cpp
157–158

That makes sense, but other similar MCID properties seem to work only one some arches (e.g., isAdd only works on ARM and Hexagon). And the current behavior of cfi-verify on unsupported arches is to give an incorrect result, so at least this isn't a regression. :) If we want this, I'd say we could just add an architecture check at the beginning of this tool.

pcc added inline comments.Jul 12 2018, 12:41 PM
tools/llvm-cfi-verify/lib/FileAnalysis.cpp
157–158

I was mostly not thinking about llvm-cfi-verify but other tools or parts of the compiler that might end up trying to use the isTrap bit thinking that it would work on all architectures. But since those users are hypothetical maybe that's not too much of a concern.

Now that you point it out the architecture check might be a nice thing to have anyway.

This revision was automatically updated to reflect the committed changes.