-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[aarch64] XOR the frame pointer with the stack cookie when protecting the stack #161114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 11 commits
9c6d4c7
da07652
00dddda
5c230ae
12ded1e
23fbbbb
c61707a
2b02cb2
823bd2f
9f95a65
6606db9
c27d13d
6536678
4493e86
c5b61ae
2763964
414995a
65acb8c
df09547
2e737b9
b9ae3ff
bb1bf30
33de52f
b41ab42
36782f4
5076be4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2497,6 +2497,15 @@ bool AArch64InstrInfo::expandPostRAPseudo(MachineInstr &MI) const { | |
| .addMemOperand(*MI.memoperands_begin()); | ||
| } | ||
| } | ||
| // To match MSVC | ||
|
||
| if (Subtarget.getTargetTriple().isOSMSVCRT() && | ||
| !Subtarget.getTargetLowering() | ||
efriedma-quic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ->getTargetMachine() | ||
| .Options.EnableGlobalISel) { | ||
| BuildMI(MBB, MI, DL, get(AArch64::SUBXrr), Reg) | ||
| .addReg(AArch64::SP) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this actually produce the correct instruction? I'm not sure SP is in the right register class; I suspect you end up subtracting from zero. The wouldn't show up in assembly output, but it would show up in disassembly. (SUBXrx64 can refer to SP this way, though.)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reply here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed SUBXrr to SUBXrx64. Could you please check it? |
||
| .addReg(Reg, RegState::Kill); | ||
| } | ||
|
|
||
| MBB.erase(MI); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5 | ||
| ; RUN: llc < %s -mtriple=aarch64-w64-mingw32 | FileCheck %s --check-prefixes=CHECK,CHECK-SD | ||
| ; RUN: llc < %s -mtriple=aarch64-w64-mingw32 -global-isel | FileCheck %s --check-prefixes=CHECK,CHECK-GI | ||
| ; RUN: llc < %s -mtriple=aarch64-w64-mingw32 -global-isel | FileCheck %s --check-prefixes=CHECK-GI | ||
|
||
|
|
||
| @var = external local_unnamed_addr global i32, align 4 | ||
| @dsolocalvar = external dso_local local_unnamed_addr global i32, align 4 | ||
|
|
@@ -15,6 +15,13 @@ define dso_local i32 @getVar() { | |
| ; CHECK-NEXT: ldr x8, [x8, :lo12:.refptr.var] | ||
| ; CHECK-NEXT: ldr w0, [x8] | ||
| ; CHECK-NEXT: ret | ||
| ; | ||
| ; CHECK-GI-LABEL: getVar: | ||
| ; CHECK-GI: // %bb.0: // %entry | ||
| ; CHECK-GI-NEXT: adrp x8, .refptr.var | ||
| ; CHECK-GI-NEXT: ldr x8, [x8, :lo12:.refptr.var] | ||
| ; CHECK-GI-NEXT: ldr w0, [x8] | ||
| ; CHECK-GI-NEXT: ret | ||
| entry: | ||
| %0 = load i32, ptr @var, align 4 | ||
| ret i32 %0 | ||
|
|
@@ -26,6 +33,12 @@ define dso_local i32 @getDsoLocalVar() { | |
| ; CHECK-NEXT: adrp x8, dsolocalvar | ||
| ; CHECK-NEXT: ldr w0, [x8, :lo12:dsolocalvar] | ||
| ; CHECK-NEXT: ret | ||
| ; | ||
| ; CHECK-GI-LABEL: getDsoLocalVar: | ||
| ; CHECK-GI: // %bb.0: // %entry | ||
| ; CHECK-GI-NEXT: adrp x8, dsolocalvar | ||
| ; CHECK-GI-NEXT: ldr w0, [x8, :lo12:dsolocalvar] | ||
| ; CHECK-GI-NEXT: ret | ||
| entry: | ||
| %0 = load i32, ptr @dsolocalvar, align 4 | ||
| ret i32 %0 | ||
|
|
@@ -37,6 +50,12 @@ define dso_local i32 @getLocalVar() { | |
| ; CHECK-NEXT: adrp x8, localvar | ||
| ; CHECK-NEXT: ldr w0, [x8, :lo12:localvar] | ||
| ; CHECK-NEXT: ret | ||
| ; | ||
| ; CHECK-GI-LABEL: getLocalVar: | ||
| ; CHECK-GI: // %bb.0: // %entry | ||
| ; CHECK-GI-NEXT: adrp x8, localvar | ||
| ; CHECK-GI-NEXT: ldr w0, [x8, :lo12:localvar] | ||
| ; CHECK-GI-NEXT: ret | ||
| entry: | ||
| %0 = load i32, ptr @localvar, align 4 | ||
| ret i32 %0 | ||
|
|
@@ -48,6 +67,12 @@ define dso_local i32 @getLocalCommon() { | |
| ; CHECK-NEXT: adrp x8, localcommon | ||
| ; CHECK-NEXT: ldr w0, [x8, :lo12:localcommon] | ||
| ; CHECK-NEXT: ret | ||
| ; | ||
| ; CHECK-GI-LABEL: getLocalCommon: | ||
| ; CHECK-GI: // %bb.0: // %entry | ||
| ; CHECK-GI-NEXT: adrp x8, localcommon | ||
| ; CHECK-GI-NEXT: ldr w0, [x8, :lo12:localcommon] | ||
| ; CHECK-GI-NEXT: ret | ||
| entry: | ||
| %0 = load i32, ptr @localcommon, align 4 | ||
| ret i32 %0 | ||
|
|
@@ -60,6 +85,13 @@ define dso_local i32 @getExtVar() { | |
| ; CHECK-NEXT: ldr x8, [x8, :lo12:__imp_extvar] | ||
| ; CHECK-NEXT: ldr w0, [x8] | ||
| ; CHECK-NEXT: ret | ||
| ; | ||
| ; CHECK-GI-LABEL: getExtVar: | ||
| ; CHECK-GI: // %bb.0: // %entry | ||
| ; CHECK-GI-NEXT: adrp x8, __imp_extvar | ||
| ; CHECK-GI-NEXT: ldr x8, [x8, :lo12:__imp_extvar] | ||
| ; CHECK-GI-NEXT: ldr w0, [x8] | ||
| ; CHECK-GI-NEXT: ret | ||
| entry: | ||
| %0 = load i32, ptr @extvar, align 4 | ||
| ret i32 %0 | ||
|
|
@@ -69,6 +101,10 @@ define dso_local void @callFunc() { | |
| ; CHECK-LABEL: callFunc: | ||
| ; CHECK: // %bb.0: // %entry | ||
| ; CHECK-NEXT: b otherFunc | ||
| ; | ||
| ; CHECK-GI-LABEL: callFunc: | ||
| ; CHECK-GI: // %bb.0: // %entry | ||
| ; CHECK-GI-NEXT: b otherFunc | ||
| entry: | ||
| tail call void @otherFunc() | ||
| ret void | ||
|
|
@@ -89,12 +125,14 @@ define dso_local void @sspFunc() #0 { | |
| ; CHECK-NEXT: add x0, sp, #7 | ||
| ; CHECK-NEXT: ldr x8, [x8, :lo12:.refptr.__stack_chk_guard] | ||
| ; CHECK-NEXT: ldr x8, [x8] | ||
| ; CHECK-NEXT: sub x8, sp, x8 | ||
| ; CHECK-NEXT: str x8, [sp, #8] | ||
| ; CHECK-NEXT: bl ptrUser | ||
| ; CHECK-NEXT: adrp x8, .refptr.__stack_chk_guard | ||
| ; CHECK-NEXT: ldr x8, [x8, :lo12:.refptr.__stack_chk_guard] | ||
| ; CHECK-NEXT: ldr x9, [sp, #8] | ||
| ; CHECK-NEXT: ldr x8, [x8] | ||
| ; CHECK-NEXT: sub x8, sp, x8 | ||
| ; CHECK-NEXT: cmp x8, x9 | ||
| ; CHECK-NEXT: b.ne .LBB6_2 | ||
| ; CHECK-NEXT: // %bb.1: // %entry | ||
|
|
@@ -110,6 +148,40 @@ define dso_local void @sspFunc() #0 { | |
| ; CHECK-NEXT: brk #0x1 | ||
| ; CHECK-NEXT: .seh_endfunclet | ||
| ; CHECK-NEXT: .seh_endproc | ||
| ; | ||
| ; CHECK-GI-LABEL: sspFunc: | ||
| ; CHECK-GI: .seh_proc sspFunc | ||
| ; CHECK-GI-NEXT: // %bb.0: // %entry | ||
| ; CHECK-GI-NEXT: sub sp, sp, #32 | ||
| ; CHECK-GI-NEXT: .seh_stackalloc 32 | ||
| ; CHECK-GI-NEXT: str x30, [sp, #16] // 8-byte Spill | ||
| ; CHECK-GI-NEXT: .seh_save_reg x30, 16 | ||
| ; CHECK-GI-NEXT: .seh_endprologue | ||
| ; CHECK-GI-NEXT: adrp x8, .refptr.__stack_chk_guard | ||
| ; CHECK-GI-NEXT: add x0, sp, #7 | ||
| ; CHECK-GI-NEXT: ldr x8, [x8, :lo12:.refptr.__stack_chk_guard] | ||
| ; CHECK-GI-NEXT: ldr x8, [x8] | ||
| ; CHECK-GI-NEXT: str x8, [sp, #8] | ||
| ; CHECK-GI-NEXT: bl ptrUser | ||
| ; CHECK-GI-NEXT: adrp x8, .refptr.__stack_chk_guard | ||
| ; CHECK-GI-NEXT: ldr x8, [x8, :lo12:.refptr.__stack_chk_guard] | ||
| ; CHECK-GI-NEXT: ldr x9, [sp, #8] | ||
| ; CHECK-GI-NEXT: ldr x8, [x8] | ||
| ; CHECK-GI-NEXT: cmp x8, x9 | ||
| ; CHECK-GI-NEXT: b.ne .LBB6_2 | ||
| ; CHECK-GI-NEXT: // %bb.1: // %entry | ||
| ; CHECK-GI-NEXT: .seh_startepilogue | ||
| ; CHECK-GI-NEXT: ldr x30, [sp, #16] // 8-byte Reload | ||
| ; CHECK-GI-NEXT: .seh_save_reg x30, 16 | ||
| ; CHECK-GI-NEXT: add sp, sp, #32 | ||
| ; CHECK-GI-NEXT: .seh_stackalloc 32 | ||
| ; CHECK-GI-NEXT: .seh_endepilogue | ||
| ; CHECK-GI-NEXT: ret | ||
| ; CHECK-GI-NEXT: .LBB6_2: // %entry | ||
| ; CHECK-GI-NEXT: bl __stack_chk_fail | ||
| ; CHECK-GI-NEXT: brk #0x1 | ||
| ; CHECK-GI-NEXT: .seh_endfunclet | ||
| ; CHECK-GI-NEXT: .seh_endproc | ||
| entry: | ||
| %c = alloca i8, align 1 | ||
| call void @llvm.lifetime.start.p0(i64 1, ptr nonnull %c) | ||
|
|
@@ -134,5 +206,4 @@ attributes #0 = { sspstrong } | |
| ; CHECK: .xword var | ||
|
|
||
| ;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line: | ||
| ; CHECK-GI: {{.*}} | ||
| ; CHECK-SD: {{.*}} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use the result of getRegister as an operand to a target-independent ISD::ADD. Register operands aren't values; they are intended only for use with specific operations which are expecting them.
It might appear to work here, sort-of, but other parts of the DAG aren't expecting it, and it will lead to weird results in general. And I'm not sure it actually is working correctly here... what's the actual encoding you're getting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running llvm/test/CodeGen/AArch64/stack-protector-target.ll, the SDValue returned by DAG.getRegister (getRegister(getStackPointerRegisterToSaveRestore(), MVT:: i64) is "t4: i64 = Register $physreg8".
DAG.getRegister wraps Register as SDValue, while DAG.getCopyFromReg (used in previous version) uses DAG.getRegister and adds a move (copy) instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the target-independent ISD::ADD to the target-independent class SelectionDAG. Could you please further review it?