Re: [PATCH] Add support for the __flash qualifier on AVR

[Pierre Langlois <pierre dot langlois at embecosm dot com>]

Hi Pedro,

On 21/07/14 16:45, Pedro Alves wrote:
> Hi Pierre,
> 
> On 07/08/2014 11:54 AM, Pierre Langlois wrote:
>> The __flash qualifier is part of the named address spaces for AVR [1]. It allows
>> putting read-only data in the flash memory, normally reserved for code.
>>
>> When used together with a pointer, the DW_AT_address_class attribute is set to 1
>> and allows GDB to detect that when it will be dereferenced, the data will be
>> loaded from the flash memory (with the LPM instruction).
>>
> 
> Thanks.  This looks good to me, with a couple nits pointed out
> below addressed.
> 

Thanks for looking at this.  I have committed a better version of this patch
last week after review.  So I just submitted another patch improving the
comments as suggested.

>> +/* Address space flags */
>> +
>> +/* We are assigning the TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1 to the flash address
>> +   space.  */
>>    /* Is it a code address?  */
>> -  if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>> -      || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
>> +  else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>> +	   || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
>>      {
>> +      /* A code address, either a function pointer or the program counter, is
>> +	 word (16 bits) addressed.  */
> 
> FYI, the "or the program counter" reference here looks a bit
> confusion-inducing to me.
> 
>   TYPE_CODE_FUNC -> function.
>   TYPE_CODE_METHOD -> C++ class member functions (methods).
> 
> I think you've added that, because the $pc register is of function
> pointer type?  I'd suggest just not bringing that up, as it was before.
> 

Yes you're right, I mentioned the $pc register because of its type.  I
understand it's confusing now.

> 
>>    /* Is it a code address?  */
>> -  if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>> -      || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
>> -      || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
>> +  else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>> +	   || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
>> +	   || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
> 
> Particularly since this bit didn't get that comment?
>
> BTW, interesting/curious that TYPE_CODE_SPACE is handled
> here but not in avr_address_to_pointer.  Offhand, looks like a
> bug, though off topic for this patch.
> 

I am bit puzzled here but I suspect this was done on purpose.  I'll submit a
patch so we can discuss it.

Thanks,

Pierre