Bug 45263 - registers used in __do_global_ctors can get clobbered
Summary: registers used in __do_global_ctors can get clobbered
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.5.1
: P3 normal
Target Milestone: 4.6.1
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
: 44617 46706 60247 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-08-12 01:19 UTC by Alastair D'Silva
Modified: 2016-01-13 20:00 UTC (History)
11 users (show)

See Also:
Host:
Target: avr
Build:
Known to work: 4.5.4, 4.6.1
Known to fail: 4.5.3, 4.6.0
Last reconfirmed:


Attachments
Test case showing that register r20 is clobbered when calling a constructor (247 bytes, application/octet-stream)
2010-08-12 01:21 UTC, Alastair D'Silva
Details
Patch to gcc/config/avr/libgcc.S saving r20 onto the stack before calling constructors (249 bytes, patch)
2010-08-12 01:22 UTC, Alastair D'Silva
Details | Diff
alternative patch using register r15 instead of r20 avoids pushing/poping (458 bytes, patch)
2010-11-30 15:16 UTC, Michael Schulze
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alastair D'Silva 2010-08-12 01:19:57 UTC
In gcc/config/avr/libgcc.S, revision 143306, a change to __do_global_ctors & __do_global_dtors was made which makes use of register r20.

This register can be used to pass parameters to the constructors, but it is not pushed/popped from the stack, so it will get clobbered if a constructor uses that register.

I have currently worked around it by pushing/popping r20 around the call to __tablejump__, but I am not convinced that this is the correct fix (the constructors should probably push & pop r20 themselves).
Comment 1 Alastair D'Silva 2010-08-12 01:21:16 UTC
Created attachment 21460 [details]
Test case showing that register r20 is clobbered when calling a constructor
Comment 2 Alastair D'Silva 2010-08-12 01:22:19 UTC
Created attachment 21461 [details]
Patch to gcc/config/avr/libgcc.S saving r20 onto the stack before calling constructors
Comment 3 Eric Weddington 2010-08-22 13:26:11 UTC
*** Bug 44617 has been marked as a duplicate of this bug. ***
Comment 4 Achim Linder 2010-08-22 15:56:17 UTC
HelloWorld arduino sketch works now with atmega1280 & gcc-4.5.1. Thanks.
Comment 5 Lukasz Ligowski 2010-11-20 01:56:34 UTC
works for me with ATmega2560 (arduino mega) on Gentoo gcc-4.5.1
Comment 6 Richard Biener 2010-11-29 14:57:33 UTC
*** Bug 46706 has been marked as a duplicate of this bug. ***
Comment 7 Michael Schulze 2010-11-30 15:16:20 UTC
Created attachment 22579 [details]
alternative patch using register r15 instead of r20 avoids pushing/poping

In reaction to bug 29141, which is still pending and not fixed, I offer a patch that corrects the handling of global constructors. The fix proposed in bug 29141 leads to wrong handling of global constructors since gcc version 4.4.0. And this is independent of the 64kB boundary because the handling is always wrong.

With the first provided patch the register r20 was always pushed and popped but this is not necessary. The global-constructors patch avoids this and uses r15 instead of r20. Due to the compiler abi the register r15 is left unchanged by called routines, and it has to be saved/restored by routines that use it.

I applied the global-constructor patch and then the compiler generates correct code for the execution of global constructors regardless of whether the constructor is located below or above 64kB. I tested the patch by my own but I think it needs further tests. Please try it out. I think without such a patch independent of using my one or the r20 patch, this bug is a show stopper since gcc version 4.4.0.
Comment 8 Michael Schulze 2010-11-30 15:21:57 UTC
Comment on attachment 22579 [details]
alternative patch using register r15 instead of r20 avoids pushing/poping

