[Bug target/56263] [avr] Provide strict address-space checking
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.
[Bug target/56263] [avr] Provide strict address-space checking
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
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
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=gccview=revrev=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
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
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
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
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
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
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
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
