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

Tue Dec 10 04:12:16 PST 2019

dnsampaio marked 7 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:
> 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)`.

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:7728
+      return Error(Operands[4]->getStartLoc(),
+                   "source and destination registers must be sp");
+    break;
----------------
efriedma wrote:
> Is this Error actually reachable?  If it is, please add a testcase.
Indeed it does not make any sense. Getting rid of it.

================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9785
+    Inst.setOpcode(ARM::t2ADDspImm);
+    Inst.addOperand(MCOperand::createReg(0)); // cc_out
+    return true;
----------------
efriedma wrote:
> dnsampaio wrote:
> > efriedma wrote:
> > > Please don't copy-paste code.
> > I'm guessing you are speaking about the ADD and SUB joining in a single case statement, right?
> I was more thinking that the t2ADDri12 and t2ADDspImm12 handling are basically identical.  But I guess add/sub are also almost identical.
Fair enough. Will join all 4 in a single case.

================
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:
> > > 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)
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70680