[PATCH] Patch that aligns on bundle size the targets of indirect jumps, and that adds masks before indirect jumps and returns
Mark Seaborn
mseaborn at chromium.org
Thu Feb 20 09:04:01 PST 2014
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCNaCl.h:15
@@ +14,3 @@
+
+#define MIPS_NACL_BUNDLE_ALIGN 4u
+
----------------
In LLVM, a "static const int" would be preferred over a #define
Can you comment that this is the log2 of the bundle size?
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCNaCl.h:20
@@ +19,3 @@
+// This function creates ELF Streamer for Mips NaCl.
+MCELFStreamer* createMipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB,
+ raw_ostream &OS,
----------------
LLVM spacing style is " *"
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCNaCl.h:26
@@ +25,2 @@
+
+#endif // MIPSMCNACL_H
----------------
Nit: Most LLVM headers don't comment the #include guard's #endif
================
Comment at: test/MC/Mips/nacl-align.ll:6
@@ +5,3 @@
+; RUN: -O3 < %s \
+; RUN: | llvm-objdump -triple mipsel -disassemble -no-show-raw-insn - \
+; RUN: | FileCheck %s -check-prefix=CHECK-NACL
----------------
Can you add a test for the MC-layer changes that tests that address-masking sandboxing is added when given assembly input, please?
For testing that indirect jump targets are properly aligned, it seems like the best way to do that would be to check that llc's assembly output contains appropriate ".align" or ".p2align" directives (rather than checking the object file output).
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp:18
@@ -17,2 +17,3 @@
#include "MipsTargetStreamer.h"
+#include "InstPrinter/MipsInstPrinter.h"
#include "llvm/ADT/Triple.h"
----------------
Nit: sort #includes?
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:22
@@ +21,3 @@
+
+static unsigned IndirectBranchMaskReg = Mips::T6;
+
----------------
Add "const"
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:11
@@ +10,3 @@
+// This file implements ELF Streamer for Mips NaCl. It emits .o object files by
+// masking dangerous instructions.
+//
----------------
You could provide more explanatory context here, e.g. "It inserts address-masking instructions as required by NaCl's SFI sandbox"
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:38
@@ +37,3 @@
+private:
+ bool IsReturn(const MCInst &MI) {
+ return MI.getOpcode() == Mips::RET;
----------------
Please use the LLVM naming style: "isReturn", with a lower case initial letter
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:42
@@ +41,3 @@
+
+ bool IsIndirectJump(const MCInst &MI) {
+ return MI.getOpcode() == Mips::JR;
----------------
Ditto: "isIndirectJump" etc.
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:46
@@ +45,3 @@
+
+ void BaseEmitInstruction(const MCInst& Inst, const MCSubtargetInfo& STI) {
+ MCELFStreamer::EmitInstruction(Inst, STI);
----------------
It seems like you could just inline this rather than defining a named function for it
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:51
@@ +50,3 @@
+ void EmitMask(unsigned AddrReg, unsigned MaskReg,
+ const MCSubtargetInfo& STI) {
+ MCInst MaskInst;
----------------
Use " &" spacing style
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:65
@@ +64,3 @@
+
+ // We cannot use this->EmitBundleLock() because EmitBundleLock, although
+ // public in MCStreamer, is declared private in MCELFStreamer.
----------------
Well, you shouldn't be working around that with a cast. Why not change MCELFStreamer.h to make EmitBundleLock() public?
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:79
@@ +78,3 @@
+ virtual void EmitInstruction(const MCInst& Inst, const MCSubtargetInfo& STI) {
+
+ if (IsIndirectJump(Inst) || IsReturn(Inst))
----------------
Nit: don't start a function with an empty line
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:43
@@ +42,3 @@
+ bool IsIndirectJump(const MCInst &MI) {
+ return MI.getOpcode() == Mips::JR;
+ }
----------------
Since you don't distinguish isReturn() and isIndirectJump(), how about folding isReturn() into isIndirectJump(), i.e.
return MI.getOpcode() == Mips::JR || MI.getOpcode() == Mips::RET;
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:74
@@ +73,3 @@
+
+public:
+
----------------
Nit: remove empty line after this
================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:281
@@ +280,3 @@
+
+ // Make sure the function entry is bundle-aligned if target is NaCl.
+ if (Subtarget->isTargetNaCl())
----------------
The comment is just duplicating the code. It would be better to say why, e.g. "NaCl sandboxing requires function entry points to be bundle-aligned"
================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:924
@@ +923,3 @@
+void MipsAsmPrinter::NaClAlignAllJumpTargets(MachineFunction &MF) {
+ assert(Subtarget->isTargetNaCl()
+ && "NaCl-related function called for non-NaCl target!");
----------------
This check seems unnecessary, because this function won't break if called unnecessarily, and it's only called in one place.
================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:928
@@ +927,3 @@
+ // Align all blocks that are jumped to through jump table.
+ MachineJumpTableInfo *jt_info = MF.getJumpTableInfo();
+ if (jt_info) {
----------------
Please use LLVM naming style for this variable
================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:929
@@ +928,3 @@
+ MachineJumpTableInfo *jt_info = MF.getJumpTableInfo();
+ if (jt_info) {
+ const std::vector<MachineJumpTableEntry> &JT = jt_info->getJumpTables();
----------------
You can shorten to "if (MachineJumpTableInfo *JtInfo = ...)"
================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:931
@@ +930,3 @@
+ const std::vector<MachineJumpTableEntry> &JT = jt_info->getJumpTables();
+ for (unsigned i = 0; i < JT.size(); ++i) {
+ std::vector<MachineBasicBlock*> MBBs = JT[i].MBBs;
----------------
"i" -> "I" to follow LLVM style
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCNaCl.h:24
@@ +23,3 @@
+ bool RelaxAll, bool NoExecStack);
+}
+
----------------
If the namespace starts with an empty line, there should be an empty line before the "}" too for consistency. Same for other cases below...
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp:113
@@ +112,3 @@
+ // Set bundle-alignment as required by the NaCl ABI for the target.
+ S->EmitBundleAlignMode(MIPS_NACL_BUNDLE_ALIGN);
+ }
----------------
How about doing this in createMipsNaClELFStreamer(), to keep the NaCl-related stuff in NaCl-related files as much as possible?
================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:31
@@ +30,3 @@
+public:
+ MipsNaClELFStreamer(MCContext &Context, MCAsmBackend &TAB, raw_ostream &OS,
+ MCCodeEmitter *Emitter)
----------------
Can this be dropped, since it doesn't do anything in addition to MCELFStreamer's constructor?
================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:78
@@ +77,3 @@
+
+ // In NaCl, all jump targets must be aligned on bundle size.
+ if (Subtarget->isTargetNaCl())
----------------
This should be "all indirect jump targets". Direct jump targets don't need to be aligned.
================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:80
@@ +79,3 @@
+ if (Subtarget->isTargetNaCl())
+ NaClAlignAllJumpTargets(MF);
+
----------------
So "AlignIndirectJumpTargets"?
http://llvm-reviews.chandlerc.com/D2847
More information about the llvm-commits
mailing list