Bug 63223 - [avr] Make jumptables work with -Wl,--section-start,.text=
Summary: [avr] Make jumptables work with -Wl,--section-start,.text=
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.8.0
: P4 enhancement
Target Milestone: 4.9.2
Assignee: Georg-Johann Lay
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-11 07:55 UTC by Georg-Johann Lay
Modified: 2014-10-22 10:46 UTC (History)
2 users (show)

See Also:
Host:
Target: avr
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
patch for dtor direction (460 bytes, patch)
2014-10-21 14:12 UTC, Jorn Wolfgang Rennecke
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Georg-Johann Lay 2014-09-11 07:55:58 UTC
avr-gcc assumes that jumptables in section .progmem.gcc_sw_table are located in the lower 64k of flash and uses LPM to read from these tables.

This does no more hold if the code is shifted to a higher address by, e.g. -Wl,--section-start,.text=... which is commonly used with bootloadres.

This PR implements ELPM to access jumptables as needed.
Comment 1 Georg-Johann Lay 2014-09-11 08:08:50 UTC
Author: gjl
Date: Thu Sep 11 08:08:17 2014
New Revision: 215152

URL: https://gcc.gnu.org/viewcvs?rev=215152&root=gcc&view=rev
Log:
gcc/
	PR target/63223
	* config/avr/avr.md (*tablejump.3byte-pc): New insn.
	(*tablejump): Restrict to !AVR_HAVE_EIJMP_EICALL.  Add void clobber.
	(casesi): Expand to *tablejump.3byte-pc if AVR_HAVE_EIJMP_EICALL.
libgcc/
	PR target/63223
	* config/avr/libgcc.S (__tablejump2__): Rewrite to use RAMPZ, ELPM
	and R24 as needed.  Make work for all devices and .text locations.
	(__do_global_ctors, __do_global_dtors): Use word addresses.
	(__tablejump__, __tablejump_elpm__): Remove functions.
	* t-avr (LIB1ASMFUNCS): Remove _tablejump, _tablejump_elpm.
	Add _tablejump2.
	(XICALL, XIJMP): New macros.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr.md
    trunk/libgcc/ChangeLog
    trunk/libgcc/config/avr/lib1funcs.S
    trunk/libgcc/config/avr/t-avr
Comment 2 Georg-Johann Lay 2014-09-11 08:20:16 UTC
Author: gjl
Date: Thu Sep 11 08:19:45 2014
New Revision: 215153

URL: https://gcc.gnu.org/viewcvs?rev=215153&root=gcc&view=rev
Log:
gcc/
	Backport from 2014-09-11 trunk r215152.
	PR target/63223
	* config/avr/avr.md (*tablejump.3byte-pc): New insn.
	(*tablejump): Restrict to !AVR_HAVE_EIJMP_EICALL.  Add void clobber.
	(casesi): Expand to *tablejump.3byte-pc if AVR_HAVE_EIJMP_EICALL.
libgcc/
	Backport from 2014-09-11 trunk r215152.
	PR target/63223
	* config/avr/libgcc.S (__tablejump2__): Rewrite to use RAMPZ, ELPM
	and R24 as needed.  Make work for all devices and .text locations.
	(__do_global_ctors, __do_global_dtors): Use word addresses.
	(__tablejump__, __tablejump_elpm__): Remove functions.
	* t-avr (LIB1ASMFUNCS): Remove _tablejump, _tablejump_elpm.
	Add _tablejump2.
	(XICALL, XIJMP): New macros.


Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/config/avr/avr.md
    branches/gcc-4_9-branch/libgcc/ChangeLog
    branches/gcc-4_9-branch/libgcc/config/avr/lib1funcs.S
    branches/gcc-4_9-branch/libgcc/config/avr/t-avr
