Bug 86040 - [avr]: RAMPZ is not always cleared after loading __flashN data
Summary: [avr]: RAMPZ is not always cleared after loading __flashN data
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 8.1.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2018-06-04 15:40 UTC by Luke Morrison
Modified: 2019-10-18 09:22 UTC (History)
1 user (show)

See Also:
Host:
Target: avr
Build:
Known to work:
Known to fail: 5.4.0, 7.4.0, 8.3.0, 9.2.0
Last reconfirmed: 2018-07-16 00:00:00


Attachments
Test case demonstrating the failure to restore/clear RAMPZ (220 bytes, text/plain)
2018-06-04 15:40 UTC, Luke Morrison
Details
Compiled output from avr-gcc 8.1.0 of the test case. (1.71 KB, text/plain)
2018-06-04 15:42 UTC, Luke Morrison
Details
C test case. (539 bytes, text/plain)
2018-07-19 19:45 UTC, Georg-Johann Lay
Details
C test case for movmem (277 bytes, text/plain)
2018-07-22 10:23 UTC, Georg-Johann Lay
Details
proposed patch (364 bytes, patch)
2019-10-09 09:11 UTC, Georg-Johann Lay
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Morrison 2018-06-04 15:40:35 UTC
Created attachment 44230 [details]
Test case demonstrating the failure to restore/clear RAMPZ

This is possibly related to bug 52461, but it appears that either not all cases were caught, or else a regression might have re-introduced the problem.

When data is fetched from program memory through the use of the __flashN named address spaces, the RAMPZ register is set-up with the correct high-order address byte prior to the data fetch. Most of the time, after the fetch is complete, RAMPZ is cleared back to zero. However, it appears that at least some of the time, RAMPZ is not cleared after use.

It's important that RAMPZ must be cleared after use, because some supported AVR processors (ie. XMEGA with EBI) also implicitly make use of the RAMPZ register whenever data-space memory is accessed through the Z pointer.

I am attaching a minimal example, targeting the ATxmega128A1U, which demonstrates both one case in which the correct behavior occurs, and another in which the wrong behavior occurs.

Specifically, when a single-byte value is fetched (myArray[x][y].value1) RAMPZ is not cleared. When a multi-byte value is fetched (myArray[x][y].value2) RAMPZ is correctly cleared.

This test case has been compiled against avr-gcc 5.4.0, 8.0.1, and 8.1.0, with optimization set to O0, O1, and Os. All cases exhibit the problem.
Comment 1 Luke Morrison 2018-06-04 15:42:07 UTC
Created attachment 44231 [details]
Compiled output from avr-gcc 8.1.0 of the test case.
Comment 2 Georg-Johann Lay 2018-07-16 12:12:58 UTC
IMO the problem are the early returns in avr.c::avr_out_lpm() that bypass the reset to 0 of RAMPZ at the end of that function if RAMPD is present:

http://gcc.gnu.org/viewcvs/gcc/trunk/gcc/config/avr/avr.c?view=markup&pathrev=257301#l3703

Affected is code for the devices in -mmcu=avrxmega5 and avrxmega7. A test case for 1-byte reads is

char func1 (const __flash2 char *p)
{
    return *p;
}

func1:
	movw r30,r24	 ;  16	[c=4 l=1]  *movhi/0
	ldi r18,2	 ;  11	[c=4 l=3]  movqi_insn/3
	out __RAMPZ__,r18
	elpm r24,Z
/* epilogue start */
	ret		 ;  19	[c=0 l=1]  return

I found no code to cover the wrong parts of the 2-byte and 4-byte reads.
Comment 3 Georg-Johann Lay 2018-07-19 09:51:54 UTC
...and here is code that triggers the wrong path of the 2-byte case:

typedef struct S
{
    const __flash2 struct S *p;
    struct S *q;
} S;

const __flash2 S* func2 (const S *s)
{
    return s->p->q->p;
}

$ avr-gcc -std=gnu99 -mmcu=atxmega128a1 -S foo.c -Os -dp

func2:
	movw r26,r24	 ;  20	[c=4 l=1]  *movhi/0
	ld r30,X+	 ;  6	[c=8 l=2]  *movhi/2
	ld r31,X
	adiw r30,2	 ;  8	[c=4 l=1]  addhi3_clobber/0
	ldi r18,2	 ;  9	[c=8 l=5]  *movhi/2
	out __RAMPZ__,r18
	elpm r0,Z+
	elpm r31,Z
	mov r30,r0
	ld r24,Z	 ;  15	[c=8 l=2]  *movhi/2
	ldd r25,Z+1
/* epilogue start */
	ret		 ;  23	[c=0 l=1]  return

Insn 9 is missing the reset of RAMPZ so that insn 15 loads with a high-byte of 2.
Comment 4 Georg-Johann Lay 2018-07-19 19:45:34 UTC
Created attachment 44412 [details]
C test case.
Comment 5 Georg-Johann Lay 2018-07-22 10:23:52 UTC
Created attachment 44416 [details]
C test case for movmem

The movmem from ASes __flash1 ... __flash5 is also affected.  As the place to fix I'd propose the output function avr.c::avr_out_movmem().
Comment 6 Georg-Johann Lay 2019-10-09 09:11:02 UTC
Created attachment 47009 [details]
proposed patch
Comment 7 Georg-Johann Lay 2019-10-18 06:54:05 UTC
Author: gjl
Date: Fri Oct 18 06:53:34 2019
New Revision: 277143

URL: https://gcc.gnu.org/viewcvs?rev=277143&root=gcc&view=rev
Log:
	PR target/86040
	* config/avr/avr.c (avr_out_lpm): Do not shortcut-return.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.c
Comment 8 Georg-Johann Lay 2019-10-18 09:10:52 UTC
Author: gjl
Date: Fri Oct 18 09:10:20 2019
New Revision: 277147

URL: https://gcc.gnu.org/viewcvs?rev=277147&root=gcc&view=rev
Log:
	Backport from 2019-10-18 trunk r277143.
	PR target/86040
	* config/avr/avr.c (avr_out_lpm): Do not shortcut-return.


Modified:
    branches/gcc-9-branch/gcc/ChangeLog
    branches/gcc-9-branch/gcc/config/avr/avr.c
Comment 9 Georg-Johann Lay 2019-10-18 09:13:05 UTC
Author: gjl
Date: Fri Oct 18 09:12:34 2019
New Revision: 277148

URL: https://gcc.gnu.org/viewcvs?rev=277148&root=gcc&view=rev
Log:
	Backport from 2019-10-18 trunk r277143.
	PR target/86040
	* config/avr/avr.c (avr_out_lpm): Do not shortcut-return.


Modified:
    branches/gcc-8-branch/gcc/ChangeLog
    branches/gcc-8-branch/gcc/config/avr/avr.c
Comment 10 Georg-Johann Lay 2019-10-18 09:16:47 UTC
Author: gjl
Date: Fri Oct 18 09:16:16 2019
New Revision: 277149

URL: https://gcc.gnu.org/viewcvs?rev=277149&root=gcc&view=rev
Log:
	Backport from 2019-10-18 trunk r277143.
	PR target/86040
	* config/avr/avr.c (avr_out_lpm): Do not shortcut-return.


Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/config/avr/avr.c
Comment 11 Georg-Johann Lay 2019-10-18 09:19:32 UTC
Fixed in v7.5, v8.4 and v9.3+