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

[Jason Merrill <jason@redhat.com>]

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";

?

>> Currently, I implement the same restrictions as the C front-end, but I
>> think that this restriction could be lifted.
> 
> Hi Paul,
> 
> this would be great.  FYI, due to AVR quirks, .rodata is located in RAM.
> Reason behind this is that in functions like
> 
> char get_letter (const char *c)
> {
>      return *c;
> }
> 
> there is no means to determine whether get_letter was called with a 
> const char* or a char*.  Accessing flash vs. RAM would require different 
> instructions, thus .rodata is part of RAM, so that RAM accesses will 
> work in either case.
> 
> The obvious problem is that this wastes RAM. One way out is to define 
> address space in flash and to pass const __flash char*, where respective 
> objects are located in flash (.progmem.data in case of avr).
> 
> This is fine for objects which the application creates, but there are 
> also artificial objects like vtables or cswtch tables.
> 
>>> 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.)
>