Bug 53256

Summary:	[avr] Attribute 'interrupt' shall override attribute 'signal'
Product:	gcc	Reporter:	Georg-Johann Lay <gjl>
Component:	target	Assignee:	Georg-Johann Lay <gjl>
Status:	RESOLVED FIXED
Severity:	enhancement	CC:	j
Priority:	P4
Version:	4.7.0
Target Milestone:	4.7.1
Host:		Target:	avr
Build:		Known to work:
Known to fail:		Last reconfirmed:	2012-05-06 00:00:00

Description Georg-Johann Lay 2012-05-06 11:41:03 UTC

AVR-LibC offered attributes 'interrupt' and 'signal' as INTERRUPT and SIGNAL through its API.  This changed at some point in time to ISR which may lead to code that adds attribute 'interrupt' and attribute 'signal' at the same time.

This leads to code that is not IRQ-safe in the current implementation which is:

1) Attributes 'interrupt' and 'signal' are exclusive. Specifying both at the same time for the same function has not a well-defined behavior.

2) 'interrupt' emits a SEI instruction as the very first instruction of the function.

3) Neither attributes make assumptions on the function's worker code. In particular, they do *not* assume that interrupts are not (re-)enabled by the user code.  Thus, it is safe to enable IRQs at any place in the function body.  This "safe" covers only the generated code and IRQ-safeness of SP changes and the instructions themselves, it does *not* cover the IRQ and timing layout of the application.

4) The AVR hardware globally disables IRQs when an IRQ is raised. This allows more efficient code in 'signal' prologues compared to 'interrupt' prologues in cases where the stack pointer has to be written.  Writing SP is not an atomic operation on all devices, and special precautions must be taken if SP is written and IRQs might be on.


As of this extension request 1) shall be changed to:

1') Attribute 'interrupt' shall override attribute 'signal'. No warning should be issued in such a case.  The documentation should be extended.

Comment 1 Georg-Johann Lay 2012-05-09 16:29:06 UTC

Author: gjl
Date: Wed May  9 16:28:53 2012
New Revision: 187342

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187342
Log:
	PR target/53256
	* config/avr/elf.h (ASM_DECLARE_FUNCTION_NAME): Remove.
	* config/avr/avr-protos.h (avr_asm_declare_function_name): Remove.
	* config/avr/avr.h (struct machine_function): Add attributes_checked_p.
	* config/avr/avr.c (avr_asm_declare_function_name): Remove.
	(expand_prologue): Move initialization of cfun->machine->is_naked,
	is_interrupt, is_signal, is_OS_task, is_OS_main from here to...
	(avr_set_current_function): ...this new static function.
	(TARGET_SET_CURRENT_FUNCTION): New define.
	(avr_function_ok_for_sibcall): Use cfun->machine->is_* instead of
	checking attributes of current_function_decl.
	(avr_regs_to_save): Ditto.
	(signal_function_p): Rename to avr_signal_function_p.
	(interrupt_function_p): Rename to avr_interrupt_function_p.
	* doc/extend.texi (Function Attributes): Better explanation of
	'interrupt' and 'signal' for AVR. Move 'ifunc' down to establish
	alphabetical order.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/avr/avr-protos.h
    trunk/gcc/config/avr/avr.c
    trunk/gcc/config/avr/avr.h
    trunk/gcc/config/avr/elf.h
    trunk/gcc/doc/extend.texi

Comment 2 Georg-Johann Lay 2012-05-09 16:39:39 UTC

Author: gjl
Date: Wed May  9 16:39:33 2012
New Revision: 187343

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187343
Log:
	Backport from 2012-05-12 mainline r187342.
	PR target/53256
	* config/avr/elf.h (ASM_DECLARE_FUNCTION_NAME): Remove.
	* config/avr/avr-protos.h (avr_asm_declare_function_name): Remove.
	* config/avr/avr.h (struct machine_function): Add attributes_checked_p.
	* config/avr/avr.c (avr_asm_declare_function_name): Remove.
	(expand_prologue): Move initialization of cfun->machine->is_naked,
	is_interrupt, is_signal, is_OS_task, is_OS_main from here to...
	(avr_set_current_function): ...this new static function.
	(TARGET_SET_CURRENT_FUNCTION): New define.
	(avr_function_ok_for_sibcall): Use cfun->machine->is_* instead of
	checking attributes of current_function_decl.
	(avr_regs_to_save): Ditto.
	(signal_function_p): Rename to avr_signal_function_p.
	(interrupt_function_p): Rename to avr_interrupt_function_p.
	* doc/extend.texi (Function Attributes): Better explanation of
	'interrupt' and 'signal' for AVR. Move 'ifunc' down to establish
	alphabetical order.


Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/avr/avr-protos.h
    branches/gcc-4_7-branch/gcc/config/avr/avr.c
    branches/gcc-4_7-branch/gcc/config/avr/avr.h
    branches/gcc-4_7-branch/gcc/config/avr/elf.h
    branches/gcc-4_7-branch/gcc/doc/extend.texi

Comment 3 Georg-Johann Lay 2012-05-09 17:03:49 UTC

Added in 4.7.1

Comment 4 Georg-Johann Lay 2012-06-08 22:42:27 UTC

See also the following AVR-LibC bug report:

http://savannah.nongnu.org/bugs/index.php?26734