>diff -Naur gcc-4.5.1-orig/gcc/config/avr/libgcc.S gcc-4.5.1/gcc/config/avr/libgcc.S
>--- gcc-4.5.1-orig/gcc/config/avr/libgcc.S	2009-05-23 09:16:07.000000000 +0200
>+++ gcc-4.5.1/gcc/config/avr/libgcc.S	2010-11-30 14:35:53.000000000 +0100
>@@ -792,21 +792,22 @@
> __do_global_ctors:
> 	ldi	r17, hi8(__ctors_start)
> 	ldi	r16, hh8(__ctors_start)
>-	ldi	r28, lo8(__ctors_end)
>+	ldi	r28, hh8(__ctors_end)
>+	mov	r15, r28
>+      ldi	r28, lo8(__ctors_end)
> 	ldi	r29, hi8(__ctors_end)
>-	ldi	r20, hh8(__ctors_end)
> 	rjmp	.L__do_global_ctors_start
> .L__do_global_ctors_loop:
> 	sbiw	r28, 2
>-	sbc     r20, __zero_reg__
>+	sbc     r15, __zero_reg__
> 	mov_h	r31, r29
> 	mov_l	r30, r28
>-	out     __RAMPZ__, r20
>+	out     __RAMPZ__, r15
> 	XCALL	__tablejump_elpm__
> .L__do_global_ctors_start:
> 	cpi	r28, lo8(__ctors_start)
> 	cpc	r29, r17
>-	cpc	r20, r16
>+	cpc	r15, r16
> 	brne	.L__do_global_ctors_loop
> #else
> __do_global_ctors:
>@@ -833,21 +834,22 @@
> __do_global_dtors:
> 	ldi	r17, hi8(__dtors_end)
> 	ldi	r16, hh8(__dtors_end)
>+	ldi	r28, hh8(__dtors_start)
>+	mov	r15, r28
> 	ldi	r28, lo8(__dtors_start)
> 	ldi	r29, hi8(__dtors_start)
>-	ldi	r20, hh8(__dtors_start)
> 	rjmp	.L__do_global_dtors_start
> .L__do_global_dtors_loop:
> 	sbiw	r28, 2
>-	sbc     r20, __zero_reg__
>+	sbc     r15, __zero_reg__
> 	mov_h	r31, r29
> 	mov_l	r30, r28
>-	out     __RAMPZ__, r20
>+	out     __RAMPZ__, r15
> 	XCALL	__tablejump_elpm__
> .L__do_global_dtors_start:
> 	cpi	r28, lo8(__dtors_end)
> 	cpc	r29, r17
>-	cpc	r20, r16
>+	cpc	r15, r16
> 	brne	.L__do_global_dtors_loop
> #else
> __do_global_dtors:
Comment 9 Bill Westfield 2010-12-04 03:11:23 UTC
This is essentially identical to the patch I provided for http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44617
so it looks good to me.
I had two worries:
Is R15 used anywhere else in startup code that might not obey the register save conventions (ie does do_global_ctors need to preserve R15)
The AVR has a lot of opcodes that are not valid on registers less than 16.  It doesn't look like any of these are used here, but I wasn't 100% positive that using R15 wouldn't cause problems...
Comment 10 Eric Weddington 2010-12-05 19:52:33 UTC
(In reply to comment #9)

> Is R15 used anywhere else in startup code that might not obey the register save
> conventions (ie does do_global_ctors need to preserve R15)

I strongly suggest not using R15. The ATtiny10 Family of devices (ATtiny10/4/5/9/20/40) only has R16-R31. So using R15 won't work for this class of devices. It would be best if we can find a register that will for all AVR devices.
Comment 11 Michael Schulze 2010-12-06 09:07:19 UTC
(In reply to comment #9)
> This is essentially identical to the patch I provided for
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44617
> so it looks good to me.
I hadn't see that patch but you are right it's almost identical. 

> I had two worries:
> Is R15 used anywhere else in startup code that might not obey the register
> save conventions (ie does do_global_ctors need to preserve R15)
> The AVR has a lot of opcodes that are not valid on registers less than 16.  It
> doesn't look like any of these are used here, but I wasn't 100% positive that
> using R15 wouldn't cause problems...
As far as I know r15 isn't used elsewhere in the start-up code and also your
second worry seems not to be a problem, because such instructions are not used
by that time.


(In reply to comment #10)
> I strongly suggest not using R15. The ATtiny10 Family of devices
> (ATtiny10/4/5/9/20/40) only has R16-R31. So using R15 won't work for this
> class of devices.
I disagree because the additional register is only in the start up code of
devices that have rampz and is omitted in all other cases. The ATtiny don't 
have a rampz (correct me if I'm wrong) thus they are not faced with the 
discussed problem in general. IMO, it is absolutely possible and correct to use 
r15 here. 

> It would be best if we can find a register that will for all AVR
> devices.
Due to the compiler's C calling conventions, there are no other free registers to use without the need of pushing and popping this registers. Only registers below r16 are free to use. All other free r16,r17,r28,r29 are already in use.
Comment 12 Georg-Johann Lay 2011-04-13 16:36:57 UTC
Author: gjl
Date: Wed Apr 13 16:36:50 2011
New Revision: 172384

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=172384
Log:
	PR target/45263
	* config/avr/libgcc.S (__do_global_ctors, __do_global_dtors): Save
	R20 around calls of __tablejump_elpm__


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/libgcc.S
Comment 13 Georg-Johann Lay 2011-04-13 16:46:32 UTC
Author: gjl
Date: Wed Apr 13 16:46:29 2011
New Revision: 172385

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=172385
Log:
Fix ChangeLog entry for PR target/45263


Modified:
    trunk/gcc/ChangeLog
Comment 14 Georg-Johann Lay 2011-04-14 15:40:11 UTC
Closed as resolved+fixed in 4.7.0
Comment 15 Georg-Johann Lay 2011-05-30 08:53:15 UTC
Author: gjl
Date: Mon May 30 08:53:12 2011
New Revision: 174427

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=174427
Log:
	PR target/45263
	* config/avr/libgcc.S (__do_global_ctors, __do_global_dtors):
	Don't use r20 around calls of __tablejump_elpm__

Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/config/avr/libgcc.S
Comment 16 Georg-Johann Lay 2012-05-02 17:23:14 UTC
Author: gjl
Date: Wed May  2 17:23:06 2012
New Revision: 187058

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187058
Log:
	Backport from 2011-05-30 4.6-branch r174427.
	PR target/45263
	* config/avr/libgcc.S (__do_global_ctors, __do_global_dtors):
	Don't use r20 around calls of __tablejump_elpm__


Modified:
    branches/gcc-4_5-branch/gcc/ChangeLog
    branches/gcc-4_5-branch/gcc/config/avr/libgcc.S
Comment 17 Georg-Johann Lay 2012-05-02 17:24:38 UTC
Backport to 4.5.4
Comment 18 Georg-Johann Lay 2014-03-31 20:09:25 UTC
*** Bug 60247 has been marked as a duplicate of this bug. ***