Bug 88656 - [7/8/9/10 Regression] lr clobbered by thumb prologue before __builtin_return_address(0) reads from it
Summary: [7/8/9/10 Regression] lr clobbered by thumb prologue before __builtin_return_...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 9.0
: P2 normal
Target Milestone: 7.5
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2019-01-02 10:05 UTC by Alexandre Oliva
Modified: 2019-05-28 16:08 UTC (History)
5 users (show)

See Also:
Host:
Target: arm-none-eabi
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-02-08 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Oliva 2019-01-02 10:05:30 UTC
Alas, the fix for bug 77933 broke __builtin_return_address(0) in both leaf and non-leaf functions on ARM -mthumb, because lr is now more likely to be used as a scratch register to save other high registers, but then its value is used in __builtin_return_address(0) as if it still held the return address.

This remains broken in GCC9 (I've just tried r267435).
Add "r4" to the clobber list, or compile the original testcase with -fno-omit-frame-pointer, to get:

	push	{r[47], lr}
	mov	lr, r9
	mov	r[47], r8
	push	{r[47], lr}
[	add	r7, sp, #0	];; -fno-omit-frame-pointer only
	mov	r0, lr


Add a call to e.g. bar() between the asm and the return, and you'll also see lr overwritten and then used as if it still held the return address:

	push	{r4, lr}
	mov	lr, r9
	mov	r4, r8
	push	{r4, lr}
	mov	r4, lr
	bl	bar
	movs	r0, r4

Before the patch for PR 77933, lr would not be overwritten before use in any of these cases.
Comment 1 Jakub Jelinek 2019-02-08 13:51:01 UTC
Confirmed with -march=armv6-m -mthumb -O2
void baz (void);

void *
foo (void)
{
  asm volatile("" : : : "r8", "r9", "r4");
  return __builtin_return_address (0);
}

void *
bar (void)
{
  asm volatile("" : : : "r8", "r9", "r4");
  baz ();
  return __builtin_return_address (0);
}
Comment 2 gerd 2019-05-25 12:01:47 UTC
Could it be that this is a duplicate of bug 88167?

I compiled a gcc 7.4.0 patched with the fix for 88167 and now get this result:

        push    {r4, lr}
        mov     r3, r8
        mov     r4, r9
        push    {r3, r4}
        mov     r0, lr

The patch for bug 88167 seems to be just in trunk for now. As the problem is a regression in all releases till gcc 7 I'd prefer if it could be backported into the corresponding branches.
Comment 3 Richard Earnshaw 2019-05-28 13:35:50 UTC
(In reply to gerd from comment #2)
> Could it be that this is a duplicate of bug 88167?
> 
> I compiled a gcc 7.4.0 patched with the fix for 88167 and now get this
> result:
> 
>         push    {r4, lr}
>         mov     r3, r8
>         mov     r4, r9
>         push    {r3, r4}
>         mov     r0, lr
> 
> The patch for bug 88167 seems to be just in trunk for now. As the problem is
> a regression in all releases till gcc 7 I'd prefer if it could be backported
> into the corresponding branches.

A regression is not a bug that applies in all previous releases.  A regression is where something worked in some previous releases but does not work now.
Comment 4 gerd 2019-05-28 13:50:55 UTC
(In reply to Richard Earnshaw from comment #3)
> A regression is not a bug that applies in all previous releases.  A
> regression is where something worked in some previous releases but does not
> work now.

using __builtin_return_address(0) as described in the bug report worked in gcc 5.4.0 and before. To be more specific, it worked before the fix for bug 77933 was incorporated. So to me this is a regression.
Comment 5 Richard Earnshaw 2019-05-28 14:05:35 UTC
(In reply to gerd from comment #4)
> (In reply to Richard Earnshaw from comment #3)
> > A regression is not a bug that applies in all previous releases.  A
> > regression is where something worked in some previous releases but does not
> > work now.
> 
> using __builtin_return_address(0) as described in the bug report worked in
> gcc 5.4.0 and before. To be more specific, it worked before the fix for bug
> 77933 was incorporated. So to me this is a regression.

then please update the known-to-work field accordingly.
Comment 6 gerd 2019-05-28 16:08:17 UTC
(In reply to Richard Earnshaw from comment #5)
> then please update the known-to-work field accordingly.

It seems I'm missing the rights to do so, could someone with the rights please do that.