[Bug target/56263] New: [avr] Provide strict address-space checking

[Bug target/56263] New: [avr] Provide strict address-space checking

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

             Bug #: 56263
           Summary: [avr] Provide strict address-space checking
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: target
        AssignedTo: gjl@gcc.gnu.org
        ReportedBy: gjl@gcc.gnu.org
                CC: demiurg_spb@freemail.ru
            Target: avr


Created attachment 29401
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29401
Test case that shall error with strict address spaces

The intrinsic address spaces introduced with PR49868 are imlemented in such a
way that each address space is a subset of each other.

This allows code like follows to operate as expected and without warnings:

char read_char (const char *address, int data_in_flash)
{
    if (data_in_flash)
        return *(const __flash char*) address;
    else
        return *address;
}

Currently, targetm.addr_space_subset_p returns always true in order to allow
pointer casts like above without diagnostics.

avr.c:avr_addr_space_subset_p() could be implemented in such a way, that it
returns true iff the respective ASes are physical subsets of each other, and
not only if their address, regarded as number, are subsets.

In order not to change the current ABI, this can be achieved by a new command
line option like -maddr-space-subset that allows the user to pick the model of
his favor.

[Bug target/56263] [avr] Provide strict address-space checking

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P5
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2013-02-09
   Target Milestone|---                         |4.8.0
     Ever Confirmed|0                           |1

[Bug target/56263] [avr] Provide strict address-space checking

[demiurg_spb at freemail dot ru]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

--- Comment #1 from demiurg_spb at freemail dot ru 2013-02-10 12:24:49 UTC ---
I think the next test case should also be considered.

const char __flash* const __flash names[] =
{
    "flash_str1",
    "flash_str2"
};

const char __flash* name1 = names[0]; // ok
const char*         name2 = names[1]; // error

[Bug target/56263] [avr] Provide strict address-space checking

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

--- Comment #2 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2013-02-11 15:09:40 UTC ---
Created attachment 29418
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29418
draft patch

    PR target/56263
    * config/avr/avr.opt (-mstrict-addr-space-subsets): New option and...
    (avr_strict_addr_space_subsets)... attached variable.
    * config/avr/avr.c (avr_addr_space_subset_p): Use it to determine
    whether of not an address spaces are subsets.
    * doc/invoke.texi (AVR Options) <-mstrict-addr-space-subsets>:
    Document it.


I had a look at this.

With strict address spaces, GCC will emit zeroes as result of casts across
address spaces.  This means that code like

char read_char (const char *address, int data_in_flash)
{
    if (data_in_flash)
        return *(const __flash char*) address;
    else
        return *address;
}

will no more operate correctly.  For the same reason, it is no more possible to
use PSTR from AVR-LibC with functions that get an address space pointer because
PSTR puts (and must put) the literal in generic space.

