Bug 56263 - [avr] Provide strict address-space checking
Summary: [avr] Provide strict address-space checking
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.8.0
: P5 enhancement
Target Milestone: 4.8.0
Assignee: Georg-Johann Lay
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2013-02-09 10:30 UTC by Georg-Johann Lay
Modified: 2013-04-20 05:10 UTC (History)
0 users

See Also:
Host:
Target: avr
Build:
Known to work:
Known to fail:
Last reconfirmed: 2013-02-09 00:00:00


Attachments
Test case that shall error with strict address spaces (132 bytes, text/plain)
2013-02-09 10:30 UTC, Georg-Johann Lay
Details
draft patch (1.02 KB, patch)
2013-02-11 15:09 UTC, Georg-Johann Lay
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Georg-Johann Lay 2013-02-09 10:30:25 UTC
Created attachment 29401 [details]
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.
Comment 1 demiurg_spb 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
Comment 2 Georg-Johann Lay 2013-02-11 15:09:40 UTC
Created attachment 29418 [details]
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.
Comment 3 demiurg_spb 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 ...
Comment 4 Georg-Johann Lay 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";
}
Comment 5 demiurg_spb 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.
Comment 6 Georg-Johann Lay 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
Comment 7 Georg-Johann Lay 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;
}
Comment 8 demiurg_spb 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
}
Comment 9 Jakub Jelinek 2013-03-22 14:42:59 UTC
GCC 4.8.0 is being released, adjusting target milestone.
Comment 10 Georg-Johann Lay 2013-04-20 05:10:59 UTC
Done, see -Waddr-space-convert.