From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90972 invoked by alias); 8 Sep 2018 23:52:54 -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 90960 invoked by uid 89); 8 Sep 2018 23:52:53 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=pushing, Other X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 08 Sep 2018 23:52:51 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id w88NqiIQ023011 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sat, 8 Sep 2018 19:52:49 -0400 Received: by simark.ca (Postfix, from userid 112) id AAFDB1E75F; Sat, 8 Sep 2018 19:52:44 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 6346A1E16B; Sat, 8 Sep 2018 19:52:43 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sat, 08 Sep 2018 23:52:00 -0000 From: Simon Marchi To: Andrew Burgess Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCHv4] gdb: Add default frame methods to gdbarch In-Reply-To: <20180908231921.GJ22193@embecosm.com> References: <20180907212619.27618-1-andrew.burgess@embecosm.com> <77e448ed-d394-88b3-c73a-ab87ec2f8c38@ericsson.com> <20180908231921.GJ22193@embecosm.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2018-09/txt/msg00217.txt.bz2 On 2018-09-09 00:19, Andrew Burgess wrote: >> > + >> > + release_value (value); >> >> I'm not too familiar with the lifetime of struct value objects, but >> why is this >> needed? > > For why, I'm not really sure. > > If we follow the code through then any call to frame_unwind_register, > frame_register_unwind, or frame_unwind_register_unsigned will do the > same release_value. This patch never set out to change the behaviour > of existing code, just to remove duplication, so on those grounds > alone, in this patch at least I'd want the call to stay. > > There is one clue in frame_register_unwind, where we have this code: > > /* Dispose of the new value. This prevents watchpoints from > trying to watch the saved frame pointer. */ > release_value (value); Ok, that's interesting. My hypothesis is that when GDB needs to watch an expression (a + b + c), it probably looks at the all_values chain to know all the intermediary values (that represent a memory location or a register) that were read from the target to evaluate that expression. It then puts some hardware or software watches on these memory locations or registers. So if the values chain contains a value representing something we don't really watch to watch (such as fp or sp, according to the comment), GDB would watch it, even though it doesn't really affect the result of the expression. That's just my guess. > Pretty cryptic, I'm not really sure how we could end up watching that > value, so I don't really understand that. (This was added in commit > 669fac235d5e about 10 years ago). > > My understanding of value lifetime is that generally values live in a > single all_values list at various points we'll call value_set_mark, > run some code that might generate some values, and then call > value_delete_to_mark to delete all values as we know they are no > longer needed. > > Values can be moved off the global all_values list, for example into > the value history, which means they will not be deleted by the call to > value_delete_to_mark. That's what I understand too. > Other than, calling this release_value here will help keep the > all_values list shorter (and the cryptic comment above) I don't see > why we couldn't just leave the $pc value on the all_values list, and > clean it up at the next call to value_delete_to_mark.... > > ... however, like I said, I'd prefer this patch be just a refactor, > unless the code is obviously wrong I'd rather keep the call in. I am fine with that (unless someone who really knows this comes up with a better explanation). Since it just causes the value we don't need anymore to be deleted immediatly, it should be harmless. > All your other comments are addressed in the version below. This version LGTM. Can you wait 2-3 days before pushing to see if somebody has something to add? Simon