From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x335.google.com (mail-wm1-x335.google.com [IPv6:2a00:1450:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id 0F97B3985425 for ; Thu, 29 Oct 2020 11:08:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0F97B3985425 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x335.google.com with SMTP id c16so2020891wmd.2 for ; Thu, 29 Oct 2020 04:08:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=mzd5SYXqtTfQq7CJ0TOudQ1ulZTaCvzSeqmexoFVojM=; b=LJ+OPM2CjOAK3d9FQ6IH3Rw6n3HI9gbj5uHJiJOyE8BBKJg48/9bID2YVcMOfkQe+0 07piVFlTTsrP15iV+LScrhtrZKZU0LvnJHJNkE49JdsCL3q7FptwZZamwXTqkwYGOXeS PHQXrfm6EyStYpRwF6UXvmJNouxqDfVP4KmTTt/rzj7U+vQ+0eFifkxhj5Q2B+YH16dd yhMBC8cYrV/SbTkHdw37feiXhGMojGwNySUErYXJ+biDhJrvf5dl+rhsQ7/36LEgwp8L /3qqayaUFGvpkOiq7qaZzl49pzuGjRtVJs50ATUFlG2/opkUnfyr7bUx4ghv3z8SET+7 utqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=mzd5SYXqtTfQq7CJ0TOudQ1ulZTaCvzSeqmexoFVojM=; b=NnTaqw+/aLKv/eh2wITg3ksrhr+wrvOMFWq1YmbRBT+EnwPt3fWqzKBmc2/l5k8zAx G/rAcDRRGexIXBEq4DBvpv9WO64cY9pPoPN0q0scsSqJ/FM/OxnLIKaGjrWElnlmuG20 pVSUlpslzSH7Mh2oCkvbMhSn5eZiE63PkWAZwQ0TT8SshZft2UQGmPh8WNXKugF4Kos5 nmrvaNbUBACJgRmAEES1BvpMZy9PKdhgFLyr4dFEkQr5N3BZztU9M+vaaVz01yJgiIK9 x+Q23pUjJqq/KkDA/HVz0oqqOz1A3z05oWCfpwKCfMgH2DyNh6UYtVSp/BfZQTqxEV4x f4aQ== X-Gm-Message-State: AOAM530FijHP9uOP5HBIccv8yHY6i2WDuAknXLprkCGEcHfyeXKWAxSu 1p8ngozHqEQXUWt5ujYzoHq+4UEnSSOPYA== X-Google-Smtp-Source: ABdhPJxGNNHcOIOCWgvA5ghl4Y4jj3Ybet96BaicR+4jHxQ234mXCZgSfgTk0471EOJHgZ8RHXq1ww== X-Received: by 2002:a1c:7409:: with SMTP id p9mr3589012wmc.167.1603969737103; Thu, 29 Oct 2020 04:08:57 -0700 (PDT) Received: from localhost (host109-154-163-49.range109-154.btcentralplus.com. [109.154.163.49]) by smtp.gmail.com with ESMTPSA id 26sm3727335wmk.42.2020.10.29.04.08.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Oct 2020 04:08:56 -0700 (PDT) Date: Thu, 29 Oct 2020 11:08:55 +0000 From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCHv5 4/4] gdb/fortran: Add support for Fortran array slices at the GDB prompt Message-ID: <20201029110855.GB2729@embecosm.com> References: <7832c05de858cfc8bf4b6abba4332523d0547805.1602439661.git.andrew.burgess@embecosm.com> <87pn5ciq2q.fsf@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87pn5ciq2q.fsf@tromey.com> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 10:52:51 up 4 days, 1:56, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, URIBL_CSS, URIBL_CSS_A autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Oct 2020 11:08:59 -0000 * Tom Tromey [2020-10-20 14:45:49 -0600]: > >>>>> "Andrew" == Andrew Burgess writes: > > Andrew> The problem is that, as I see it, the current value contents model > Andrew> assumes that an object base address will be the lowest address within > Andrew> that object, and that the contents of the object start at this base > Andrew> address and occupy the TYPE_LENGTH bytes after that. > > Andrew> ( We do have the embedded_offset, which is used for C++ sub-classes, > Andrew> such that an object can start at some offset from the content buffer, > Andrew> however, the assumption that the object then occupies the next > Andrew> TYPE_LENGTH bytes is still true within GDB. ) > > Relatedly, we had a bug for a while where the Python val-print code > could be given just a virtual base of an object, and it would then try > to examine memory outside this buffer. > > I've also considered separating values from memory in some ways. > > Here's something weird in gdb... debug this program: > > #include > char b[] = "hello"; > struct x { > char *x; > }; > int main() > { > struct x val; val.x = b; > printf("%s\n", val.x); > b[1] = 'q'; > printf("%s\n", val.x); > return 0; > } > > If you stop at the first printf and "print val" you get: > > (gdb) p val > $1 = { > x = 0x40200c "hello" > } > > Then at the second you can see: > > (gdb) p val > $2 = { > x = 0x40200c "hqllo" > } > (gdb) p $1 > $3 = { > x = 0x40200c "hqllo" > } > > That is, the apparent value of the string in "$1" changed. This happens > because the value only holds the pointer, the contents are read during > printing. > > So, sometimes I've wondered if we want to fix that, by say attaching > more memory to the value, say as it is printed. I would like that too I think. I've wondered whether a model where the value provides some kind of data cache like behaviour would work. But we'd attach and detach different value caches at different times. So when we read the value of V1 we "attach" V1 as the current caching value we'd then cache all memory that ends up being read within the value - assuming of course that the content is not already cached. After we've finished reading V1 and displayed it (or whatever) V1 would be "detached". Later when we want to reread V1 we again "attach" V1, only this time the content will already be cached on the value... > > Another thing I've considered is maybe letting multiple values share > some memory (to avoid duplicating large arrays); or maybe going the > other way and lazily populating arrays when they are used purely as > intermediate values. This, please, yes! > > Kind of random thoughts, though I suppose the lazy array thing is > similar to something you've done in this patch. > > Andrew> Where this hack will show through to the user is if they ask for the > Andrew> address of an array in their program with a negative array stride, the > Andrew> address they get from GDB will not match the address that would be > Andrew> computed within the Fortran program. > > Calls for a second hack ;) Yes. Or to maybe fix the value base address, content address issue properly, which is what I'd like to do. Getting this in first is good (I think) as (1) it adds some useful functionality to GDB, even if it does have at least one known rough edge, and (2) it provides more things to test a real fix for the base address / content address issue against. > > FWIW I don't think I really have a problem with your proposed hack. Thanks. > > Andrew> + /* Create a new offset calculator for TYPE, which is either an array or a > Andrew> + string. */ > Andrew> + fortran_array_offset_calculator (struct type *type) > > Single-argument constructors should normally be explicit. Fixed. > > Andrew> +/* A base class used by fortran_array_walker. */ > Andrew> +class fortran_array_walker_base_impl > Andrew> +{ > Andrew> +public: > > A class with only public methods (and no data) can just be a "struct". Fixed. > > Andrew> + /* Constructor. */ > Andrew> + explicit fortran_array_walker_base_impl () > Andrew> + { /* Nothing. */ } > > Doesn't need the constructor. Fixed. > > Andrew> +/* A class to wrap up the process of iterating over a multi-dimensional > Andrew> + Fortran array. IMPL is used to specialise what happens as we walk over > Andrew> + the array. See class FORTRAN_ARRAY_WALKER_BASE_IMPL (above) for the > Andrew> + methods than can be used to customise the array walk. */ > Andrew> +template > Andrew> +class fortran_array_walker > > This seems to mix compile-time- and runtime- polymorphism. > > Maybe the idea was not to have virtual methods? But in that case this: Unless I've mucked up I don't think there _are_ any virtual methods. > > Andrew> + /* Ensure that Impl is derived from the required base class. This just > Andrew> + ensures that all of the required API methods are available and have a > Andrew> + sensible default implementation. */ > Andrew> + gdb_static_assert ((std::is_base_of::value)); > > ... seems weird. I guess the idea is to use method hiding as a kind of > static overriding? But if any method is missing, compilation will just > fail anyway. Your guess is exactly correct. I could delete this line, this would have the benefit that, like you say if any methods are missing things will just fail at compile time, and I could use _any_ class, not just sub-classes of fortran_array_walker_base_impl. The benefit of keeping the line (that I was aiming for) is it I didn't imagine any sensible use of this class where we didn't specialise using a sub-class of fortran_array_walker_base_impl. The static assert has zero runtime cost, and, should a developer try using a non-sub-class, they'll be told explicitly sub-class this thing. Alternatively I can rewrite the comment to make things clearer. Do you think there's a rewritten comment that you'd be happy with? I'm really not that attached to this one line. If you really don't like it, it's gone! Let me know your thoughts. Thanks, Andrew