[PATCH] D70680: [ARM][Thumb2] Fix ADD/SUB invalid writes to SP

Mon Dec 30 03:21:22 PST 2019

dnsampaio marked 6 inline comments as done.
dnsampaio added inline comments.

================
Comment at: llvm/lib/Target/ARM/ARMLoadStoreOptimizer.cpp:711
+                                                        : ARM::t2SUBri)
+                         : (isThumb1 && Offset < 8 && Base != ARM::SP)
+                               ? ARM::tSUBi3
----------------
efriedma wrote:
> dnsampaio wrote:
> > efriedma wrote:
> > > Not your patch, but the way Thumb1 is handling SP here doesn't make any sense.  Maybe add a FIXME?
> > What do you mean with the SP handling of thumb1? There is a section just below handling `if (BaseOpc == ARM::tADDrSPi)`.
> I mean that we try to use tSUBi8 on SP.
> 
> But I guess that can't actually happen. If "Offset < 0" is true, we can't be in Thumb1 mode because there aren't any load/store instructions with negative offsets.  So the "Base != ARM::SP" check here is unnecessary.
Got it. Adding a FIXME here for it.

================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:6630
+static DecodeStatus DecodeT2AddSubSPImm(MCInst &Inst, unsigned Insn,
+                                        uint64_t Address, const void *Decoder) {
+  const unsigned Rd = fieldFromInstruction(Insn, 8, 4);
----------------
efriedma wrote:
> dnsampaio wrote:
> > efriedma wrote:
> > > dnsampaio wrote:
> > > > efriedma wrote:
> > > > > Please don't copy-paste code.
> > > > Again, not quite sure, but guessing I can reduce the if/else common parts.
> > > Nevermind; I assumed you copy-pasted this without really checking.
> > > 
> > > Why do we need a C++ DecoderMethod for t2ADDspImm, when we don't need one for t2ADDri?
> > If I don't use a custom decoder, the disassembly of the instruction `0x0d 0xf1 0x00 0x0d` (should disassemble as `add.w sp, sp, #0`) is matched as a `ADR, t2ADR` in the generated `build/lib/Target/ARM/ARMGenAsmWriter.inc`. I'm not fully aware of why yet, probably the same reason why `ADR` was being decoded as `SUB`?
> > The `ADR` instruction seems to have less operands and the disassembler dies with the below error when printing the `cc_out` operand,:
> > ```/work/bf/LLVM/build/bin/llvm-mc -triple=thumbv7-apple-darwin -mcpu=cortex-a8 -disassemble < /tmp/a
> >         .section        __TEXT,__text,regular,pure_instructions
> >         adds.w  r1, r2, #496
> >         addllvm-mc: /work/bf/LLVM/src/llvm/include/llvm/ADT/SmallVector.h:153: const T& llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::operator[](llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::size_type) const [with T = llvm::MCOperand; <template-parameter-1-2> = void; llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::const_reference = const llvm::MCOperand&; llvm::SmallVectorTemplateCommon<T, <template-parameter-1-2> >::size_type = long unsigned int]: Assertion `idx < size()' failed.
> > Stack dump:
> > 0.      Program arguments: /work/bf/LLVM/build/bin/llvm-mc -triple=thumbv7-apple-darwin -mcpu=cortex-a8 -disassemble 
> >  #0 0x00007f342f10611d llvm::sys::PrintStackTrace(llvm::raw_ostream&) /work/bf/LLVM/src/llvm/lib/Support/Unix/Signals.inc:548:22
> >  #1 0x00007f342f1061b0 PrintStackTraceSignalHandler(void*) /work/bf/LLVM/src/llvm/lib/Support/Unix/Signals.inc:609:1
> >  #2 0x00007f342f103fe0 llvm::sys::RunSignalHandlers() /work/bf/LLVM/src/llvm/lib/Support/Signals.cpp:68:20
> >  #3 0x00007f342f105a9c SignalHandler(int) /work/bf/LLVM/src/llvm/lib/Support/Unix/Signals.inc:390:1
> >  #4 0x00007f342e5b14b0 (/lib/x86_64-linux-gnu/libc.so.6+0x354b0)
> >  #5 0x00007f342e5b1428 raise /build/glibc-LK5gWL/glibc-2.23/signal/../sysdeps/unix/sysv/linux/raise.c:54:0
> >  #6 0x00007f342e5b302a abort /build/glibc-LK5gWL/glibc-2.23/stdlib/abort.c:91:0
> >  #7 0x00007f342e5a9bd7 __assert_fail_base /build/glibc-LK5gWL/glibc-2.23/assert/assert.c:92:0
> >  #8 0x00007f342e5a9c82 (/lib/x86_64-linux-gnu/libc.so.6+0x2dc82)
> >  #9 0x00007f343641da0f llvm::SmallVectorTemplateCommon<llvm::MCOperand, void>::operator[](unsigned long) const /work/bf/LLVM/src/llvm/include/llvm/ADT/SmallVector.h:154:19
> > #10 0x00007f343641d851 llvm::MCInst::getOperand(unsigned int) const /work/bf/LLVM/src/llvm/include/llvm/MC/MCInst.h:180:71
> > #11 0x00007f34364199e0 llvm::ARMInstPrinter::printSBitModifierOperand(llvm::MCInst const*, unsigned int, llvm::MCSubtargetInfo const&, llvm::raw_ostream&) /work/bf/LLVM/src/llvm/lib/Target/ARM/MCTargetDesc/ARMInstPrinter.cpp:997:35
> > #12 0x00007f3436408771 llvm::ARMInstPrinter::printInstruction(llvm::MCInst const*, llvm::MCSubtargetInfo const&, llvm::raw_ostream&) /work/bf/LLVM/build/lib/Target/ARM/ARMGenAsmWriter.inc:9164:26
> > #13 0x00007f34364164ba llvm::ARMInstPrinter::printInst(llvm::MCInst const*, llvm::raw_ostream&, llvm::StringRef, llvm::MCSubtargetInfo const&) /work/bf/LLVM/src/llvm/lib/Target/ARM/MCTargetDesc/ARMInstPrinter.cpp:307:18
> > #14 0x00007f342f913b4c llvm::MCTargetStreamer::prettyPrintAsm(llvm::MCInstPrinter&, llvm::raw_ostream&, llvm::MCInst const&, llvm::MCSubtargetInfo const&) /work/bf/LLVM/src/llvm/lib/MC/MCStreamer.cpp:983:1
> > #15 0x00007f342f8933a9 (anonymous namespace)::MCAsmStreamer::EmitInstruction(llvm::MCInst const&, llvm::MCSubtargetInfo const&) /work/bf/LLVM/src/llvm/lib/MC/MCAsmStreamer.cpp:1947:40
> > #16 0x0000000000431159 PrintInsts(llvm::MCDisassembler const&, std::pair<std::vector<unsigned char, std::allocator<unsigned char> >, std::vector<char const*, std::allocator<char const*> > > const&, llvm::SourceMgr&, llvm::raw_ostream&, llvm::MCStreamer&, bool, llvm::MCSubtargetInfo const&) /work/bf/LLVM/src/llvm/tools/llvm-mc/Disassembler.cpp:73:7
> > #17 0x0000000000431a62 llvm::Disassembler::disassemble(llvm::Target const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, llvm::MCSubtargetInfo&, llvm::MCStreamer&, llvm::MemoryBuffer&, llvm::SourceMgr&, llvm::MCContext&, llvm::raw_ostream&, llvm::MCTargetOptions const&) /work/bf/LLVM/src/llvm/tools/llvm-mc/Disassembler.cpp:197:34
> > #18 0x00000000004190fd main /work/bf/LLVM/src/llvm/tools/llvm-mc/llvm-mc.cpp:521:36
> > #19 0x00007f342e59c830 __libc_start_main /build/glibc-LK5gWL/glibc-2.23/csu/../csu/libc-start.c:325:0
> > #20 0x0000000000416f29 _start (/work/bf/LLVM/build/bin/llvm-mc+0x416f29)
> > Aborted (core dumped)
> > ```
> I don't follow why it's crashing; if it matched adr, it would be trying to print an adr, not an add.  The immediate cause of the crash is that the printer is expecting an operand to represent the "s" bit, and isn't finding it.  But ARM::t2ADDspImm should have an operand to represent the "s" bit.
> 
> I guess there's a sort of weird overlap here; DecoderGPRRegisterClass returns SoftFail where it should actually be hard-failing.  So the ri/ri12 variants actually match in cases where we don't want them to.  I would have expected that to mean you need a decoder for the ri/ri12 variants, not the sp variants, though, and I don't think it would cause a crash.
Changing DecoderGPRRegisterClass to return a `MCDisassembler::Fail` breaks some tests. Would it be ok if I keep the custom decoder for now, and add a FIXME to the DecoderGPRRegisterClass?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70680/new/

https://reviews.llvm.org/D70680