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

[Pedro Alves <palves at redhat dot com>]

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.

> +/* 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.


>    /* 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.

>      return avr_make_iaddr (addr << 1);
>    else
>      return avr_make_saddr (addr);
> @@ -1342,6 +1364,45 @@ avr_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>    return -1;
>  }
>  
> +/* Map DW_AT_address_class attributes to a type_instance_flag_value.  Note that
> +   this attribute is only valid with pointer types and therefore the flag is set
> +   to the pointer type and not its target type.  */
> +
> +static int
> +avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
> +{
> +  // __flash qualifier

Please use /**/-style comments, and write full sentences.

> +  if (dwarf2_addr_class == 1 && byte_size == 2)
> +    return AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
> +  return 0;
> +}
> +

Thanks,
-- 
Pedro Alves