[llvm] r194428 - Fix the recently added anyregcc convention to handle spilled operands.

Sean Silva silvas at purdue.edu
Tue Nov 12 19:08:59 PST 2013 

On Tue, Nov 12, 2013 at 9:50 PM, Filip Pizlo <fpizlo at apple.com> wrote:

>
> On Nov 12, 2013, at 6:09 PM, Sean Silva <silvas at purdue.edu> wrote:
>
>
>
>
> On Tue, Nov 12, 2013 at 7:42 PM, Andrew Trick <atrick at apple.com> wrote:
>
>>
>> On Nov 12, 2013, at 1:10 PM, Eric Christopher <echristo at gmail.com> wrote:
>>
>> On Mon, Nov 11, 2013 at 7:47 PM, Andrew Trick <atrick at apple.com> wrote:
>>
>>
>> On Nov 11, 2013, at 7:11 PM, Sean Silva <silvas at purdue.edu> wrote:
>>
>>   StartIdx = isPatchPoint ?
>> -             StartIdx + MI->getOperand(StartIdx+3).getImm() + 5 :
>> +             StartIdx + NumCallArgs + 5 :
>>              StartIdx + 2;
>>
>> Magic numbers?
>>
>>
>> Yes, I absolutely hate referring to operands by position. I was also
>> thinking of fixing this when I fixed the bug today, but haven’t figured
>> out
>> the right way to do it yet.
>>
>>
>> Enum along with the instruction?
>>
>>
>> :) That’s what I’ve done in other compilers. So how do we do it with LLVM
>> target opcodes? Any examples?
>>
>> Also the documentation as pointed out by Sean. Please do document the
>> cases he describes and probably update the code to fail in some sort
>> of spectacular manner.
>>
>>
>> anyregcc is meant to be used for patching code sequences in place of a
>> call where we just need 2-3 incoming values guaranteed to be in registers.
>> If a JIT does ask for more register args than the platform can handle,
>> which would be completely insane, then they get this spectacular error:
>>
>> "LLVM ERROR: ran out of registers during register allocation"
>>
>
> Is this one of those checks that evaporates in release builds?
>
>
> You will see this error in release builds.  It triggers the LLVM fatal
> error handler.
>

Awesome. That should avoid anything bad happening.


>
> Sorry for nagging on this but I just want to be double-triple sure since a
> slip up in this regard is basically a guaranteed to be an exploitable
> security vulnerability of the worst kind
>
>
> Unless I'm missing something, the worst case is that you die in a fire
> while compiling.  This is harmless from a security standpoint.
>
> Even if this wasn't true, I still wouldn't be trippin'.  Of course if you
> have a JIT then it carries security vulnerabilities.  There are many, many
> ways in which JITs have to be careful to ensure that they don't generate
> wrong code that is exploitable.  I don't think there is any way that LLVM
> could validate or verify that the IR it receives doesn't contain
> exploitable nonsense.  A far bigger worry than patchpoint argument overflow
> would be that the generated IR has a buffer overflow.
>
> , and it's not difficult to imagine a scenario where this number gets
> goofed via a typo, or across architectures, etc.; If it doesn't have one
> already, I would recommend making sure that JSC has a test that pushes up
> against the limit of this number)
>
>
> JSC won't have any tests for this.  In JSC there is no such thing as an
> inline cache (i.e. patchpoint) that uses AnyRegCC and has a variable number
> of arguments.  I suspect that this can be generalized to other clients of
> patchpoints.  To me, the only reason why it's vararg on the LLVM side is to
> ensure that LLVM doesn't have to know anything about the source language
> construct that caused it.
>
> As an example, in JS, inline caches arise from heap access productions,
> like:
>
> o.f = v
> v = o.f
> o[i] = v
> v = o[i]
>
> and so on.  Notice that in each of these there is never more than three
> "operands" to the production:
>  o.f = v   // two operands (o, v)
> v = o.f   // one operand (o)
> o[i] = v   // three operands (o, i, v)
> v = o[i]   // two operands (o, i)
>
> It makes no sense for JSC to test for an inline cache that has more than
> three operands because there is no such construct in JavaScript.  There is
> no way for our frontend to generate a patchpoint that isn't for one of
> these constructs.  Generalizing again, I don't think that other languages
> have constructs that would benefit from IC and that use more than three or
> so operands.
>

Yeah, now that you mention it, having more than a couple operands to the
inline cache op would results in a "curse of dimensionality", i.e. the
space of parameters that is being cached would be too sparse to be
practically useful.


>
> And FWIW, JSC's FTL JIT only uses inline caches for the o.f case, so two
> operands is the max.  AFAICT, array accesses benefit much more from
> speculative type inference than from self-modifying code.
>
> For calls, which do have arbitrary numbers of arguments, we do use inline
> caches but we never use AnyRegCC - we use WebKitJS instead.  In general, I
> don't think it would make sense for anyone to use AnyRegCC for a patchpoint
> that is being used to implement calls, since for calls you want LLVM to put
> the call arguments into your calling convention's places instead of putting
> them "anywhere".
>
> So - in the particular case of JSC there is no way for a patchpoint to
> have more than 2 arguments right now or 3 arguments in any hypothetical
> future.  In the general case I don't see a practical application of
> patchpoint+AnyRegCC that involves more than a small constant number of
> arguments (and that constant is 3 for any dynamic language construct I can
> think of right now).
>

