From: Georg-Johann Lay <avr@gjlay.de>
To: Jason Merrill <jason@redhat.com>,
Paul Iannetta <piannetta@kalrayinc.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH v3] c++: parser - Support for target address spaces in C++
Date: Tue, 15 Nov 2022 13:15:04 +0100 [thread overview]
Message-ID: <30df2a2b-c895-1ae6-100a-ad1450a5ba22@gjlay.de> (raw)
In-Reply-To: <2184e167-795f-126b-b535-fe3130441e08@redhat.com>
Am 14.11.22 um 18:55 schrieb Jason Merrill:
> On 11/10/22 06:40, Georg-Johann Lay wrote:
>>
>>
>> Am 10.11.22 um 15:08 schrieb Paul Iannetta:
>>> On Thu, Nov 03, 2022 at 02:38:39PM +0100, Georg-Johann Lay wrote:
>>>>> [PATCH v3] c++: parser - Support for target address spaces in C++
>>>> 2. Will it work with compound literals?
>>>> =======================================
>>>>
>>>> Currently, the following C code works for target avr:
>>>>
>>>> const __flash char *pHallo = (const __flash char[]) { "Hallo" };
>>>>
>>>> This is a pointer in RAM (AS0) that holds the address of a string in
>>>> flash
>>>> (AS1) and is initialized with that address. Unfortunately, this does
>>>> not
>>>> work locally:
>>>>
>>>> const __flash char* get_hallo (void)
>>>> {
>>>> [static] const __flash char *p2 = (const __flash char[]) {
>>>> "Hallo2" };
>>>> return p2;
>>>> }
>>>>
>>>> foo.c: In function 'get_hallo':
>>>> foo.c: error: compound literal qualified by address-space qualifier
>>>>
>>>> Is there any way to make this work now? Would be great!
>
> I don't object to allowing this, but what's the advantage of this
> pattern over
>
> static __flash const char p2[] = "Hallo2";
>
> ?
Hi Jason.
Take an example that's a bit more complicated, like:
#define FSTR(X) (const __flash char[]) { X }
const __flash char* strings[] =
{
FSTR("cat"),
FSTR("dog"),
FSTR("petrophaga lorioti")
};
This won't work in a function just because GCC rejects it, no matter
whether strings[] itself is in __flash or not. One work around would be to
const __flash char str_cat[] = "cat";
const __flash char str_dog[] = "dog";
const __flash char str_petrophaga_lorioti[] = "petrophaga_lorioti";
const __flash char* strings[] =
{
str_cat,
str_dog,
str_petrophaga_lorioti
};
but anyone would prefer the first alternative.
In a more broader context, code without ASes like
const char* strings[] =
{
"cat",
"dog",
"petrophaga lorioti"
};
that makes perfect sense in C/C++ should also be possible for
address-spaces, no matter whether the pointers and/or strings[] is in
non-generic AS.
Unfortunately, ISO/IEC TR 18037 seems to never have considered such
code, and they mostly are concerned with how code is being accessed, but
not how code is being located.
Johann
>>> Currently, I implement the same restrictions as the C front-end, but I
>>> think that this restriction could be lifted.
>>>> 3. Will TARGET_ADDR_SPACE_DIAGNOSE_USAGE still work?
>>>> ====================================================
>>>>
>>>> Currently there is target hook TARGET_ADDR_SPACE_DIAGNOSE_USAGE.
>>>> I did not see it in your patches, so maybe I just missed it? See
>>>> https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gccint/Named-Address-Spaces.html#index-TARGET_005fADDR_005fSPACE_005fDIAGNOSE_005fUSAGE
>>>
>>> That was a point I overlooked in my previous patch. This will be in
>>> my new revision where I also add implicit conversion between
>>> address spaces and also expose TARGET_ADDR_SPACE_CONVERT.
>>>
>>>> 4. Will it be possible to put C++ virtual tables in ASs, and how?
>>>> =================================================================
>>>
>>> Currently, I do not allow the declaration of instances of classes in
>>> an address space, mainly to not have to cope with the handling of the
>>> this pointer. That is,
>>>
>>> __flash Myclass *t;
>>>
>>> does not work. Nevertheless, I admit that this is would be nice to
>>> have.
>>>
>>>> One big complaint about avr-g++ is that there is no way to put
>>>> vtables in
>>>> flash (address-space 1) and to access them accordingly. How can
>>>> this be
>>>> achieved with C++ address spaces?
>>>
>>> Do you want only the vtables in the flash address space or do you want
>>> to be able to have the whole class content.
>>
>> My question is about vtables, not the bits that represent some object.
>> vtables are stored independently of objects, usually in .rodata +
>> comdat. Notice that vtables are read-only and in static storage, even
>> if objects are neither.
>>
>> The problem with vtables is that the user has no handle to specify
>> where to locate them -- and even if, due to AVR quirks, the right
>> instruction must be used. Thus just putting vtables in flash by means
>> of some section attribute won't work, only address-spaces can do the
>> trick.
>>
>>> 1. If you only want the vtables, I think that a target hook called
>>> at vtable creation would do the trick.
>>
>> Yes, that would be enough, see https://gcc.gnu.org/PR43745
>
> As you say there, this would be an ABI change, so there would need to be
> a transition strategy. I don't know to what extent AVR users try to use
> older compiled code vs. always rebuilding everything.
>
>> Johann
>>
>>> 2. If you want to be able to have pointer to classes in __flash, I
>>> will need to further the support I have currently implemented to
>>> support the pointer this qualified with an address space.
>>> Retrospectively, I think this have to be implemented.
>>>
>>> Paul
>>
>> Would be great if this would work, but I think this can be really
>> tricky, because it's already tricky for non-class objects.
>
> Indeed, especially if objects of the same type can live either in flash
> or RAM: you'd need 2 or more of each method for the different accesses.
> Perhaps via cloning.
>
> Simpler might be to declare that objects of a particular class can only
> live in flash.
>
>> A user has to specify __flash explicitly, which is quite different to
>> plain objects. For example, a const int can live in .rodata, but in
>> cases like
>>
>> extern int func();
>> extern const int ival;
>> const int ival = func();
>>
>> ival would live in .bss and be initialized at runtime by a static
>> constructor. Consequently,
>>
>> const __flash int ival = func();
>>
>> is invalid code that has to be diagnosed [1], because in the avr case,
>> __flash means non-volatile memory, which contradicts initialization at
>> runtime.
>>
>> So only objects that are TREE_READONLY can go into AS __flash,
>> TREE_CONST is not enough.
>>
>> How is this problem addressed? Does this require a new target hook to
>> diagnose such cases, and does the compiler know at that stage that an
>> object will be TREE_READONLY?
>
> That does sound like a job for a new target hook, if there isn't a
> suitable one already.
>
>> [1] Notice that in C it's enough to check that __flash is always
>> accompanied by const, but in C++ this is not more enough as shown in
>> the example above.
>>
>> Johann
>>
>>
>> p.s: I just checked clang v16, it doesn't complain and accepts
>> nonsensical code like:
>>
>> extern int func();
>>
>> extern const __flash int cval;
>> const __flash int cval = func();
>>
>> int get_cval()
>> {
>> return cval;
>> }
>>
>> (clang puts cval into .bss and initializes at startup, but get_cval
>> will read from flash due to __flash.)
>>
>
prev parent reply other threads:[~2022-11-15 12:15 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-06 14:34 [RFC] " Paul Iannetta
2022-10-06 17:34 ` Jason Merrill
2022-10-09 16:12 ` [PATCH] " Paul Iannetta
2022-10-10 19:20 ` Jason Merrill
2022-10-11 22:03 ` [PATCH v2] " Paul Iannetta
2022-10-12 1:49 ` Jason Merrill
2022-10-13 0:52 ` Paul Iannetta
2022-10-13 5:46 ` Jakub Jelinek
2022-10-13 15:14 ` Paul Iannetta
2022-10-13 15:02 ` Jason Merrill
2022-10-13 15:23 ` Paul Iannetta
2022-10-13 15:47 ` Jason Merrill
2022-10-13 16:02 ` Paul Iannetta
2022-10-13 19:41 ` Jason Merrill
2022-10-13 21:57 ` Paul Iannetta
2022-10-14 15:19 ` Jason Merrill
2022-10-18 7:37 ` [PATCH v3] " Paul Iannetta
2022-10-18 14:24 ` Jason Merrill
2022-10-18 17:01 ` Paul Iannetta
2022-10-19 18:55 ` Jason Merrill
2022-10-26 7:18 ` Paul Iannetta
2022-10-26 16:28 ` Jason Merrill
2022-11-10 15:42 ` [PATCH v4] " Paul Iannetta
2022-11-03 13:38 ` [PATCH v3] " Georg-Johann Lay
2022-11-10 14:08 ` Paul Iannetta
2022-11-10 16:40 ` Georg-Johann Lay
2022-11-14 17:55 ` Jason Merrill
2022-11-15 12:15 ` Georg-Johann Lay [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=30df2a2b-c895-1ae6-100a-ad1450a5ba22@gjlay.de \
--to=avr@gjlay.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
--cc=piannetta@kalrayinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).