[PATCH] Patch that adds address-masking instructions before loads and stores, and after the instructions that change stack-pointer.
Mark Seaborn
mseaborn at chromium.org
Fri Mar 7 11:13:12 PST 2014
================
Comment at: test/MC/Mips/nacl-branch-delay.ll:3
@@ +2,3 @@
+; RUN: -O3 < %s | FileCheck %s
+
+; RUN: llc -filetype=asm -mtriple=mipsel-none-nacl -relocation-model=static \
----------------
This file belongs in test/CodeGen -- it's not testing the MC layer.
I didn't notice this when you added test/MC/Mips/nacl-align.ll. Could you do a separate change to move that to test/CodeGen, please?
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:120
@@ +119,3 @@
+ sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, false);
+ else if (isStackChange(Inst))
+ sandboxLoadStoreStackChange(Inst, -1U/*not used*/, STI, false, true);
----------------
Currently this case kicks in for:
sw $sp, 123($sp)
and will convert it to:
sw $sp, 123($sp)
and $sp, $sp, $15
which is a bug. Can you fix this and add a test for it, please?
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:58
@@ +57,3 @@
+
+ bool isStackChange(const MCInst &MI) {
+ return (MI.getNumOperands() > 0 && MI.getOperand(0).isReg()
----------------
This name is a bit misleading because it will return true for store instructions which only read $sp. You're relying on the caller first checking whether the instruction is a store, which fails in one case -- see other comment.
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:114
@@ +113,3 @@
+ else if (isDangerousLoad(Inst, &AddrIdx)) {
+ if (!isStackChange(Inst))
+ sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, false);
----------------
Since only one arg changes below, you could write:
sandboxLoadStoreStackChange(Inst, AddrIdx, STI, true, isStackChange(Inst));
without a conditional. (But see other comments.)
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:32
@@ +31,3 @@
+
+bool isDangerousLoadOpcode(unsigned Opcode, int *AddrIdx) {
+ switch (Opcode) {
----------------
Sasa Stankovic wrote:
> Mark Seaborn wrote:
> > Could this be combined with isDangerousStoreOpcode() as isDangerousMemoryAccess()? There's not a big difference between loads and stores for sandboxing.
> There have to be separate methods for loads and stores because of the case where
> load also changes SP (or they can be combined in one method with one additional
> parameter that tells whether we check loads or stores).
I see what you mean. However, this leads to a bug (see other comment). How about having an isMemoryAccess() which also returns whether the instruction is a load or a store?
http://llvm-reviews.chandlerc.com/D2904
More information about the llvm-commits
mailing list