Comment 3 Georg-Johann Lay 2014-09-11 08:22:12 UTC
Fixed in 4.9.2.
Comment 4 Jorn Wolfgang Rennecke 2014-10-17 14:40:44 UTC
(In reply to Georg-Johann Lay from comment #1)
> Author: gjl
> Date: Thu Sep 11 08:08:17 2014
> New Revision: 215152
> 
> URL: https://gcc.gnu.org/viewcvs?rev=215152&root=gcc&view=rev
> Log:
> gcc/
> 	PR target/63223
> 	* config/avr/avr.md (*tablejump.3byte-pc): New insn.
> 	(*tablejump): Restrict to !AVR_HAVE_EIJMP_EICALL.  Add void clobber.
> 	(casesi): Expand to *tablejump.3byte-pc if AVR_HAVE_EIJMP_EICALL.
> libgcc/
> 	PR target/63223
> 	* config/avr/libgcc.S (__tablejump2__): Rewrite to use RAMPZ, ELPM
> 	and R24 as needed.  Make work for all devices and .text locations.
> 	(__do_global_ctors, __do_global_dtors): Use word addresses.

do_global_dtors is supposed to start at the start and increment from there.
I see it used to be half-way wrong and half-way correct.
(Starting at the start, decrementing for __AVR_HAVE_ELPM__, incrementing otherwise.)
However, you now made it all the way use an incorrect order - starting at the
end and incrementing from there.
Is there a rationale for this?
Comment 5 Jorn Wolfgang Rennecke 2014-10-17 14:56:18 UTC
I also observe that the cpi/cpc/brne idiom that is used throughout -
before and after your patch - is nonsentical.
Comment 6 Jorn Wolfgang Rennecke 2014-10-17 15:11:13 UTC
(In reply to Jorn Wolfgang Rennecke from comment #4)
> However, you now made it all the way use an incorrect order - starting at the
> end and incrementing from there.
Oops, I mean decrementing from there.  But the point still stands.
Comment 7 Jorn Wolfgang Rennecke 2014-10-17 15:47:54 UTC
(In reply to Jorn Wolfgang Rennecke from comment #5)
> I also observe that the cpi/cpc/brne idiom that is used throughout -
> before and after your patch - is nonsentical.

Oops, I drew conclusions from the "operation" short description of CPC that are not borne out by the detailed flag setting description.
Comment 8 Georg-Johann Lay 2014-10-21 09:31:51 UTC
(In reply to Jorn Wolfgang Rennecke from comment #4)
> (In reply to Georg-Johann Lay from comment #1)
> do_global_dtors is supposed to start at the start and increment from there.
> I see it used to be half-way wrong and half-way correct.
> (Starting at the start, decrementing for __AVR_HAVE_ELPM__, incrementing
> otherwise.)
> However, you now made it all the way use an incorrect order - starting at the
> end and incrementing from there.
> Is there a rationale for this?

The old code was broken as it decremented begainning at the start address.  The flaw never came apparent for __dtors_start = __dtors_end or with simulators that terminated in exit.

The new code uses the same traverse direction like __do_global_ctors.

Is the order of .ctors, .dtors defined in any way?  I.e. how do you express that constructor A must run before constructor B in the C program?  Same for destructors.
Comment 9 Jorn Wolfgang Rennecke 2014-10-21 14:08:21 UTC
(In reply to Georg-Johann Lay from comment #8)
> (In reply to Jorn Wolfgang Rennecke from comment #4)
> > (In reply to Georg-Johann Lay from comment #1)
> > do_global_dtors is supposed to start at the start and increment from there.
> > I see it used to be half-way wrong and half-way correct.
> > (Starting at the start, decrementing for __AVR_HAVE_ELPM__, incrementing
> > otherwise.)
> > However, you now made it all the way use an incorrect order - starting at the
> > end and incrementing from there.
> > Is there a rationale for this?
> 
> The old code was broken as it decremented begainning at the start address. 
> The flaw never came apparent for __dtors_start = __dtors_end or with
> simulators that terminated in exit.
> 
> The new code uses the same traverse direction like __do_global_ctors.
> 
> Is the order of .ctors, .dtors defined in any way?  I.e. how do you express
> that constructor A must run before constructor B in the C program?  Same for
> destructors.

The C++ standard says that destructors have to run in reverse order of completion
of constructors.
crtstuff.c:__do_global_ctors_aux starts at the first constructor, and increments from there;
crtstuff.c:__do_global_dtors_aux starts at the last destructor, and decrements from there.
Comment 10 Jorn Wolfgang Rennecke 2014-10-21 14:12:56 UTC
Created attachment 33768 [details]
patch for dtor direction

I have this patch for fixing the direction of the dtor execution,
but I got stuck trying to write a testcase.
Comment 11 Georg-Johann Lay 2014-10-22 08:47:53 UTC
(In reply to Jorn Wolfgang Rennecke from comment #10)
> Created attachment 33768 [details]
> patch for dtor direction
> 
> I have this patch for fixing the direction of the dtor execution,
> but I got stuck trying to write a testcase.

That won't work.  I'll install a working implementation soon so you can proceed.
Comment 12 Georg-Johann Lay 2014-10-22 10:46:43 UTC
Author: gjl
Date: Wed Oct 22 10:46:11 2014
New Revision: 216551

URL: https://gcc.gnu.org/viewcvs?rev=216551&root=gcc&view=rev
Log:
	PR target/63223
	* config/avr/lib1funcs.S (__do_global_dtors): Reverse execution
	order to first...last.


Modified:
    branches/gcc-4_9-branch/libgcc/ChangeLog
    branches/gcc-4_9-branch/libgcc/config/avr/lib1funcs.S