(In reply to comment #1)
> I think the next test case should also be considered.
> 
> const char __flash* const __flash names[] =
> {
>     "flash_str1",
>     "flash_str2"

This cannot work because ISO/IEC TR18037 forces these literals into generic
space.

> };
> 
> const char __flash* name1 = names[0]; // ok
> const char*         name2 = names[1]; // error

Attached is the draft patch that I used.  This means that in order to make this
work, the compiler proper has to be extended and the feature cannot be
implemented in the avr backend alone.  Thus suspending for now and for an other
stage.

[Bug target/56263] [avr] Provide strict address-space checking

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |SUSPENDED

[Bug target/56263] [avr] Provide strict address-space checking

[demiurg_spb at freemail dot ru]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

--- Comment #3 from demiurg_spb at freemail dot ru 2013-02-12 06:47:43 UTC ---
(In reply to comment #2)

> This cannot work because ISO/IEC TR18037 forces these literals into generic
> space.
> 

ISO/IEC TR18037
5.1.2 Address-space type qualifiers:

If the type of an object is qualified by an address space name, the
object is allocated in the specified address space; otherwise, the object is
allocated in the generic address space.


I just re-read the standard.
I could not find any reason not permitted to place the data in __flash address
space in that case:

const char __flash* const __flash names[] = {"flash_str1", "flash_str2"};

IAR compilers it does, and that hinders gcc do the same?
I think it's an easy misunderstanding, preventing common sense ...

[Bug target/56263] [avr] Provide strict address-space checking

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

--- Comment #4 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2013-02-19 17:58:36 UTC ---
(In reply to comment #3)
> (In reply to comment #2)
> 
>> This cannot work because ISO/IEC TR18037 forces these literals into generic
>> space.
>> 
> 
> ISO/IEC TR18037
> 5.1.2 Address-space type qualifiers:
> 
> If the type of an object is qualified by an address space name, the
> object is allocated in the specified address space; otherwise, the object is
> allocated in the generic address space.
> 
> I just re-read the standard.
> I could not find any reason not permitted to place the data in __flash address
> space in that case:
> 
> const char __flash* const __flash names[] = {"flash_str1", "flash_str2"};

The initializer at the righ side has type "const char *const[]" thus names[] is
located in flash because names[] is qualified __flash.  However, the Embedded C
does not say anything about the literals like "flash_str1" of type "const
char*".  Therefore, vanilla C applies which says that these literals go into
generic space.

> IAR compilers it does, and that hinders gcc do the same?
> I think it's an easy misunderstanding, preventing common sense ...

It's a shortcoming in the Embedded C paper and I agree with you that more
elaborate Embedded C paper would be more convenient here.

There are two ways out of this:

1) Extend the Embedded C paper and propose an addendum to the ISO WG14.

2) Implement this extension no matter whether Embedded C comes with this
extension.  Find someone who implements this extension, supports it and makes
sure there are no conflicts with the vanilla Embedded C.

Notice that with the extension, in the following example "init" would be
located in flash but "assign" would still be located in RAM.

void f_init (void)
{
    const __flash char *str = "init";
}

void f_assign (void)
{
    const __flash char *str;
    str = "assign";
}

[Bug target/56263] [avr] Provide strict address-space checking

[demiurg_spb at freemail dot ru]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

--- Comment #5 from demiurg_spb at freemail dot ru 2013-02-20 05:53:53 UTC ---
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > 
> 
> It's a shortcoming in the Embedded C paper and I agree with you that more
> elaborate Embedded C paper would be more convenient here.
> 
> There are two ways out of this:
> 
> 1) Extend the Embedded C paper and propose an addendum to the ISO WG14.
> 
> 2) Implement this extension no matter whether Embedded C comes with this
> extension.  Find someone who implements this extension, supports it and makes
> sure there are no conflicts with the vanilla Embedded C.
> 
> Notice that with the extension, in the following example "init" would be
> located in flash but "assign" would still be located in RAM.
> 
> void f_init (void)
> {
>     const __flash char *str = "init";
> }
> 
> void f_assign (void)
> {
>     const __flash char *str;
>     str = "assign";
> }

In my view, in this situation, the data must be placed in a flash ...
Standard really needs serious improvement.
It's logical, when the right-hand and left-hand side of each other have the
appropriate type. Moreover, for the implementation of this simple idea is not
objective difficulties.

[Bug target/56263] [avr] Provide strict address-space checking

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

--- Comment #6 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2013-03-12 11:42:30 UTC ---
Author: gjl
Date: Tue Mar 12 11:42:26 2013
New Revision: 196611

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=196611
Log:
    PR target/56263
    * config/avr/avr.c (TARGET_CONVERT_TO_TYPE): Define to...
    (avr_convert_to_type): ...this new static function.
    * config/avr/avr.opt (-Waddr-space-convert): New C option.
    * doc/invoke.texi (AVR Options): Document it.


Modified:
    trunk/gcc/config/avr/avr.c
    trunk/gcc/config/avr/avr.opt
    trunk/gcc/doc/invoke.texi

