Bug 83370 - [AARCH64]Tailcall register may be corrupted by epilogue code
Summary: [AARCH64]Tailcall register may be corrupted by epilogue code
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: 6.5
Assignee: Renlin Li
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2017-12-11 15:20 UTC by Renlin Li
Modified: 2018-02-02 11:02 UTC (History)
1 user (show)

See Also:
Host:
Target: aarch64
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-02-01 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Renlin Li 2017-12-11 15:20:26 UTC
The following example generates incorrect code:

void (*f)();
int xx;
void tailcall (int i)
{
   int arr[5000];
   xx = arr[i];
   f();
}

When built with -O2 -ffixed-x0 -ffixed-x1 -ffixed-x2 -ffixed-x3 -ffixed-x4 -ffixed-x5 -ffixed-x6 -ffixed-x7 -ffixed-x8 -ffixed-x9 -ffixed-x10 -ffixed-x11 -ffixed-x12 -ffixed-x13 -ffixed-x14 -ffixed-x15 -ffixed-x17 -ffixed-x18

tailcall:
	mov	x16, 20016
	sub	sp, sp, x16
	adrp	x16, .LANCHOR0
	stp	x19, x30, [sp]
	add	x19, sp, 16
	ldr	s0, [x19, w0, sxtw 2]
	ldp	x19, x30, [sp]
	str	s0, [x16, #:lo12:.LANCHOR0]
	mov	x16, 20016
	add	sp, sp, x16
	br	x16   // oops

So the issue is there is nothing in the tail call instruction that prevents it from using IP0/IP1 which are used as temporaries in the epilogue. We use the temporary for frames of 4-64KB, so this issue is more likely today (previously temporary was used only in frames larger than 16MBytes).

The problem appears to be that while we have explicit clobbers in a tailcall, they are after the call, not before it:

(call_insn/j 16 12 17 2 (parallel [
            (call (mem:DI (reg/f:DI 84 [ f ]) [0 *f.0_2 S8 A8])
                (const_int 0 [0]))
            (return)
        ]) "tailcall.c":13 42 {*sibcall_insn}
     (expr_list:REG_DEAD (reg/f:DI 84 [ f ])
        (expr_list:REG_CALL_DECL (nil)
            (nil)))
    (expr_list (clobber (reg:DI 17 x17))
        (expr_list (clobber (reg:DI 16 x16))
            (nil))))

This issues affects gcc-5, gcc-6, gcc-7 and current trunk.
Comment 1 Renlin Li 2018-02-01 13:02:55 UTC
Author: renlin
Date: Thu Feb  1 13:02:24 2018
New Revision: 257294

URL: https://gcc.gnu.org/viewcvs?rev=257294&root=gcc&view=rev
Log:
[PR83370][AARCH64]Use tighter register constraint for sibcall patterns.

In aarch64 backend, ip0/ip1 register will be used in the prologue/epilogue as
temporary register.

When the compiler is performing sibcall optimization. It has the chance to use
ip0/ip1 register for indirect function call to hold the address. However,
those two register might be clobbered by the epilogue code which makes the
last sibcall instruction invalid.

The patch here renames the register class CALLER_SAVE_REGS to TAILCALL_ADDR_REGS
to reflect its usage, and remove IP registers from this class.

gcc/

2018-02-01  Renlin Li  <renlin.li@arm.com>

	PR target/83370
	* config/aarch64/aarch64.c (aarch64_class_max_nregs): Handle
	TAILCALL_ADDR_REGS.
	(aarch64_register_move_cost): Likewise.
	* config/aarch64/aarch64.h (reg_class): Rename CALLER_SAVE_REGS to
	TAILCALL_ADDR_REGS.
	(REG_CLASS_NAMES): Likewise.
	(REG_CLASS_CONTENTS): Rename CALLER_SAVE_REGS to
	TAILCALL_ADDR_REGS. Remove IP registers.
	* config/aarch64/aarch64.md (Ucs): Update register constraint.

gcc/testsuite/

2018-02-01  Richard Sandiford  <richard.sandiford@linaro.org>

	PR target/83370
	* gcc.target/aarch64/pr83370.c: New.


Added:
    trunk/gcc/testsuite/gcc.target/aarch64/pr83370.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/aarch64/aarch64.c
    trunk/gcc/config/aarch64/aarch64.h
    trunk/gcc/config/aarch64/constraints.md
    trunk/gcc/testsuite/ChangeLog
Comment 2 Renlin Li 2018-02-01 13:05:43 UTC
fix has been commit in trunk.
Comment 3 Richard Earnshaw 2018-02-01 13:58:54 UTC
Doesn't this need backporting?
Comment 4 Renlin Li 2018-02-01 21:09:38 UTC
Author: renlin
Date: Thu Feb  1 21:09:06 2018
New Revision: 257314

URL: https://gcc.gnu.org/viewcvs?rev=257314&root=gcc&view=rev
Log:
[PR83370][AARCH64]Use tighter register constraint for sibcall patterns.

gcc/

	backport from mainline
	2018-02-01  Renlin Li  <renlin.li@arm.com>

	PR target/83370
	* config/aarch64/aarch64.c (aarch64_class_max_nregs): Handle
	TAILCALL_ADDR_REGS.
	(aarch64_register_move_cost): Likewise.
	* config/aarch64/aarch64.h (reg_class): Rename CALLER_SAVE_REGS to
	TAILCALL_ADDR_REGS.
	(REG_CLASS_NAMES): Likewise.
	(REG_CLASS_CONTENTS): Rename CALLER_SAVE_REGS to
	TAILCALL_ADDR_REGS. Remove IP registers.
	* config/aarch64/aarch64.md (Ucs): Update register constraint.

gcc/testsuite/

	backport from mainline
	2018-02-01  Richard Sandiford  <richard.sandiford@linaro.org>

	PR target/83370
	* gcc.target/aarch64/pr83370.c: New.

Added:
    branches/gcc-7-branch/gcc/testsuite/gcc.target/aarch64/pr83370.c
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/config/aarch64/aarch64.c
    branches/gcc-7-branch/gcc/config/aarch64/aarch64.h
    branches/gcc-7-branch/gcc/config/aarch64/constraints.md
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 5 Renlin Li 2018-02-01 21:33:37 UTC
Author: renlin
Date: Thu Feb  1 21:33:05 2018
New Revision: 257315

URL: https://gcc.gnu.org/viewcvs?rev=257315&root=gcc&view=rev
Log:
[PR83370][AARCH64]Use tighter register constraint for sibcall patterns.

gcc/

	backport from mainline
	2018-02-01  Renlin Li  <renlin.li@arm.com>

	PR target/83370
	* config/aarch64/aarch64.c (aarch64_class_max_nregs): Handle
	TAILCALL_ADDR_REGS.
	(aarch64_register_move_cost): Likewise.
	* config/aarch64/aarch64.h (reg_class): Rename CALLER_SAVE_REGS to
	TAILCALL_ADDR_REGS.
	(REG_CLASS_NAMES): Likewise.
	(REG_CLASS_CONTENTS): Rename CALLER_SAVE_REGS to
	TAILCALL_ADDR_REGS. Remove IP registers.
	* config/aarch64/aarch64.md (Ucs): Update register constraint.

gcc/testsuite/

	backport from mainline
	2018-02-01  Richard Sandiford  <richard.sandiford@linaro.org>

	PR target/83370
	* gcc.target/aarch64/pr83370.c: New.


Added:
    branches/gcc-6-branch/gcc/testsuite/gcc.target/aarch64/pr83370.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/config/aarch64/aarch64.c
    branches/gcc-6-branch/gcc/config/aarch64/aarch64.h
    branches/gcc-6-branch/gcc/config/aarch64/constraints.md
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
Comment 6 Renlin Li 2018-02-02 10:48:38 UTC
(In reply to Richard Earnshaw from comment #3)
> Doesn't this need backporting?


Yes, it is needed. The same problem happens in gcc-6 and gcc-7.
The backporting is approved and committed now.