From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24544 invoked by alias); 23 Jul 2014 16:52:22 -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 24475 invoked by uid 89); 23 Jul 2014 16:52:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 X-HELO: mail-wi0-f172.google.com Received: from mail-wi0-f172.google.com (HELO mail-wi0-f172.google.com) (209.85.212.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 23 Jul 2014 16:52:21 +0000 Received: by mail-wi0-f172.google.com with SMTP id n3so8167313wiv.17 for ; Wed, 23 Jul 2014 09:52:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=EDiu2putP3ZgEmB4KazGv7/5ZGl4Kfk0B+6e8eYgxYM=; b=iXVPq4OtevAyCxEX+Bg9zGBda6yzy19e7aEv9qTAzwYvSxKEe3TQpUTkzBKtoAcw6+ h057mljCFLDarNRrRz9Dk8TutnZEp8m8AeZ961Ndsh1N3u+7No67b+SqsG4iEz1EvURV 7eWyAVa+3QDSIjnV0O4ONxLuwpxx5PhAEwu8ZA7ZqObsgHi/F3TqAA7CmWrgd9k+fqN6 Idcx6/3lcTUplRNuA6lxNcYlqHLHebS0aumSRUZWUJb165PPGp6hPAAihDHCC8mZCAFO pdqoHYOH3SXm0JF2pYIF3XaT7K0In70mmIUcYYW1bfuG/lkUyAJmxYfJqRlB5smUqEhd fezw== X-Gm-Message-State: ALoCoQnQvmoKOUJypMo81pAlqBDw+Qt3ZQGyj9Bdco19wKToXD6kbYYlK+ZpAW9IipYBjcG26xMp X-Received: by 10.180.218.12 with SMTP id pc12mr27027912wic.15.1406134336659; Wed, 23 Jul 2014 09:52:16 -0700 (PDT) Received: from [192.168.0.134] (cust64-dsl91-135-5.idnet.net. [91.135.5.64]) by mx.google.com with ESMTPSA id ev18sm3235793wid.1.2014.07.23.09.52.15 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 23 Jul 2014 09:52:15 -0700 (PDT) Message-ID: <53CFE83F.7030002@embecosm.com> Date: Wed, 23 Jul 2014 17:42:00 -0000 From: Pierre Langlois User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Pedro Alves , 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> <53CD357E.50708@redhat.com> In-Reply-To: <53CD357E.50708@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg00581.txt.bz2 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