Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1185703004:
Add constant blinding/pooling option for X8632 code translation (Closed)
|
Created:
5 years, 6 months ago by qining Modified:
5 years, 6 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd constant blinding/pooling option for X8632 code translation.
GOAL:
The goal is to remove the ability of an attacker to control immediates emitted into the text section.
OPTION:
The option -randomize-pool-immediates is set to none by default (-randomize-pool-immediates=none). To turn on constant blinding, set -randomize-pool-immediates=randomize; to turn on constant pooling, use -randomize-pool-immediates=pool.
Not all constant integers in the input pexe file will be randomized or pooled. The signed representation of a candidate constant integer must be between -randomizeOrPoolImmediatesThreshold/2 and +randomizeOrPoolImmediatesThreshold/2. This threshold value can be set with command line option: "-randomize-pool-threshold". By default this threshold is set to 0xffff.
The constants introduced by instruction lowering (e.g. constants in shifting, masking) and argument lowering are not blinded in this way. The mask used for sandboxing is not affected either.
APPROACH:
We use GAS syntax in these examples.
Constant blinding for immediates:
Original:
add 0x1234, eax
After:
mov 0x1234+cookie, temp_reg
lea -cookie[temp_reg], temp_reg
add temp_reg, eax
Constant blinding for memory addressing offsets:
Original:
mov 0x1234(eax, esi, 1), ebx
After:
lea 0x1234+cookie(eax), temp_reg
mov -cookie(temp_reg, esi, 1), ebx
We use "lea" here because it won't affect flag register, so it is safer to transform immediate-involved instructions.
Constant pooling for immediates:
Original:
add 0x1234, eax
After:
mov [memory label of 0x1234], temp_reg
add temp_reg, eax
Constant pooling for addressing offsets:
Original:
mov 0x1234, eax
After:
mov [memory label of 0x1234], temp_reg
mov temp_reg, eax
Note in both cases, temp_reg may be assigned with "eax" here, depends on the
liveness analysis. So this approach may not require extra register.
IMPLEMENTATION:
Processing:
TargetX8632::randomizeOrPoolImmediate(Constant *Immediate, int32_t RegNum);
TargetX8632::randomizeOrPoolImmediate(OperandX8632Mem *Memoperand, int32_t RegNum);
Checking eligibility:
ConstantInteger32::shouldBeRandomizedOrPooled(const GlobalContext *Ctx);
ISSUES:
1. bool Ice::TargetX8632::RandomizationPoolingPaused is used to guard some translation phases to disable constant blinding/pooling temporally. Helper class BoolFlagSaver is added to latch the value of RandomizationPoolingPaused.
Known phases that need to be guarded are: doLoadOpt() and advancedPhiLowering(). However, during advancedPhiLowering(), if the destination variable has a physical register allocated, constant blinding and pooling are allowed. Stopping blinding/pooling for doLoadOpt() won't hurt our randomization or pooling as the optimized addressing operands will be processed again in genCode() phase.
2. i8 and i16 constants are collected with different constant pools now, instead of sharing a same constant pool with i32 constants. This requires emitting two more pools during constants lowering, hence create two more read-only data sections in the resulting ELF and ASM. No runtime issues have been observed so far.
BUG=
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=253dc8a870cbd144ba217a44efbd07e6bcd71e97
Patch Set 1 #Patch Set 2 : reformat #
Total comments: 87
Patch Set 3 : remove parameter-unused warning in shouldBeRandomizedOrPooled() #Patch Set 4 : Fix the issues in the last commit #Patch Set 5 : reformat #
Total comments: 20
Patch Set 6 : remove the MemOperand->getBase()->setWeightInfinite() in randomizeOrPoolImmediate() #Patch Set 7 : Avoid calling randomizationOrPoolImmediate() multiple times on a same operand" #
Total comments: 39
Patch Set 8 : Add bool flag: Randomized to OperandX8632Mem class, fix comments, rebased to master:b0a8c24ecd98f4b… #
Total comments: 9
Patch Set 9 : #Patch Set 10 : add command line option for immediates randomization/pooling threshold #Patch Set 11 : minor #Patch Set 12 : Fix the default randomization/pooling threshold to be 0xffff #
Total comments: 15
Patch Set 13 : fix the lit tests and some issues #
Total comments: 6
Patch Set 14 : change the default sz-seed back to 1. #
Messages
Total messages: 26 (2 generated)
qining@google.com changed reviewers: + jpp@chromium.org, jvoung@chromium.org, stichnot@chromium.org
Nice start! This touches and messes with some very fundamental assumptions and machinery in Subzero, so it's great to have something that passes the tests and runs Spec. FYI, I'll also want to see cleaner comments (clarity, grammar, formatting), plus I'll probably have more comments on the latter part of the lowering code before this is done. https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.cpp#newc... src/IceClFlags.cpp:261: clEnumValN(Ice::RandomizeImmediates, "constant-blinding", I suggest "blind" instead of "constant-blinding" and "pool" instead of "constant-pooling". https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.h File src/IceClFlags.h (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.h#newcod... src/IceClFlags.h:181: Ice::RandomizeAndPoolImmediatesEnum option) { capitalize Option per LLVM coding conventions https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.h#newcod... src/IceClFlags.h:184: bool shouldRandomizeImmediates() const { I think it would be better to have a single getRandomizeImmediates() method instead of a bunch of bool getters. There are other getters here that return an enum type, e.g. getOptLevel(), getOutFileType(), getTargetArch(), getVerbose(), etc. https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.h#newcod... src/IceClFlags.h:192: Ice::NoneImmediatesRandomizationPooling; I don't think you need "Ice::" here and above since you're already in the Ice namespace. https://codereview.chromium.org/1185703004/diff/20001/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceDefs.h#newcode227 src/IceDefs.h:227: // Immeidates Pooling and Randomization options "Immediates" misspelled. I would prefer not so much capitalization, such as "Options for pooling and randomization of immediates". https://codereview.chromium.org/1185703004/diff/20001/src/IceDefs.h#newcode229 src/IceDefs.h:229: NoneImmediatesRandomizationPooling, For non-scoped enum values in the top-level (Ice) namespace, let's use a common prefix on the values. E.g., RPI_None, RPI_Randomize, RPI_Pool https://codereview.chromium.org/1185703004/diff/20001/src/IceELFObjectWriter.cpp File src/IceELFObjectWriter.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceELFObjectWriter.... src/IceELFObjectWriter.cpp:508: // When constant pooling turned on, only emit labels for eligible constants end sentence with period https://codereview.chromium.org/1185703004/diff/20001/src/IceELFObjectWriter.... src/IceELFObjectWriter.cpp:510: if (llvm::isa<ConstantInteger32>(C) && Normally this kind of type checking would be done like this: if (auto *CI32 = llvm::dyn_cast<ConstantInteger32>(C)) { if (!CI32->shouldBeRandomizedOrPooled(&Ctx)) continue; } Note the use of dyn_cast<> instead of dyn_cast_or_null<> since we know C will not be null. Also note that the dyn_cast<> pattern handles both the isa<> check and the cast. However, can't you avoid all the type checking and just do this? if (!Const->shouldBeRandomizedOrPooled(&Ctx)) continue; https://codereview.chromium.org/1185703004/diff/20001/src/IceELFObjectWriter.... src/IceELFObjectWriter.cpp:512: ->shouldBeRandomizedOrPooled(&Ctx) == false) use "!x" instead of "x==false" https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.cpp#newc... src/IceOperand.cpp:494: // =========== Immediate Radomization and Pooling routines ============== Randomization https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.cpp#newc... src/IceOperand.cpp:504: bool typeCorrect = getType() == IceType_i32; Can you just do this? if (getType() != IceType_i32) return false; https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.cpp#newc... src/IceOperand.cpp:506: X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD / 2 + Value >= Could you add a comment explaining that this checks whether the signed representation of Value is between -threshold/2 and +threshold/2? (I had to dig into the arcane rules of C++ automatic signed/unsigned conversion to convince myself of this.) https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:163: // Judge if this given immediate should be randomized or pooled End this and the next sentence with a period. https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:165: // truely go through this method truly https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:192: bool ConstantInteger32::shouldBeRandomizedOrPooled(const GlobalContext *Ctx); I'm not sure it's sufficient to limit blinding to 32-bit integer constants. Might we want to apply it to ConstantInteger16 as well? On the other hand, if the blinding threshold for i32 is 0xffff, then an attacker already has 16 bits to control and in theory it might not matter to also let them control all 16 bits of an i16 constant. Maybe the best way is to implement blinding for i32, i16, and i8, but apply the same threshold to all types. For example, if the threshold remains 0xffff, then we would end up never blinding any i16 or i8 values. But if we changed the threshold to 0xff, then we might blind some i16 constants, but never any i8 constants. And so on. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:442: // qining: initialize the assistant pause flag "qining:"? (here and below) What does "assistant" mean here? https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:447: // Note, when a physical regs are available for the Dest I'm not 100% sure what this part of the comment means, but I suspect you want to leave a TODO here for when some other part of phi lowering gets fixed. Something like: TODO(qining,stichnot): Recombobulate the frobnicator after phi lowering is fixed. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:450: RandomizationPoolingPaused = false; Do this simple initialization in the initializer list of the ctor. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:496: bool RPLatchForDoLoadOpt = RandomizationPoolingPaused; You are using this pattern enough times (5?) that it might be cleaner to manage the push/pop through a helper class defined in an anonymous namespace. E.g.: class BoolFlagSaver { // Disable default no-arg and copy ctors, and default assignment operator public: BoolFlagSaver(bool &F, bool NewValue) : Flag(F) { OldValue = F; F = NewValue; } ~BoolFlagSaver() { Flag = OldValue; } private: bool &Flag; bool OldValue; }; Then use it like this: { BoolFlagSaver B(RandomizationPoolingPaused, true); doLoadOpt(); } This helper would go away once all the workarounds are fixed. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:527: // : In general we need to pause constant blinding or pooling ":" ? https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:528: // advanced phi lowering, unless the lowering assignment has a during advanced phi lowering https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1195: ConstantInteger32 *ConstInt = llvm::dyn_cast<ConstantInteger32>( Use cast<> instead of dyn_cast<>. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1196: Ctx->getConstantInt32(static_cast<uint32_t>(Const->getValue()))); I think you should use int32_t instead of uint32_t. (Realizing that the code originally used uint32_t...) https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1223: ConstantInteger32 *ConstInt = llvm::dyn_cast<ConstantInteger32>( cast<> https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1224: Ctx->getConstantInt32(static_cast<uint32_t>(Const->getValue() >> 32))); int32_t https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:4680: From = randomizeOrPoolImmediate(llvm::dyn_cast<OperandX8632Mem>(From)); You've already done a dyn_cast<>(From) in the enclosing "if" statement, so I think you could just use Mem here. (But you would also have to change the "From = ..." 6 lines up to be "Mem = ...".) https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:4709: if (From && llvm::isa<ConstantInteger32>(From)) { "From" must be non-null, so no need to test that. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:4711: randomizeOrPoolImmediate(llvm::cast<Constant>(From), RegNum); You can avoid the cast<> and the earlier isa<> check by using dyn_cast<>. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:4779: OperandX8632Mem *TargetX8632::formMemoryOperand(Operand *operand, Type Ty, Variable names should be capitalized, so maybe Opnd instead? https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5083: llvm::dyn_cast_or_null<ConstantInteger32>(C) Same comments here as in IceELFObjectWriter.cpp https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5137: static uint64_t cookie = 0; Don't use static non-const variables, as they are not thread-safe. Maybe store the cookie in GlobalContext instead? https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5149: Operand *TargetX8632::randomizeOrPoolImmediate(Constant *immediate, Capitalize "immediate" arg name. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5160: ->shouldBeRandomizedOrPooled(Ctx)) { Call immediate->shouldBeRandomizedOrPooled(Ctx) directly without type checking or casting? https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5171: // andvancedPhiLowering()=>lowerAssign(). In this case we should reuse advancedPhiLowering https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5175: ConstantInteger32 *integer = llvm::cast<ConstantInteger32>(immediate); Capitalize variable names https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5177: uint64_t cookie = getRandomizationCookie(); uint32_t https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5187: } else { Don't use this pattern: if (abc) { def; return; } else { ghi; } Instead, use: if (abc) { def; return; } ghi; https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5196: // andvancedPhiLowering()=>lowerAssign(). In this case we should reuse advancedPhiLowering https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5203: Constant *symbol = Ctx->getConstantSym(0, label_stream.str(), true); Try to avoid using non-obvious constants as arguments to function calls. Instead, something like this: const RelocOffsetT Offset = 0; const bool SuppressMangling = true; ... getConstantSym(Offset, label_stream.str(), SuppressMangling); For better or worse, the ::create() methods seem to be exceptions to this rule... https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5218: assert(llvm::isa<OperandX8632Mem>(MemOperand)); This assert isn't very useful and should always succeed (unless MemOperand==nullptr in which case isa<> will crash). https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5225: if (MemOperand->getOffset() && Something like this might be more compact: if (auto *C = llvm::dyn_cast_or_null<Constant>(MemOperand->getOffset()) { if (C->shouldBeRandomizedOrPooled(Ctx)) { ... } } https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5246: if (MemOperand->getBase() != NULL) nullptr, or just leave out the != part https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5247: MemOperand->getBase()->setWeightInfinite(); This will get you into trouble. The base variable could be a program variable that is live throughout the function. There could be many memory operands, each with a different base variable that is live throughout the function. The register allocator won't have enough registers to satisfy all the infinite-weight directives. Because of this, we need to set infinite weight or a fixed register assignment only to temporaries we create with a short lifetime. The way to deal with this is to _mov the variable into a temporary variable with infinite weight, and reconstruct the mem operand using that temporary for the base. However, I'm a bit puzzled that this is even necessary. Normally legalize() will rewrite any memory operand into one that guarantees all of its variables have infinite weight, and all operands are supposed to be legalized. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.h (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.h:574: uint32_t getRandomizationCookie(); I think this method should be const. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.h:575: Operand *randomizeOrPoolImmediate(Constant *immediate, capitalize "immediate"
Reply test https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:450: RandomizationPoolingPaused = false; On 2015/06/12 23:48:17, stichnot wrote: > Do this simple initialization in the initializer list of the ctor. Agree, but should we still leave the comments (may be a TODO) here?
Reply test
https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.cpp#newc... src/IceClFlags.cpp:261: clEnumValN(Ice::RandomizeImmediates, "constant-blinding", On 2015/06/12 23:48:16, stichnot wrote: > I suggest "blind" instead of "constant-blinding" and "pool" instead of > "constant-pooling". Agree, but I think we should use 'randomize' instead of 'blind', because we call this option: 'randomize-pool-immediates'. https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.h File src/IceClFlags.h (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.h#newcod... src/IceClFlags.h:181: Ice::RandomizeAndPoolImmediatesEnum option) { On 2015/06/12 23:48:17, stichnot wrote: > capitalize Option per LLVM coding conventions Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.h#newcod... src/IceClFlags.h:184: bool shouldRandomizeImmediates() const { On 2015/06/12 23:48:16, stichnot wrote: > I think it would be better to have a single getRandomizeImmediates() method > instead of a bunch of bool getters. There are other getters here that return an > enum type, e.g. getOptLevel(), getOutFileType(), getTargetArch(), getVerbose(), > etc. Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceClFlags.h#newcod... src/IceClFlags.h:192: Ice::NoneImmediatesRandomizationPooling; On 2015/06/12 23:48:17, stichnot wrote: > I don't think you need "Ice::" here and above since you're already in the Ice > namespace. Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceDefs.h File src/IceDefs.h (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceDefs.h#newcode227 src/IceDefs.h:227: // Immeidates Pooling and Randomization options On 2015/06/12 23:48:17, stichnot wrote: > "Immediates" misspelled. > > I would prefer not so much capitalization, such as "Options for pooling and > randomization of immediates". Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceDefs.h#newcode229 src/IceDefs.h:229: NoneImmediatesRandomizationPooling, On 2015/06/12 23:48:17, stichnot wrote: > For non-scoped enum values in the top-level (Ice) namespace, let's use a common > prefix on the values. E.g., > RPI_None, > RPI_Randomize, > RPI_Pool Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceELFObjectWriter.cpp File src/IceELFObjectWriter.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceELFObjectWriter.... src/IceELFObjectWriter.cpp:508: // When constant pooling turned on, only emit labels for eligible constants On 2015/06/12 23:48:17, stichnot wrote: > end sentence with period Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceELFObjectWriter.... src/IceELFObjectWriter.cpp:510: if (llvm::isa<ConstantInteger32>(C) && On 2015/06/12 23:48:17, stichnot wrote: > Normally this kind of type checking would be done like this: > > if (auto *CI32 = llvm::dyn_cast<ConstantInteger32>(C)) { > if (!CI32->shouldBeRandomizedOrPooled(&Ctx)) > continue; > } > > Note the use of dyn_cast<> instead of dyn_cast_or_null<> since we know C will > not be null. Also note that the dyn_cast<> pattern handles both the isa<> check > and the cast. > > However, can't you avoid all the type checking and just do this? > > if (!Const->shouldBeRandomizedOrPooled(&Ctx)) > continue; I have moved shouldBeRandomizedOrPooled() to class Constant or even Operand. shouldBePooled flag is also added to the Constant class. https://codereview.chromium.org/1185703004/diff/20001/src/IceELFObjectWriter.... src/IceELFObjectWriter.cpp:512: ->shouldBeRandomizedOrPooled(&Ctx) == false) On 2015/06/12 23:48:17, stichnot wrote: > use "!x" instead of "x==false" Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.cpp#newc... src/IceOperand.cpp:494: // =========== Immediate Radomization and Pooling routines ============== On 2015/06/12 23:48:17, stichnot wrote: > Randomization Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.cpp#newc... src/IceOperand.cpp:504: bool typeCorrect = getType() == IceType_i32; On 2015/06/12 23:48:17, stichnot wrote: > Can you just do this? > if (getType() != IceType_i32) > return false; Done. I've also add IceType_i8, IceType_i16 as valid type for randomization and pooling. https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.cpp#newc... src/IceOperand.cpp:506: X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD / 2 + Value >= On 2015/06/12 23:48:17, stichnot wrote: > Could you add a comment explaining that this checks whether the signed > representation of Value is between -threshold/2 and +threshold/2? > > (I had to dig into the arcane rules of C++ automatic signed/unsigned conversion > to convince myself of this.) Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:163: // Judge if this given immediate should be randomized or pooled On 2015/06/12 23:48:17, stichnot wrote: > End this and the next sentence with a period. Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:165: // truely go through this method On 2015/06/12 23:48:17, stichnot wrote: > truly Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceOperand.h#newcod... src/IceOperand.h:192: bool ConstantInteger32::shouldBeRandomizedOrPooled(const GlobalContext *Ctx); On 2015/06/12 23:48:17, stichnot wrote: > I'm not sure it's sufficient to limit blinding to 32-bit integer constants. > Might we want to apply it to ConstantInteger16 as well? > > On the other hand, if the blinding threshold for i32 is 0xffff, then an attacker > already has 16 bits to control and in theory it might not matter to also let > them control all 16 bits of an i16 constant. > > Maybe the best way is to implement blinding for i32, i16, and i8, but apply the > same threshold to all types. For example, if the threshold remains 0xffff, then > we would end up never blinding any i16 or i8 values. But if we changed the > threshold to 0xff, then we might blind some i16 constants, but never any i8 > constants. And so on. Done. We now support randomizing and pooling of i8 and i16 constant integers. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:442: // qining: initialize the assistant pause flag On 2015/06/12 23:48:18, stichnot wrote: > "qining:"? (here and below) > > What does "assistant" mean here? Done. Removed https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:450: RandomizationPoolingPaused = false; On 2015/06/12 23:48:17, stichnot wrote: > Do this simple initialization in the initializer list of the ctor. Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:496: bool RPLatchForDoLoadOpt = RandomizationPoolingPaused; On 2015/06/12 23:48:17, stichnot wrote: > You are using this pattern enough times (5?) that it might be cleaner to manage > the push/pop through a helper class defined in an anonymous namespace. E.g.: > > class BoolFlagSaver { > // Disable default no-arg and copy ctors, and default assignment operator > public: > BoolFlagSaver(bool &F, bool NewValue) : Flag(F) { > OldValue = F; > F = NewValue; > } > ~BoolFlagSaver() { Flag = OldValue; } > private: > bool &Flag; > bool OldValue; > }; > > Then use it like this: > > { > BoolFlagSaver B(RandomizationPoolingPaused, true); > doLoadOpt(); > } > > This helper would go away once all the workarounds are fixed. Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:527: // : In general we need to pause constant blinding or pooling On 2015/06/12 23:48:18, stichnot wrote: > ":" ? Done. Should I remove this comment? https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:528: // advanced phi lowering, unless the lowering assignment has a On 2015/06/12 23:48:17, stichnot wrote: > during advanced phi lowering Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1195: ConstantInteger32 *ConstInt = llvm::dyn_cast<ConstantInteger32>( On 2015/06/12 23:48:18, stichnot wrote: > Use cast<> instead of dyn_cast<>. Done https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1196: Ctx->getConstantInt32(static_cast<uint32_t>(Const->getValue()))); On 2015/06/12 23:48:18, stichnot wrote: > I think you should use int32_t instead of uint32_t. (Realizing that the code > originally used uint32_t...) Done https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1223: ConstantInteger32 *ConstInt = llvm::dyn_cast<ConstantInteger32>( On 2015/06/12 23:48:18, stichnot wrote: > cast<> Done https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1224: Ctx->getConstantInt32(static_cast<uint32_t>(Const->getValue() >> 32))); On 2015/06/12 23:48:17, stichnot wrote: > int32_t Done https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:4680: From = randomizeOrPoolImmediate(llvm::dyn_cast<OperandX8632Mem>(From)); On 2015/06/12 23:48:17, stichnot wrote: > You've already done a dyn_cast<>(From) in the enclosing "if" statement, so I > think you could just use Mem here. (But you would also have to change the "From > = ..." 6 lines up to be "Mem = ...".) Done https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:4709: if (From && llvm::isa<ConstantInteger32>(From)) { On 2015/06/12 23:48:18, stichnot wrote: > "From" must be non-null, so no need to test that. Done https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:4711: randomizeOrPoolImmediate(llvm::cast<Constant>(From), RegNum); On 2015/06/12 23:48:17, stichnot wrote: > You can avoid the cast<> and the earlier isa<> check by using dyn_cast<>. Done https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:4779: OperandX8632Mem *TargetX8632::formMemoryOperand(Operand *operand, Type Ty, On 2015/06/12 23:48:17, stichnot wrote: > Variable names should be capitalized, so maybe Opnd instead? Done, replaced with Opnd. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5083: llvm::dyn_cast_or_null<ConstantInteger32>(C) On 2015/06/12 23:48:18, stichnot wrote: > Same comments here as in IceELFObjectWriter.cpp Done. shouldBePooled will be checked before pooling for each constant. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5137: static uint64_t cookie = 0; On 2015/06/12 23:48:18, stichnot wrote: > Don't use static non-const variables, as they are not thread-safe. Maybe store > the cookie in GlobalContext instead? Done. Moved getRandomizationCookie() and the cookie variable to GlobalContext class, guarded by a lock. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5149: Operand *TargetX8632::randomizeOrPoolImmediate(Constant *immediate, On 2015/06/12 23:48:18, stichnot wrote: > Capitalize "immediate" arg name. Done https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5160: ->shouldBeRandomizedOrPooled(Ctx)) { On 2015/06/12 23:48:18, stichnot wrote: > Call immediate->shouldBeRandomizedOrPooled(Ctx) directly without type checking > or casting? Done. Changed to: if(Constant *C = dyn_cast_or_null<Constant>(Immediate)){ if(C->shouldBeRandomizedOrPooled(Ctx)) { ..... } } https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5171: // andvancedPhiLowering()=>lowerAssign(). In this case we should reuse On 2015/06/12 23:48:18, stichnot wrote: > advancedPhiLowering Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5175: ConstantInteger32 *integer = llvm::cast<ConstantInteger32>(immediate); On 2015/06/12 23:48:18, stichnot wrote: > Capitalize variable names Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5177: uint64_t cookie = getRandomizationCookie(); On 2015/06/12 23:48:18, stichnot wrote: > uint32_t Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5187: } else { On 2015/06/12 23:48:18, stichnot wrote: > Don't use this pattern: > if (abc) { > def; > return; > } else { > ghi; > } > > Instead, use: > if (abc) { > def; > return; > } > ghi; Done. Changed to: if (abc) { return xyz; } if (edf) { return uvw; } assert(false); https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5196: // andvancedPhiLowering()=>lowerAssign(). In this case we should reuse On 2015/06/12 23:48:17, stichnot wrote: > advancedPhiLowering Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5203: Constant *symbol = Ctx->getConstantSym(0, label_stream.str(), true); On 2015/06/12 23:48:17, stichnot wrote: > Try to avoid using non-obvious constants as arguments to function calls. > Instead, something like this: > > const RelocOffsetT Offset = 0; > const bool SuppressMangling = true; > ... getConstantSym(Offset, label_stream.str(), SuppressMangling); > > For better or worse, the ::create() methods seem to be exceptions to this > rule... Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5218: assert(llvm::isa<OperandX8632Mem>(MemOperand)); On 2015/06/12 23:48:18, stichnot wrote: > This assert isn't very useful and should always succeed (unless > MemOperand==nullptr in which case isa<> will crash). Done. Changed to assert(MemOperand). https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5225: if (MemOperand->getOffset() && On 2015/06/12 23:48:18, stichnot wrote: > Something like this might be more compact: > > if (auto *C = llvm::dyn_cast_or_null<Constant>(MemOperand->getOffset()) { > if (C->shouldBeRandomizedOrPooled(Ctx)) { > ... > } > } Done. https://codereview.chromium.org/1185703004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5246: if (MemOperand->getBase() != NULL) On 2015/06/12 23:48:18, stichnot wrote: > nullptr, or just leave out the != part Done. https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp#newc... src/IceClFlags.cpp:110: cl::init(1)); Initialize the random number seed with value 1. https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp#newc... src/IceClFlags.cpp:269: The default option is set to None. The commented lines will be removed soon. https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h File src/IceGlobalContext.h (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h#... src/IceGlobalContext.h:417: } Method to initialize the randomization cookie and get the cookie https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h#... src/IceGlobalContext.h:491: uint32_t RandomizationCookie; Move the Randomization cookie to here from TargetX8632. https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc... src/IceOperand.cpp:497: const uint32_t X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD = 0x1; The THRESHOLD is set to 0x1. https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc... src/IceOperand.cpp:505: getType() != IceType_i8) Consider i8, 16 constant integers. https://codereview.chromium.org/1185703004/diff/80001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:467: // of Phi lowering assignment, we should set this flag to This will be removed soon. https://codereview.chromium.org/1185703004/diff/80001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1437: } I believe this new block comes from rebasing. https://codereview.chromium.org/1185703004/diff/80001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:1486: For the operations using helper calls, we should not call loOperand() and hiOPerand() anymore, as these two methods may insert new instructions whose destination variable won't be used anywhere. Such redundant variables will fail in -Om1 cases. https://codereview.chromium.org/1185703004/diff/80001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:5366: I will remove the commented if statement. That is the original way. As we add shouldBePooled flag to all constants, we now depends on that flag during pooling constants.
jfb@chromium.org changed reviewers: + jfb@chromium.org
Should the randomization immediate be per-program (as it is now) or at a smaller granularity (say per-function)? It would be useful to quote existing security research on this decision, and document your approach in the codebase with references to why you chose this implementation approach. (sorry for the driveby! I had free time and this review jumped at me) https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp#newc... src/IceClFlags.cpp:109: // cl::init(time(0))); Could you remove this? I find it quite scary since it's non-deterministic. https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h File src/IceGlobalContext.h (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h#... src/IceGlobalContext.h:408: return RandomizationCookie; This technically needs to be an atomic read, though it can be a relaxed atomic read. This will be a problem when running as a pexe because relaxed atomics are upgraded to seq_cst. Could this not be synchronized at all, and instead initialized once before IceGlobalContext is created? https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc... src/IceOperand.cpp:100: #endif `#if 0` and tabs are sad. Could you make this a comment, if it's useful to keep? https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc... src/IceOperand.cpp:497: const uint32_t X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD = 0x1; This isn't use by other files? It's currently exported to them, and could cause ODR (unlikely given the name). It should be static per LLVM's coding convention (C++ usually prefers anonymous namespace, but not LLVM).
Thanks a lot for your comments, JF.
The purpose of constant blinding and pooling is to prevent the
attacker/programmer from embedding executable code through declaring and using
constants. After constant blinding, an attacker/programmer can not precisely
learn how the constants being encoded in users' machine. And constant pooling
move the immediates to memory, this makes it much harder to reuse the constants
as executable code.
Constant blinding works in following way:
Original: (src, dst)
add 0x1234, eax
After:
mov 0x1234+cookie, temp_reg
lea -cookie[temp_reg], temp_reg
add temp_reg, eax
We use "lea" here because it won't affect flag register, I think it's much safer
than "xor".
Constant pooling:
Original: (src, dst)
add 0x1234, eax
After:
mov [memory label of 0x1234], temp_reg
add temp_reg, eax
Note in both cases, temp_reg may be assigned with "eax" here, depends on the
liveness analysis. So this approach may not require extra register.
https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):
https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:109: // cl::init(time(0)));
On 2015/06/17 11:29:18, JF wrote:
> Could you remove this? I find it quite scary since it's non-deterministic.
But this random seed is supposed to be non-deterministic, isn't it? I set the
seed to 1 just for testing. But actually we can assign the seed value through
command line options to make it deterministic even if its default value is
random.
https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):
https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:408: return RandomizationCookie;
On 2015/06/17 11:29:18, JF wrote:
> This technically needs to be an atomic read, though it can be a relaxed atomic
> read. This will be a problem when running as a pexe because relaxed atomics
are
> upgraded to seq_cst.
>
> Could this not be synchronized at all, and instead initialized once before
> IceGlobalContext is created?
Do you mean using std::atomic<int> for the randomization cookie? You are right
that this cookie just needs to be initialized once. Reading it won't cause any
synchronization problem. I'm think if I should initialize it in the constructor
of IceGlobalContext. I always hope to confine this variable in smaller scope.
https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp
File src/IceOperand.cpp (right):
https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:100: #endif
On 2015/06/17 11:29:18, JF wrote:
> `#if 0` and tabs are sad. Could you make this a comment, if it's useful to
keep?
Sorry for the unnecessary tabs. I will remove them. But I'm not so sure the
purpose of this '#if 0' here.
https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:497: const uint32_t X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD
= 0x1;
On 2015/06/17 11:29:18, JF wrote:
> This isn't use by other files? It's currently exported to them, and could
cause
> ODR (unlikely given the name). It should be static per LLVM's coding
convention
> (C++ usually prefers anonymous namespace, but not LLVM).
Yes, you are right, I should confine this to a smaller scope. It can be either a
static constant or a local constant in the shouldBeRandomizeOrPooled() method
below.
On Wed, Jun 17, 2015 at 8:20 PM, <qining@google.com> wrote: > Thanks a lot for your comments, JF. > > The purpose of constant blinding and pooling is to prevent the > attacker/programmer from embedding executable code through declaring and > using > constants. After constant blinding, an attacker/programmer can not > precisely > learn how the constants being encoded in users' machine. And constant > pooling > move the immediates to memory, this makes it much harder to reuse the > constants > as executable code. > > Constant blinding works in following way: > Original: (src, dst) > add 0x1234, eax > After: > mov 0x1234+cookie, temp_reg > lea -cookie[temp_reg], temp_reg > add temp_reg, eax > > We use "lea" here because it won't affect flag register, I think it's much > safer > than "xor". > > Constant pooling: > Original: (src, dst) > add 0x1234, eax > After: > mov [memory label of 0x1234], temp_reg > add temp_reg, eax > > Note in both cases, temp_reg may be assigned with "eax" here, depends on > the > liveness analysis. So this approach may not require extra register. I understand how it works. I'm asking for documentation because it's important for the code to reflect what research it's based on, how it implements that research, and what expected advantages are. That'll be relevant for others reading the code, and as more research happens (since it'll be clear what the state of the art was when the code was written or updated). https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp > File src/IceClFlags.cpp (right): > > > https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp#newc... > src/IceClFlags.cpp:109: // cl::init(time(0))); > On 2015/06/17 11:29:18, JF wrote: > >> Could you remove this? I find it quite scary since it's >> > non-deterministic. > > > But this random seed is supposed to be non-deterministic, isn't it? I > set the seed to 1 just for testing. But actually we can assign the seed > value through command line options to make it deterministic even if its > default value is random. This is usually done by the embedded, in this Chrome or the command-line. Defaulting to time(0) is surprising. https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h > File src/IceGlobalContext.h (right): > > > https://codereview.chromium.org/1185703004/diff/80001/src/IceGlobalContext.h#... > src/IceGlobalContext.h:408: return RandomizationCookie; > On 2015/06/17 11:29:18, JF wrote: > >> This technically needs to be an atomic read, though it can be a >> > relaxed atomic > >> read. This will be a problem when running as a pexe because relaxed >> > atomics are > >> upgraded to seq_cst. >> > > Could this not be synchronized at all, and instead initialized once >> > before > >> IceGlobalContext is created? >> > > Do you mean using std::atomic<int> for the randomization cookie? You are > right that this cookie just needs to be initialized once. Reading it > won't cause any synchronization problem. I'm think if I should > initialize it in the constructor of IceGlobalContext. I always hope to > confine this variable in smaller scope. If it can be initialized once before other threads are created then that's best. https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp > File src/IceOperand.cpp (right): > > > https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc... > src/IceOperand.cpp:100: #endif > On 2015/06/17 11:29:18, JF wrote: > >> `#if 0` and tabs are sad. Could you make this a comment, if it's >> > useful to keep? > > Sorry for the unnecessary tabs. I will remove them. But I'm not so sure > the purpose of this '#if 0' here. > > > https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc... > src/IceOperand.cpp:497: const uint32_t > X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD = 0x1; > On 2015/06/17 11:29:18, JF wrote: > >> This isn't use by other files? It's currently exported to them, and >> > could cause > >> ODR (unlikely given the name). It should be static per LLVM's coding >> > convention > >> (C++ usually prefers anonymous namespace, but not LLVM). >> > > Yes, you are right, I should confine this to a smaller scope. It can be > either a static constant or a local constant in the > shouldBeRandomizeOrPooled() method below. > > https://codereview.chromium.org/1185703004/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
Prevent memory operands being randomized twice by adding a bool flag to OperandX8632Mem. https://codereview.chromium.org/1185703004/diff/140001/src/IceInstX8632.cpp File src/IceInstX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/140001/src/IceInstX8632.cpp#n... src/IceInstX8632.cpp:94: : OperandX8632(kMem, Ty), Randomized(false), Base(Base), Offset(Offset), add bool flag Randomized. Randomized operand should not be randomized again. https://codereview.chromium.org/1185703004/diff/140001/src/IceTargetLoweringX... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/140001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5610: return MemOperand; If this memory operand has been randomized, do not randomize it again. https://codereview.chromium.org/1185703004/diff/140001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5655: NewMemOperand->Randomized = true; label this new memory operand as randomized. This memory operand should not be randomized again.
https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceClFlags.cpp#newc... src/IceClFlags.cpp:110: cl::init(1)); On 2015/06/17 04:28:54, qining wrote: > Initialize the random number seed with value 1. In production, we'll have to find a way to make the seed "random enough", while either being deterministic or having the ability to dump the seed in order to reproduce a failure. In the meantime, a fixed seed seems fine. https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/80001/src/IceOperand.cpp#newc... src/IceOperand.cpp:100: #endif On 2015/06/17 18:20:36, qining wrote: > On 2015/06/17 11:29:18, JF wrote: > > `#if 0` and tabs are sad. Could you make this a comment, if it's useful to > keep? > > Sorry for the unnecessary tabs. I will remove them. But I'm not so sure the > purpose of this '#if 0' here. Let's keep the #if code. This was meant to give a much clearer documentation of the semantics, and validate that the more complex optimized implementation gives the same result. But it's expensive enough that I never want it on except in emergencies. https://codereview.chromium.org/1185703004/diff/120001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceClFlags.cpp#new... src/IceClFlags.cpp:260: // cl::init(Ice::RPI_Randomize), remove commented lines https://codereview.chromium.org/1185703004/diff/120001/src/IceELFObjectWriter... File src/IceELFObjectWriter.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceELFObjectWriter... src/IceELFObjectWriter.cpp:491: assert(WriteAmt == sizeof(typename ConstType::PrimType) || I think this assert should be removed. The comment above, "Check that we write the full PrimType", can be moved to the line "assert(WriteAmt == EntSize);". https://codereview.chromium.org/1185703004/diff/120001/src/IceGlobalContext.h File src/IceGlobalContext.h (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceGlobalContext.h... src/IceGlobalContext.h:409: RandomizationCookieLock.lock(); You should use the std::unique_lock<> pattern (see other uses in Subzero) when possible, rather than explicit lock()/unlock(). But to echo JF's point, you need to lock reads as well, which means moving the lock(), or the unique_lock<>, to the beginning of the method. But means tons of extra lock operations, which you were clearly trying to avoid. For now, I think you can just make RandomizationCookie a const field and pass it into the ctor. I.e., make the RNG.next() calls before GlobalContext is created. In the future, we will probably want to hold this in the Cfg or TargetLowering object. https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp#new... src/IceOperand.cpp:497: static const uint32_t X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD = 0x1; Do you really mean to set such a low threshold? IceOperand.cpp represents the high-level IR, and should not contain references to a specific target like x86. Don't use such a global static variable. I think you can just move it inside the function below, and also make it a constexpr: constexpr uint32_t Threshold = 0xWhatever; https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp#new... src/IceOperand.cpp:499: // Specialization of the template member function for ConstantInteger32 It seems that you're trying to specialize for i32 on x86, but by specializing ConstantInteger32, it's forcing the same logic on all targets. You could try to move the specialization into a target-specific file, but then you'd have issues with multiply defined symbols when multiple targets are included. I think it would be better to precompute this as a flag in the Constant object. The only thing is that the Constants are created through the GlobalContext, which would have to consult the TargetLowering to get the flag's proper value. This probably means a lot of changes, across the code, so let's leave it as is for now, but add a TODO(stichnot) comment here to fix it. https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp#new... src/IceOperand.cpp:507: // if(getType() != IceType_i32) return false; remove commented code https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.h#newco... src/IceOperand.h:128: // Whether we should pool this constant. Usually Float/Doubled Relocatables Doubled ==> Double https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.h#newco... src/IceOperand.h:129: // and pooled Integers should be flaged true. flagged https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:264: bool &Flag; Can you do this? bool &const Flag; const bool OldValue; and initialize OldValue in the ctor initializer list? https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:535: // qining: In general we need to pause constant blinding or pooling Don't tag comments with your name. Use TODO(qining): or TODO(qining,stichnot) or omit it. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:1206: if (OperandX8632Mem *Mem = llvm::cast<OperandX8632Mem>(Operand)) { llvm::dyn_cast https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:1440: switch (Inst->getOp()) { Add a comment explaining why these instructions are being lowered in a separate switch statement from below. (I really don't like having to do this, but so far I haven't thought of a better solution.) https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:1673: default: Don't use default, instead just explicitly list Udiv/Sdiv/Urem/Srem. That way, if a new Arithmetic instruction is ever introduced, the compile will warn if we forget to add its i64 lowering. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:4894: if (llvm::isa<Constant>(From)) { Change to something like: if (auto *Const = llvm::dyn_cast<Constant>(From)) { and then use Const below instead of llvm::cast<Constant>(From) . https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5260: template <> struct PoolTypeConverter<int> { Please use uint32_t/uint16_t/uint8_t instead of int/short/char. (I think the unsigned versions are fine, but if not, use int*_t versions.) https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5384: // insert: lea -cookie[Reg], Reg Explain in a comment that lea is used (as opposed to, say, xor) to avoid affecting the flags. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5397: OperandX8632Mem::create(Func, IceType_i32, Reg, Offset, NULL, 0)); nullptr https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5476: if(Value == -Cookie) return MemOperand; make format Also, there's an interesting one-in-four-billion corner case where Cookie==MININT and therefore Cookie==-Cookie. Any issue there? https://codereview.chromium.org/1185703004/diff/120001/tests_lit/llvm2ice_tes... File tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll (right): https://codereview.chromium.org/1185703004/diff/120001/tests_lit/llvm2ice_tes... tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll:4: ; RUN: -sz-seed=1 -randomize-pool-immediates=randomize \ Reduce the indentation so lines don't exceed 80 columns. Follow the indentation style of other .ll files. (This is just for the RUN lines; you don't have much control over the CHECK lines below.) https://codereview.chromium.org/1185703004/diff/140001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1185703004/diff/140001/src/IceOperand.h#newco... src/IceOperand.h:130: bool shouldBePooled; Make this protected with a public getter and/or setter.
https://codereview.chromium.org/1185703004/diff/140001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1185703004/diff/140001/src/IceInstX8632.h#new... src/IceInstX8632.h:92: bool Randomized; Make this private, with getter/setter methods. https://codereview.chromium.org/1185703004/diff/140001/tests_lit/llvm2ice_tes... File tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll (right): https://codereview.chromium.org/1185703004/diff/140001/tests_lit/llvm2ice_tes... tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll:32: ; POOLING: mov e{{[a-z]*}},DWORD PTR ds:0x0 {{[0-9a-f]*[0-9a-f]*}}: R_386_32 .L$i32${{[0-9]*}} I think you can simplify {{[0-9a-f]*[0-9a-f]*}} to just {{[0-9a-f]*}} here and below.
Updated in patch set 9. https://codereview.chromium.org/1185703004/diff/120001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceClFlags.cpp#new... src/IceClFlags.cpp:260: // cl::init(Ice::RPI_Randomize), On 2015/06/19 16:51:02, stichnot wrote: > remove commented lines Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceELFObjectWriter... File src/IceELFObjectWriter.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceELFObjectWriter... src/IceELFObjectWriter.cpp:491: assert(WriteAmt == sizeof(typename ConstType::PrimType) || On 2015/06/19 16:51:02, stichnot wrote: > I think this assert should be removed. The comment above, "Check that we write > the full PrimType", can be moved to the line "assert(WriteAmt == EntSize);". Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp#new... src/IceOperand.cpp:497: static const uint32_t X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD = 0x1; On 2015/06/19 16:51:03, stichnot wrote: > Do you really mean to set such a low threshold? > > IceOperand.cpp represents the high-level IR, and should not contain references > to a specific target like x86. > > Don't use such a global static variable. I think you can just move it inside > the function below, and also make it a constexpr: > > constexpr uint32_t Threshold = 0xWhatever; Done. Chang back to 0xffff. Do we need to add a command line option for it? https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp#new... src/IceOperand.cpp:499: // Specialization of the template member function for ConstantInteger32 On 2015/06/19 16:51:03, stichnot wrote: > It seems that you're trying to specialize for i32 on x86, but by specializing > ConstantInteger32, it's forcing the same logic on all targets. You could try to > move the specialization into a target-specific file, but then you'd have issues > with multiply defined symbols when multiple targets are included. > > I think it would be better to precompute this as a flag in the Constant object. > The only thing is that the Constants are created through the GlobalContext, > which would have to consult the TargetLowering to get the flag's proper value. > > This probably means a lot of changes, across the code, so let's leave it as is > for now, but add a TODO(stichnot) comment here to fix it. Done. I've labeled that with a TODO tag. But actually this function can be moved into TargetX8632. Just add a Constant* as an argument. However, that means some type checking will be necessary. I think the only benefit of this specialization is avoiding type checking (This may not improve performance at all :( ). https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.h#newco... src/IceOperand.h:128: // Whether we should pool this constant. Usually Float/Doubled Relocatables On 2015/06/19 16:51:03, stichnot wrote: > Doubled ==> Double Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.h#newco... src/IceOperand.h:129: // and pooled Integers should be flaged true. On 2015/06/19 16:51:03, stichnot wrote: > flagged Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:264: bool &Flag; On 2015/06/19 16:51:03, stichnot wrote: > Can you do this? > > bool &const Flag; > const bool OldValue; > > and initialize OldValue in the ctor initializer list? I can initialize OldValue in the initialization list. Is this possible to do :bool& const Flag? My compiler complain about that, and I think Flag should not be a constant, as we will need to assign it in the destructor. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:535: // qining: In general we need to pause constant blinding or pooling On 2015/06/19 16:51:03, stichnot wrote: > Don't tag comments with your name. Use TODO(qining): or TODO(qining,stichnot) > or omit it. Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:1206: if (OperandX8632Mem *Mem = llvm::cast<OperandX8632Mem>(Operand)) { On 2015/06/19 16:51:03, stichnot wrote: > llvm::dyn_cast Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:1440: switch (Inst->getOp()) { On 2015/06/19 16:51:03, stichnot wrote: > Add a comment explaining why these instructions are being lowered in a separate > switch statement from below. (I really don't like having to do this, but so far > I haven't thought of a better solution.) Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:1673: default: On 2015/06/19 16:51:03, stichnot wrote: > Don't use default, instead just explicitly list Udiv/Sdiv/Urem/Srem. That way, > if a new Arithmetic instruction is ever introduced, the compile will warn if we > forget to add its i64 lowering. Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:4894: if (llvm::isa<Constant>(From)) { On 2015/06/19 16:51:03, stichnot wrote: > Change to something like: > if (auto *Const = llvm::dyn_cast<Constant>(From)) { > and then use Const below instead of llvm::cast<Constant>(From) . Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5260: template <> struct PoolTypeConverter<int> { On 2015/06/19 16:51:03, stichnot wrote: > Please use uint32_t/uint16_t/uint8_t instead of int/short/char. (I think the > unsigned versions are fine, but if not, use int*_t versions.) Done. I think both signed and unsigned should be fine, as they are only used in creating the PoolTypeConverter struct, not used directly in any logic. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5384: // insert: lea -cookie[Reg], Reg On 2015/06/19 16:51:03, stichnot wrote: > Explain in a comment that lea is used (as opposed to, say, xor) to avoid > affecting the flags. Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5397: OperandX8632Mem::create(Func, IceType_i32, Reg, Offset, NULL, 0)); On 2015/06/19 16:51:03, stichnot wrote: > nullptr Done. https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5476: if(Value == -Cookie) return MemOperand; On 2015/06/19 16:51:03, stichnot wrote: > make format > > Also, there's an interesting one-in-four-billion corner case where > Cookie==MININT and therefore Cookie==-Cookie. Any issue there? I think Cookie==MIN_INT should still be fine. Assume offset = 0x5, Cookie + offset => 0x80000005. Then -cookie = 0x80000000, so Cookie + offset - Cookie = 0x80000005 + 0x80000000 => 0x5. If offset is -5 = 0xfffffffb, Cookie + offset => 0x7ffffffb Cookie + offset - Cookie => 0xfffffffb = -5. There overflow in the calculation above, if that isn't a problem, we should be fine with here. https://codereview.chromium.org/1185703004/diff/120001/tests_lit/llvm2ice_tes... File tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll (right): https://codereview.chromium.org/1185703004/diff/120001/tests_lit/llvm2ice_tes... tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll:4: ; RUN: -sz-seed=1 -randomize-pool-immediates=randomize \ On 2015/06/19 16:51:03, stichnot wrote: > Reduce the indentation so lines don't exceed 80 columns. Follow the indentation > style of other .ll files. (This is just for the RUN lines; you don't have much > control over the CHECK lines below.) Done. https://codereview.chromium.org/1185703004/diff/140001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1185703004/diff/140001/src/IceInstX8632.h#new... src/IceInstX8632.h:92: bool Randomized; On 2015/06/19 16:57:22, stichnot wrote: > Make this private, with getter/setter methods. Done. https://codereview.chromium.org/1185703004/diff/140001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1185703004/diff/140001/src/IceOperand.h#newco... src/IceOperand.h:130: bool shouldBePooled; On 2015/06/19 16:51:03, stichnot wrote: > Make this protected with a public getter and/or setter. Done. https://codereview.chromium.org/1185703004/diff/140001/tests_lit/llvm2ice_tes... File tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll (right): https://codereview.chromium.org/1185703004/diff/140001/tests_lit/llvm2ice_tes... tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll:32: ; POOLING: mov e{{[a-z]*}},DWORD PTR ds:0x0 {{[0-9a-f]*[0-9a-f]*}}: R_386_32 .L$i32${{[0-9]*}} On 2015/06/19 16:57:22, stichnot wrote: > I think you can simplify > {{[0-9a-f]*[0-9a-f]*}} > to just > {{[0-9a-f]*}} > here and below. Done.
https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp#new... src/IceOperand.cpp:497: static const uint32_t X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD = 0x1; On 2015/06/19 20:22:25, qining wrote: > On 2015/06/19 16:51:03, stichnot wrote: > > Do you really mean to set such a low threshold? > > > > IceOperand.cpp represents the high-level IR, and should not contain references > > to a specific target like x86. > > > > Don't use such a global static variable. I think you can just move it inside > > the function below, and also make it a constexpr: > > > > constexpr uint32_t Threshold = 0xWhatever; > > Done. Chang back to 0xffff. Do we need to add a command line option for it? I think a command line option would be most excellent. (defaulting to 0xffff or whatever) https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:264: bool &Flag; On 2015/06/19 20:22:25, qining wrote: > On 2015/06/19 16:51:03, stichnot wrote: > > Can you do this? > > > > bool &const Flag; > > const bool OldValue; > > > > and initialize OldValue in the ctor initializer list? > > I can initialize OldValue in the initialization list. Is this possible to do > :bool& const Flag? My compiler complain about that, and I think Flag should not > be a constant, as we will need to assign it in the destructor. I think I was wrong about const and Flag, sorry.
Add command line option for randomization/pooling threshold in patch set 11. Thanks, https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp File src/IceOperand.cpp (right): https://codereview.chromium.org/1185703004/diff/120001/src/IceOperand.cpp#new... src/IceOperand.cpp:497: static const uint32_t X86_INT_IMMEDIATE_RANDOMIZATION_THRESHOLD = 0x1; On 2015/06/19 23:12:15, stichnot wrote: > On 2015/06/19 20:22:25, qining wrote: > > On 2015/06/19 16:51:03, stichnot wrote: > > > Do you really mean to set such a low threshold? > > > > > > IceOperand.cpp represents the high-level IR, and should not contain > references > > > to a specific target like x86. > > > > > > Don't use such a global static variable. I think you can just move it > inside > > > the function below, and also make it a constexpr: > > > > > > constexpr uint32_t Threshold = 0xWhatever; > > > > Done. Chang back to 0xffff. Do we need to add a command line option for it? > > I think a command line option would be most excellent. (defaulting to 0xffff or > whatever) Done.
https://codereview.chromium.org/1185703004/diff/220001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceGlobalContext.c... src/IceGlobalContext.cpp:270: // Initialize the randomization cookie for diversification. Maybe say "constant blinding" instead of "diversification"? Because it isn't being used for e.g. register randomization. https://codereview.chromium.org/1185703004/diff/220001/src/IceGlobalContext.h File src/IceGlobalContext.h (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceGlobalContext.h... src/IceGlobalContext.h:415: uint32_t getRandomizationCookie() { return RandomizationCookie; } Declare this method const. https://codereview.chromium.org/1185703004/diff/220001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceInstX8632.h#new... src/IceInstX8632.h:91: bool getRandomize() { return Randomized; } Call this getRandomized() for consistency. Also declare the method as const. https://codereview.chromium.org/1185703004/diff/220001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceOperand.h#newco... src/IceOperand.h:130: bool getShouldBePooled() { return shouldBePooled; } mark as const https://codereview.chromium.org/1185703004/diff/220001/src/IceOperand.h#newco... src/IceOperand.h:143: // Whether we should pool this constant. Usually Float/Double Relocatables I don't understand the mention of Relocatables? ConstantRelocatable values are never pooled, are they? https://codereview.chromium.org/1185703004/diff/220001/src/IceTargetLoweringX... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:1830: should has already been handled before"); should have https://codereview.chromium.org/1185703004/diff/220001/src/IceTargetLoweringX... src/IceTargetLoweringX8632.cpp:5150: From = Ctx->getConstantZero(Ty); I think you need to reset Const as well, e.g.: Const = Ctx->getConstantZero(Ty); From = Const; https://codereview.chromium.org/1185703004/diff/220001/tests_lit/llvm2ice_tes... File tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll (right): https://codereview.chromium.org/1185703004/diff/220001/tests_lit/llvm2ice_tes... tests_lit/llvm2ice_tests/randomize-pool-immediate-basic.ll:4: ; RUN: -sz-seed=1 -randomize-pool-immediates=randomize \ Remove the tabs.
I just noticed that "make -f Makefile.standalone check-lit" is failing on the 3 randomization tests. I suspect it's an initialization order issue.
Fixed the three randomization tests. The problem was caused by the initialization of RandomizationCookie in GlobalContext. I add an "if" statement to stop calling RNG when '-randomize-pool-immediates' is set to 'none'. https://codereview.chromium.org/1185703004/diff/220001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceGlobalContext.c... src/IceGlobalContext.cpp:270: // Initialize the randomization cookie for diversification. On 2015/06/20 17:42:16, stichnot wrote: > Maybe say "constant blinding" instead of "diversification"? Because it isn't > being used for e.g. register randomization. Done. https://codereview.chromium.org/1185703004/diff/220001/src/IceGlobalContext.h File src/IceGlobalContext.h (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceGlobalContext.h... src/IceGlobalContext.h:415: uint32_t getRandomizationCookie() { return RandomizationCookie; } On 2015/06/20 17:42:16, stichnot wrote: > Declare this method const. Done. https://codereview.chromium.org/1185703004/diff/220001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceInstX8632.h#new... src/IceInstX8632.h:91: bool getRandomize() { return Randomized; } On 2015/06/20 17:42:16, stichnot wrote: > Call this getRandomized() for consistency. Also declare the method as const. Done. https://codereview.chromium.org/1185703004/diff/220001/src/IceOperand.h File src/IceOperand.h (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceOperand.h#newco... src/IceOperand.h:130: bool getShouldBePooled() { return shouldBePooled; } On 2015/06/20 17:42:16, stichnot wrote: > mark as const Done. https://codereview.chromium.org/1185703004/diff/220001/src/IceOperand.h#newco... src/IceOperand.h:143: // Whether we should pool this constant. Usually Float/Double Relocatables On 2015/06/20 17:42:16, stichnot wrote: > I don't understand the mention of Relocatables? ConstantRelocatable values are > never pooled, are they? Done. https://codereview.chromium.org/1185703004/diff/240001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/1185703004/diff/240001/src/IceGlobalContext.c... src/IceGlobalContext.cpp:271: if (Flags.getRandomizeAndPoolImmediatesOption() != RPI_None) Add this flag to avoid using RNG. This keeps the random numbers used in nop-insertion and randomize-regalloc tests unchanged.
I took a really brief look at the generated code, and the spec2k performance with a really low blinding threshold. The slowdown didn't seem to be as much as I would have guessed, so that's good. I was also surprised that the sandboxed code sequenced for indirect calls, specifically the masking of the jump target, wasn't affected by blinding (and if it were, surely the validator would have rejected it). Then I looked through the code and realized that "magic constants" used during lowering, such as the sandboxing jump target mask, aren't affected by constant blinding/pooling. This is good. However, I think that should be documented in this CL description. State that blinding/pooling only applies to constants derived from the pexe, and not to fixed immediates that are part of the lowering process, such as masks and shift values. For the most part (if not entirely), these are the result of getConstantInt32() calls. https://codereview.chromium.org/1185703004/diff/220001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceInstX8632.h#new... src/IceInstX8632.h:91: bool getRandomize() { return Randomized; } On 2015/06/20 23:32:37, qining wrote: > On 2015/06/20 17:42:16, stichnot wrote: > > Call this getRandomized() for consistency. Also declare the method as const. > > Done. No -- it still isn't marked as const. https://codereview.chromium.org/1185703004/diff/240001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/240001/src/IceClFlags.cpp#new... src/IceClFlags.cpp:109: cl::init(time(0))); Why did this get switched back??? https://codereview.chromium.org/1185703004/diff/240001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/1185703004/diff/240001/src/IceGlobalContext.c... src/IceGlobalContext.cpp:271: if (Flags.getRandomizeAndPoolImmediatesOption() != RPI_None) On 2015/06/20 23:32:37, qining wrote: > Add this flag to avoid using RNG. This keeps the random numbers used in > nop-insertion and randomize-regalloc tests unchanged. Good, but please add a comment briefly explaining the issue and a TODO to fix it along with other RNG refactoring.
Yes, you are right. This blinding/pooling only applies to constants that appear in pexe and also memory offsets. Constants that are introduced by arithmetic lowering and arguments lowering are not affected. I have dumb question, how do I modify the description of this change list? I do think I need to change it as the content is stale. I think I should also measure the overhead of this blinding and pooling. Thanks, https://codereview.chromium.org/1185703004/diff/220001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1185703004/diff/220001/src/IceInstX8632.h#new... src/IceInstX8632.h:91: bool getRandomize() { return Randomized; } On 2015/06/21 00:05:27, stichnot wrote: > On 2015/06/20 23:32:37, qining wrote: > > On 2015/06/20 17:42:16, stichnot wrote: > > > Call this getRandomized() for consistency. Also declare the method as > const. > > > > Done. > > No -- it still isn't marked as const. Oops, I'm so sorry I forgot to mark it const. It will be fixed in the next update. https://codereview.chromium.org/1185703004/diff/240001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/240001/src/IceClFlags.cpp#new... src/IceClFlags.cpp:109: cl::init(time(0))); On 2015/06/21 00:05:27, stichnot wrote: > Why did this get switched back??? I think we can explicitly set this seed. Originally, there is only cl::init(time(0))). cl::init(1) was added by me for testing. Should I add the init(1) back and comment the time(0) again? https://codereview.chromium.org/1185703004/diff/240001/src/IceGlobalContext.cpp File src/IceGlobalContext.cpp (right): https://codereview.chromium.org/1185703004/diff/240001/src/IceGlobalContext.c... src/IceGlobalContext.cpp:271: if (Flags.getRandomizeAndPoolImmediatesOption() != RPI_None) On 2015/06/21 00:05:27, stichnot wrote: > On 2015/06/20 23:32:37, qining wrote: > > Add this flag to avoid using RNG. This keeps the random numbers used in > > nop-insertion and randomize-regalloc tests unchanged. > > Good, but please add a comment briefly explaining the issue and a TODO to fix it > along with other RNG refactoring. Done.
To modify the description, click the "Edit Issue" link in the upper left, then modify the Description box, then click the "Update Issue" button when you're done. This is the description that will be displayed by "git log", so it's important that it's a good description. https://codereview.chromium.org/1185703004/diff/240001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1185703004/diff/240001/src/IceClFlags.cpp#new... src/IceClFlags.cpp:109: cl::init(time(0))); On 2015/06/21 00:30:47, qining wrote: > On 2015/06/21 00:05:27, stichnot wrote: > > Why did this get switched back??? > > I think we can explicitly set this seed. Originally, there is only > cl::init(time(0))). cl::init(1) was added by me for testing. Should I add the > init(1) back and comment the time(0) again? Yes, please make this init(1) by default. I think it was a mistake for it to have been init(time(0)) by default in the first place. We'll have to do something besides init(1) in production, but whatever it is will surely not be time(0), so let's avoid nondeterminism in the meantime.
On 2015/06/21 00:30:48, qining wrote: > I think I should also measure the overhead of this blinding and pooling. That's true in general, and you'll do that later, but you don't need to as part of this CL, particularly since your code doesn't run by default. If you make a nontrivial change that is enabled by default, you would want to do one or more of the following: 1. Measure the effect on O2 translation time for large pexes, like spec2k.gcc or pnacl-llc.pexe. 2. Measure the effect on code quality, as measured by spec2k. 3. For a refactor or something else that shouldn't change the output, compare the spec2k asm output using -filetype=asm -asm-verbose=1 . I don't think any of these really apply here, so it's optional.
Updated the description of this issue. And changed back the default value of sz-seed to 1.
lgtm
Message was sent while issue was closed.
Committed patchset #14 (id:260001) manually as 253dc8a870cbd144ba217a44efbd07e6bcd71e97 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
