[PATCH] Implement NaCl sandboxing of function calls

Mark Seaborn mseaborn at chromium.org
Mon Mar 10 14:19:33 PDT 2014



================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:166
@@ +165,3 @@
+      return;
+    } else if (isCall(Inst.getOpcode(), &IsIndirectCall)) {
+      Call = Inst;
----------------
Nit: drop the "else" to be consistent with the rest of the function -- it isn't needed if the "if" block returns.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:45
@@ -43,1 +44,3 @@
 private:
+  MCInst Call;
+  bool SandboxingCall;
----------------
Please comment these three fields.  "Call" and "IsIndirectCall" are only defined if SandboxingCall is true -- so please comment that, and put SandboxingCall first.

================
Comment at: test/MC/Mips/nacl-mask.s:12
@@ -11,2 +11,3 @@
 
+	.align	4
 test1:
----------------
Is the reason for adding this that EmitBundleLock(true) complains if an alignment hasn't been set already?  Or does the test work without adding this .align?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:167
@@ +166,3 @@
+    } else if (isCall(Inst.getOpcode(), &IsIndirectCall)) {
+      Call = Inst;
+      SandboxingCall = true;
----------------
Maybe comment this block: "Save up the call to emit after we see the branch delay instruction that follows"?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsNaClELFStreamer.cpp:46
@@ +45,3 @@
+  MCInst Call;
+  bool SandboxingCall;
+  bool IsIndirectCall;
----------------
Maybe something like "Pending" or "Saved" rather than "Sandboxing" would better suggest what's going on here?


http://llvm-reviews.chandlerc.com/D3032



More information about the llvm-commits mailing list