[Bug target/56263] [avr] Provide strict address-space checking

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|SUSPENDED                   |ASSIGNED

--- Comment #7 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2013-03-12 21:21:19 UTC ---
This patch implements -Waddr-space-convert and will print diagnostics for casts
to non-containing address spaces.

It's just a quick implementation in order to get the patch into 4.8.0 which
will be frozen for release withing the next few days.

Some work still to be done:

- Try to avoid warnings for casts from PSTR ("text") to const __flash char*.
  PSTR is a commonly used idion from AVR-LibC's avr/progmem.h, namely

  #define PSTR(s)                                                    \
   (__extension__(                                                   \
      {                                                              \ 
         static const char __c[] __attribute__((__progmem__)) = (s); \
         &__c[0];                                                    \
      }))

- Try to distinguish between implicit casts and explicit casts requested
  by the user

- Allow to pick a warning level for the previous kinds of casts


(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > 
> > It's a shortcoming in the Embedded C paper and I agree with you that more
> > elaborate Embedded C paper would be more convenient here.
> > 
> > There are two ways out of this:
> > 
> > 1) Extend the Embedded C paper and propose an addendum to the ISO WG14.
> > 
> > 2) Implement this extension no matter whether Embedded C comes with this
> > extension.  Find someone who implements this extension, supports it and
> > makes sure there are no conflicts with the vanilla Embedded C.
> > 
> > Notice that with the extension, in the following example "init" would be
> > located in flash but "assign" would still be located in RAM.
> > 
> > void f_init (void)
> > {
> >     const __flash char *str = "init";
> > }
> > 
> > void f_assign (void)
> > {
> >     const __flash char *str;
> >     str = "assign";
> > }
> 
> In my view, in this situation, the data must be placed in a flash ...
> Standard really needs serious improvement.

ACK.  May be that is the reason for why Embedded-C did not go into C11.

However, waiting until the Embedded-C paper will be extended in that direction
is pointless.  Just try to participate the ISO process; it will take years...

Maybe it's doable in the avr backend, but then we need a proper specification
and enough knowledge to decide whether or not all hooks are guaranteeing that
the BE can take the decision in every case.

> It's logical, when the right-hand and left-hand side of each other have the
> appropriate type. Moreover, for the implementation of this simple idea is not
> objective difficulties.

Sorry? I don't understand you last remark.  Are you saying it is trivial to
implement in the avr backend?

Before implementing it, you'll have to specify it.  What should do this code?

const __flash char* f (int i)
{
    const __flash char *p = "Hallo" + i;
    return p;
}

[Bug target/56263] [avr] Provide strict address-space checking

[demiurg_spb at freemail dot ru]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

--- Comment #8 from demiurg_spb at freemail dot ru 2013-03-13 06:46:40 UTC ---
(In reply to comment #7)
> Sorry? I don't understand you last remark.  Are you saying it is trivial to
> implement in the avr backend?
>
No. Implementation is hard work.
I mean that if we take (typeof(lhs)==typeof(rhs)) axiom in initialization and
assignment: we have no logical problem at all.

> Before implementing it, you'll have to specify it.  What should do this code?
> 
> const __flash char* f (int i)
> {
>     const __flash char *p = "Hallo" + i;
>     return p;
> }

Yes it's not trivial... But it should be equal to next cases:

const __flash char* f (int i)
{
    const __flash char *p = "Hallo"; // flash str
    return &p[i];
}

const __flash char* f (int i)
{
    return "Hallo" + i;  // flash str
}

const __flash char* f (int i)
{
    return &"Hallo"[i];  // flash str
}

[Bug target/56263] [avr] Provide strict address-space checking

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.0                       |4.8.1

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-03-22 14:42:59 UTC ---
GCC 4.8.0 is being released, adjusting target milestone.

[Bug target/56263] [avr] Provide strict address-space checking

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56263

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|4.8.1                       |4.8.0

--- Comment #10 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2013-04-20 05:10:59 UTC ---
Done, see -Waddr-space-convert.