[PATCH] D48836: [cfi-verify] Support AArch64.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:157
 bool FileAnalysis::isCFITrap(const Instr &InstrMeta) const {
-  return MII->getName(InstrMeta.Instruction.getOpcode()) == "TRAP";
+  return MII->getName(InstrMeta.Instruction.getOpcode()) == "TRAP" ||
+         MII->getName(InstrMeta.Instruction.getOpcode()) == "BRK";
----------------
jgalenson wrote:
> pcc wrote:
> > vlad.tsyrklevich wrote:
> > > 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.)
> > 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.
> 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?
I don't have a preference between the two.


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:319
 
-      for (unsigned RegNum : RegisterNumbers) {
+      for (auto RI = CurRegisterNumbers.begin(), RE = CurRegisterNumbers.end();
+           RI != RE; ++RI) {
----------------
jgalenson wrote:
> vlad.tsyrklevich wrote:
> > Any reason to not use a range-for here?
> 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.
Ah, I missed that. That's fine then.


================
Comment at: tools/llvm-cfi-verify/lib/FileAnalysis.cpp:325
+          if (canLoad && InstrDesc.mayLoad()) {
+            canLoad = false;
+            CurRegisterNumbers.erase(RI);
----------------
jgalenson wrote:
> vlad.tsyrklevich wrote:
> > 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.
> 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?
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D48836