Ah, I wasn't aware that there were these other factors capping the total
number of arguments (and I think we can assume that any architecture worth
porting to is going to have at least 3 registers for this :)


>
> OTOH, since patchpoint AnyRegCC argument overflow causes the fatal error
> handler to be triggered, you could sort of imagine writing a test for this
> on the LLVM side.
>

Yeah, based on your description it does seem like this error condition test
is better suited to be on the LLVM side.

-- Sean Silva


>
>
>
>>
>> If a front end requests anyregcc on a call that is not a patchpoint they
>> get:
>>
>> "The AnyReg calling convention is only supported by the stackmap and
>> patchpoint intrinsics."
>>
>> It would be so great to be able to check these docs in:
>> http://llvm-reviews.chandlerc.com/D1981
>>
>> You and Sean are the only reviewers I’m not sure about.
>>
>
> Oof, sorry about dropping that on the floor!
>
> -- Sean Silva
>
>
>>
>> -Andy
>>
>>
>> -eric
>>
>> -Andy
>>
>> On Mon, Nov 11, 2013 at 5:40 PM, Andrew Trick <atrick at apple.com> wrote:
>>
>>
>> Author: atrick
>> Date: Mon Nov 11 16:40:25 2013
>> New Revision: 194428
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=194428&view=rev
>> Log:
>> Fix the recently added anyregcc convention to handle spilled operands.
>>
>> Fixes <rdar://15432754> [JS] Assertion: "Folded a def to a non-store!"
>>
>> The primary purpose of anyregcc is to prevent a patchpoint's call
>> arguments and return value from being spilled. They must be available
>> in a register, although the calling convention does not pin the
>> register. It's up to the front end to avoid using this convention for
>> calls with more arguments than allocatable registers.
>>
>>
>>
>> What happens otherwise? UB? Do we expose an API to indicate the maximum
>> number available? This sounds ripe for turning into an exploitable
>> vulnerability without some way for the API user to check their assumptions
>> about how many registers LLVM will provide them with.
>>
>> Also, btw, anyregcc is not documented in LangRef. Please update the
>> documentation.
>>
>> -- Sean Silva
>>
>>
>>
>> Modified:
>>    llvm/trunk/lib/Target/X86/X86InstrInfo.cpp
>>    llvm/trunk/test/CodeGen/X86/anyregcc.ll
>>
>> Modified: llvm/trunk/lib/Target/X86/X86InstrInfo.cpp
>> URL:
>>
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86InstrInfo.cpp?rev=194428&r1=194427&r2=194428&view=diff
>>
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Target/X86/X86InstrInfo.cpp (original)
>> +++ llvm/trunk/lib/Target/X86/X86InstrInfo.cpp Mon Nov 11 16:40:25 2013
>> @@ -4207,10 +4207,19 @@ static MachineInstr* foldPatchpoint(Mach
>>   MachineInstrBuilder MIB(MF, NewMI);
>>
>>   bool isPatchPoint = MI->getOpcode() == TargetOpcode::PATCHPOINT;
>> +  // For PatchPoint, the call args are not foldable.
>> +  unsigned NumCallArgs = MI->getOperand(StartIdx+3).getImm();
>>   StartIdx = isPatchPoint ?
>> -             StartIdx + MI->getOperand(StartIdx+3).getImm() + 5 :
>> +             StartIdx + NumCallArgs + 5 :
>>              StartIdx + 2;
>>
>> +  // Return false if any operands requested for folding are not foldable
>> (not
>> +  // part of the stackmap's live values).
>> +  for (SmallVectorImpl<unsigned>::const_iterator I = Ops.begin(), E =
>> Ops.end();
>> +       I != E; ++I) {
>> +    if (*I < StartIdx)
>> +      return 0;
>> +  }
>>   // No need to fold return, the meta data, and function arguments
>>   for (unsigned i = 0; i < StartIdx; ++i)
>>     MIB.addOperand(MI->getOperand(i));
>>
>> Modified: llvm/trunk/test/CodeGen/X86/anyregcc.ll
>> URL:
>>
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/anyregcc.ll?rev=194428&r1=194427&r2=194428&view=diff
>>
>>
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/X86/anyregcc.ll (original)
>> +++ llvm/trunk/test/CodeGen/X86/anyregcc.ll Mon Nov 11 16:40:25 2013
>> @@ -8,11 +8,11 @@
>> ; Num Constants
>> ; CHECK-NEXT:   .long   0
>> ; Num Callsites
>> -; CHECK-NEXT:   .long   6
>> +; CHECK-NEXT:   .long   7
>>
>> ; test
>> ; CHECK-NEXT:   .long   0
>> -; CHECK-NEXT:   .long   L{{.*}}-_test
>> +; CHECK-LABEL:  .long   L{{.*}}-_test
>> ; CHECK-NEXT:   .short  0
>> ; 3 locations
>> ; CHECK-NEXT:   .short  3
>> @@ -39,7 +39,7 @@ entry:
>>
>> ; property access 1 - %obj is an anyreg call argument and should
>> therefore be in a register
>> ; CHECK-NEXT:   .long   1
>> -; CHECK-NEXT:   .long   L{{.*}}-_property_access1
>> +; CHECK-LABEL:  .long   L{{.*}}-_property_access1
>> ; CHECK-NEXT:   .short  0
>> ; 2 locations
>> ; CHECK-NEXT:   .short  2
>> @@ -62,7 +62,7 @@ entry:
>>
>> ; property access 2 - %obj is an anyreg call argument and should
>> therefore be in a register
>> ; CHECK-NEXT:   .long   2
>> -; CHECK-NEXT:   .long   L{{.*}}-_property_access2
>> +; CHECK-LABEL:  .long   L{{.*}}-_property_access2
>> ; CHECK-NEXT:   .short  0
>> ; 2 locations
>> ; CHECK-NEXT:   .short  2
>> @@ -86,7 +86,7 @@ entry:
>>
>> ; property access 3 - %obj is a frame index
>> ; CHECK-NEXT:   .long   3
>> -; CHECK-NEXT:   .long   L{{.*}}-_property_access3
>> +; CHECK-LABEL:  .long   L{{.*}}-_property_access3
>> ; CHECK-NEXT:   .short  0
>> ; 2 locations
>> ; CHECK-NEXT:   .short  2
>> @@ -110,7 +110,7 @@ entry:
>>
>> ; anyreg_test1
>> ; CHECK-NEXT:   .long   4
>> -; CHECK-NEXT:   .long   L{{.*}}-_anyreg_test1
>> +; CHECK-LABEL:  .long   L{{.*}}-_anyreg_test1
>> ; CHECK-NEXT:   .short  0
>> ; 14 locations
>> ; CHECK-NEXT:   .short  14
>> @@ -193,7 +193,7 @@ entry:
>>
>> ; anyreg_test2
>> ; CHECK-NEXT:   .long   5
>> -; CHECK-NEXT:   .long   L{{.*}}-_anyreg_test2
>> +; CHECK-LABEL:  .long   L{{.*}}-_anyreg_test2
>> ; CHECK-NEXT:   .short  0
>> ; 14 locations
>> ; CHECK-NEXT:   .short  14
>> @@ -274,6 +274,35 @@ entry:
>>   ret i64 %ret
>> }
>>
>> +; Test spilling the return value of an anyregcc call.
>> +;
>> +; <rdar://problem/15432754> [JS] Assertion: "Folded a def to a
>> non-store!"
>> +;
>> +; CHECK-LABEL: .long 12
>> +; CHECK-LABEL: .long L{{.*}}-_patchpoint_spilldef
>> +; CHECK-NEXT: .short 0
>> +; CHECK-NEXT: .short 3
>> +; Loc 0: Register (some register that will be spilled to the stack)
>> +; CHECK-NEXT: .byte  1
>> +; CHECK-NEXT: .byte  0
>> +; CHECK-NEXT: .short {{[0-9]+}}
>> +; CHECK-NEXT: .long  0
>> +; Loc 1: Register RDI
>> +; CHECK-NEXT: .byte  1
>> +; CHECK-NEXT: .byte  0
>> +; CHECK-NEXT: .short 5
>> +; CHECK-NEXT: .long  0
>> +; Loc 1: Register RSI
>> +; CHECK-NEXT: .byte  1
>> +; CHECK-NEXT: .byte  0
>> +; CHECK-NEXT: .short 4
>> +; CHECK-NEXT: .long  0
>> +define i64 @patchpoint_spilldef(i64 %p1, i64 %p2, i64 %p3, i64 %p4) {
>> +entry:
>> +  %result = tail call anyregcc i64 (i32, i32, i8*, i32, ...)*
>> @llvm.experimental.patchpoint.i64(i32 12, i32 15, i8* inttoptr (i64 0 to
>> i8*), i32 2, i64 %p1, i64 %p2)
>> +  tail call void asm sideeffect "nop",
>>
>> "~{ax},~{bx},~{cx},~{dx},~{bp},~{si},~{di},~{r8},~{r9},~{r10},~{r11},~{r12},~{r13},~{r14},~{r15}"()
>> nounwind
>> +  ret i64 %result
>> +}
>> +
>> declare void @llvm.experimental.patchpoint.void(i32, i32, i8*, i32, ...)
>> declare i64 @llvm.experimental.patchpoint.i64(i32, i32, i8*, i32, ...)
>> -
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131112/0c93d500/attachment.html>

More information about the llvm-commits mailing list