From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 807 invoked by alias); 21 Jul 2014 15:45:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 782 invoked by uid 89); 21 Jul 2014 15:45:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 21 Jul 2014 15:45:09 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s6LFj5wn019673 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 21 Jul 2014 11:45:05 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s6LFj28p030504; Mon, 21 Jul 2014 11:45:04 -0400 Message-ID: <53CD357E.50708@redhat.com> Date: Mon, 21 Jul 2014 16:02:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Pierre Langlois , gdb-patches@sourceware.org Subject: Re: [PATCH] Add support for the __flash qualifier on AVR References: <1404816844-1639-1-git-send-email-pierre.langlois@embecosm.com> In-Reply-To: <1404816844-1639-1-git-send-email-pierre.langlois@embecosm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-07/txt/msg00542.txt.bz2 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