Re: [PATCH v3] c++: parser - Support for target address spaces in C++

[Georg-Johann Lay <avr@gjlay.de>]

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.)
>>
>