[Patch, avr] Fix PR65210

[Patch, avr] Fix PR65210

[Senthil Kumar Selvaraj]

Hi,

  This (rather trivial) patch fixes PR65210. The ICE happens because code
  wasn't handling io_low attribute where it is supposed to.

  If this is ok, could someone commit please? I don't have commit
  access.

Regards
Senthil

gcc/ChangeLog

2015-09-02  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	PR target/65210
	* config/avr/avr.c (avr_eval_addr_attrib): Look for io_low
	attribute as well.

gcc/testsuite/ChangeLog

	PR target/65210
	* gcc.target/avr/pr65210.c: New test.

diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
index bec9a8b..9f5bc88 100644
--- gcc/config/avr/avr.c
+++ gcc/config/avr/avr.c
@@ -9069,6 +9069,8 @@ avr_eval_addr_attrib (rtx x)
       if (SYMBOL_REF_FLAGS (x) & SYMBOL_FLAG_IO)
 	{
 	  attr = lookup_attribute ("io", DECL_ATTRIBUTES (decl));
+         if (!attr || !TREE_VALUE (attr))
+           attr = lookup_attribute ("io_low", DECL_ATTRIBUTES (decl));
 	  gcc_assert (attr);
 	}
       if (!attr || !TREE_VALUE (attr))
diff --git gcc/testsuite/gcc.target/avr/pr65210.c gcc/testsuite/gcc.target/avr/pr65210.c
new file mode 100644
index 0000000..1aed441
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr65210.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+
+/* This testcase exposes PR65210. Usage of the io_low attribute
+   causes assertion failure because code only looks for the io
+   attribute if SYMBOL_FLAG_IO is set. */
+
+volatile char q __attribute__((io_low,address(0x81)));

Re: [Patch, avr] Fix PR65210

[Denis Chertykov]

Committed.

2015-09-02 10:51 GMT+03:00 Senthil Kumar Selvaraj
<senthil_kumar.selvaraj@atmel.com>:
> Hi,
>
>   This (rather trivial) patch fixes PR65210. The ICE happens because code
>   wasn't handling io_low attribute where it is supposed to.
>
>   If this is ok, could someone commit please? I don't have commit
>   access.
>
> Regards
> Senthil
>
> gcc/ChangeLog
>
> 2015-09-02  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
>         PR target/65210
>         * config/avr/avr.c (avr_eval_addr_attrib): Look for io_low
>         attribute as well.
>
> gcc/testsuite/ChangeLog
>
>         PR target/65210
>         * gcc.target/avr/pr65210.c: New test.
>
> diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c
> index bec9a8b..9f5bc88 100644
> --- gcc/config/avr/avr.c
> +++ gcc/config/avr/avr.c
> @@ -9069,6 +9069,8 @@ avr_eval_addr_attrib (rtx x)
>        if (SYMBOL_REF_FLAGS (x) & SYMBOL_FLAG_IO)
>         {
>           attr = lookup_attribute ("io", DECL_ATTRIBUTES (decl));
> +         if (!attr || !TREE_VALUE (attr))
> +           attr = lookup_attribute ("io_low", DECL_ATTRIBUTES (decl));
>           gcc_assert (attr);
>         }
>        if (!attr || !TREE_VALUE (attr))
> diff --git gcc/testsuite/gcc.target/avr/pr65210.c gcc/testsuite/gcc.target/avr/pr65210.c
> new file mode 100644
> index 0000000..1aed441
> --- /dev/null
> +++ gcc/testsuite/gcc.target/avr/pr65210.c
> @@ -0,0 +1,7 @@
> +/* { dg-do compile } */
> +
> +/* This testcase exposes PR65210. Usage of the io_low attribute
> +   causes assertion failure because code only looks for the io
> +   attribute if SYMBOL_FLAG_IO is set. */
> +
> +volatile char q __attribute__((io_low,address(0x81)));

Re: [Patch, avr] Fix PR65210

[Georg-Johann Lay]

Senthil Kumar Selvaraj schrieb:
>   This (rather trivial) patch fixes PR65210. The ICE happens because code
>   wasn't handling io_low attribute where it is supposed to.

Hi Senthil, could you line out for what these new attributes are good 
for?  The Compiler just maps the argument to a compile-time const, so 
the attributes do the same as a cast to a volatile address.  Except that 
  the user must know in advance what I/O Region is the right one.

Supporting similar RELOCs for io* doesn't make hardly any sense, or are 
there Plans to add respective RELOCs (and ones for bitfields)?

Thanks, Johann

Re: [Patch, avr] Fix PR65210

[Senthil Kumar Selvaraj]

On Tue, Sep 08, 2015 at 09:16:35AM +0200, Georg-Johann Lay wrote:
> Senthil Kumar Selvaraj schrieb:
> >  This (rather trivial) patch fixes PR65210. The ICE happens because code
> >  wasn't handling io_low attribute where it is supposed to.
> 
> Hi Senthil, could you line out for what these new attributes are good for?
> The Compiler just maps the argument to a compile-time const, so the
> attributes do the same as a cast to a volatile address.  Except that  the
> user must know in advance what I/O Region is the right one.

IIRC, Joern Rennecke added these so you could have extern declarations
(without actual addresses) specifying that they will be in IO address
space, so that the compiler can generate IN/OUT or the bit addressable
instructions (SBI/CBI etc..).

For example,

extern char volatile SOMEPORT __attribute__((io_low));
int main()
{
  SOMEPORT |= 1;
}

$ ~/avr/install/bin/avr-gcc -mmcu=avr5 useio.c -Os -S
$ cat useio.s
<snip>
sbi SOMEPORT-32,0
ldi r24,0
ldi r25,0
ret
</snip>

whereas one without io_low will generate a lds, ori, sts sequence.
> 
> Supporting similar RELOCs for io* doesn't make hardly any sense, or are
> there Plans to add respective RELOCs (and ones for bitfields)?

binutils already has those - R_AVR_PORT6 and R_AVR_PORT5 do that. This was added
back in 2014 (https://sourceware.org/ml/binutils/2014-06/msg00122.html).