From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com [IPv6:2a00:1450:4864:20::12c]) by sourceware.org (Postfix) with ESMTPS id A16B2385C408 for ; Sat, 8 Jan 2022 16:26:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A16B2385C408 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-lf1-x12c.google.com with SMTP id g11so27481782lfu.2 for ; Sat, 08 Jan 2022 08:26:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=CWX6c2dlnYXjFnteKuW5+QbRMsty5HBTHbWBo+AaL2M=; b=Arvl97DEpUb66Af8k4wO9EwW5ON25KNSZdaOR2gl5zB7LMYa7QAitme+Sp2vC9eTCg F6Abc4tfSuuYTbiKLxe2ldxp999aSlcJKVsP+kByxbZDlMK9MXIPdnskVugvYEmlgPKa RWyxHYgObL6Cn/beJEuwz8G6gtuyaZURBi/BxZ95itS6E0WqsAuiRQqmdUqCc1yQmMRP iYRqmUtkSe4sYsXNM4Wkpy9bwu+gwya+3lvyfLWIRVEJfmaoYsZgdNVXUQOohX+IkRfn otOP4wNgAYuCe00D1R2A3T27krkWyxGI2Ha1qJS3Icr+3judMrllP4yDmWGdn6FZQWcw NQaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=CWX6c2dlnYXjFnteKuW5+QbRMsty5HBTHbWBo+AaL2M=; b=pcyXGLEgirdoqO4ScdZeXD8zTSMKl8e6gAvW93mt+3MmGv3IcyuuOShRyLz0yllgul 4vSq/3jfIchJevBoj/9xYHUgsDVSeAutpuK4173LNOSDeYSDC3LW4LLPT0imS2/73x7W WlPk1gqBrkIuIKmq5xW32egvzaqedCKX0UP6yPgWKfSbwagMNAIMWMOVLDcEsgSUydZR /dAPljgUGCm5JNQxeEI8h7xX6YGSvG1P/Mj6riWqUExEJYrCq55WZj+WAuruCUcYKBf1 snOxk2lgJ/khrywoHL41fYSm3BLT6Fn8f0uZ7RDiPNQdxHfF+vWFMPt1lh1Pruk3sGfL G3Tw== X-Gm-Message-State: AOAM533xEiDuBrprQZDRtufaqVAzAWl6UKxMJQQBJv+rk4Xy1ijuV+W9 qvgwUKA5XvBn+x712kc80yiPOQ== X-Google-Smtp-Source: ABdhPJw4eUOOVDoqMAhqRe6SIPuOrIyrwdVTKT2onUuQnqbsabJYuA0xrWH/sCTzQr4HjLfhzqpEnQ== X-Received: by 2002:a05:6512:3e28:: with SMTP id i40mr57657413lfv.436.1641659160103; Sat, 08 Jan 2022 08:26:00 -0800 (PST) Received: from [192.168.219.3] ([78.8.192.131]) by smtp.gmail.com with ESMTPSA id br31sm282509lfb.279.2022.01.08.08.25.58 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sat, 08 Jan 2022 08:25:59 -0800 (PST) Date: Sat, 8 Jan 2022 16:25:54 +0000 (GMT) From: "Maciej W. Rozycki" To: Andrew Burgess cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/6] Respect `set print repeats' with Fortran arrays In-Reply-To: <20211215151852.GK175541@redhat.com> Message-ID: References: <20211215151852.GK175541@redhat.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_ASCII_DIVIDERS, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Sat, 08 Jan 2022 16:26:04 -0000 On Wed, 15 Dec 2021, Andrew Burgess wrote: > > Index: src/gdb/f-array-walker.h > > =================================================================== > > --- src.orig/gdb/f-array-walker.h > > +++ src/gdb/f-array-walker.h > > @@ -131,6 +131,18 @@ struct fortran_array_walker_base_impl > > void finish_dimension (bool inner_p, bool last_p) > > { /* Nothing. */ } > > > > + /* Called when processing dimensions of the array other than the > > + innermost one. WALK_1 is the walker to normally call, ELT_TYPE is > > + the type of the element being extracted, and ELT_OFF is the offset > > + of the element from the start of array being walked, and LAST_P is > > + true only when this is the last element that will be processed in > > + this dimension. */ > > + void process_dimension (std::function walk_1, > > I think you should be using gdb::function_view here as the lambda only > needs to live for the lifetime of the function call. Changed; I wasn't aware of `gdb::function_view'. > > Index: src/gdb/f-valprint.c > > =================================================================== > > --- src.orig/gdb/f-valprint.c > > +++ src/gdb/f-valprint.c > > @@ -21,6 +21,7 @@ > > along with this program. If not, see . */ > > > > #include "defs.h" > > +#include "annotate.h" > > #include "symtab.h" > > #include "gdbtypes.h" > > #include "expression.h" > > @@ -96,6 +97,15 @@ f77_get_dynamic_length_of_aggregate (str > > * TYPE_LENGTH (check_typedef (TYPE_TARGET_TYPE (type))); > > } > > > > +/* Per-dimension statistics. */ > > + > > +struct dimension_stats > > +{ > > + /* Element counter. */ > > + int nelts; > > + bool elts_counted; > > +}; > > The comments here seem to just be repeating the variable names. Could > we give more information. e.g. "Element counter", what elements is it > counting, it's per dimension, but, is it counting every element? The > number of repeated elements in a series? What does the bool mean? > Does it mean we've counted everything? Just started counting? It counts the total number of elements in each dimension; obviously this needs to be done only once per dimension. Your discussion about `index_type' with 5/6 has actually inspired me to get rid of this active counter however and pass the total per-dimension element count as a parameter to `start_dimension' instead. I think it simplifies code enough to make it worth it. > I also wondered about whether we could replace this with, or otherwise > make use of a gdb::optional here? I see there's one place where we > set elts_counted without appearing to set nelts, I'm guessing we're > picking up the default value of 0 in that case? It's not clear to me what you mean with using `gdb::optional' here and we actually do always set `nelts' before flipping `elts_counted'. If you think of code in `process_dimension', then we have: if (!repeated || last_p) { /* ... */ if (!m_stats[dim_indx].elts_counted) m_stats[dim_indx].nelts += nrepeats * m_stats[dim_indx + 1].nelts; } /* ... */ if (last_p) m_stats[dim_indx].elts_counted = true; there, so `nelts' does get set as both conditionals execute if `last_p', which is when we're at the last element that concludes counting. It's now gone along with the active counter though. > > @@ -128,8 +141,18 @@ class fortran_array_printer_impl : publi > > bool continue_walking (bool should_continue) > > { > > bool cont = should_continue && (m_elts < m_options->print_max); > > + > > if (!cont && should_continue) > > - fputs_filtered ("...", m_stream); > > + { > > + if (m_nrepeats) > > The GDB style is to not treat integers as booleans, so this should be > written as: > > if (m_nrepeats > 0) I think the rule either changed or was only introduced at one point and I missed that. Fixed now, though I chose to transcribe it literally. > > @@ -149,22 +178,200 @@ class fortran_array_printer_impl : publi > > fputs_filtered (")", m_stream); > > if (!last_p) > > fputs_filtered (" ", m_stream); > > + > > + m_dimension--; > > + } > > + > > + /* Called when processing dimensions of the array other than the > > + innermost one. WALK_1 is the walker to normally call, ELT_TYPE is > > + the type of the element being extracted, and ELT_OFF is the offset > > + of the element from the start of array being walked, and LAST_P is > > + true only when this is the last element that will be processed in > > + this dimension. */ > > + void process_dimension (std::function walk_1, > > + struct type *elt_type, LONGEST elt_off, bool last_p) > > + { > > + size_t dim_indx = m_dimension - 1; > > + struct type *elt_type_prev = m_elt_type_prev; > > + LONGEST elt_off_prev = m_elt_off_prev; > > + bool repeated = (m_options->repeat_count_threshold < UINT_MAX > > + && elt_type_prev != nullptr > > + && (m_elts + ((m_nrepeats + 1) > > + * m_stats[dim_indx + 1].nelts) > > + <= m_options->print_max) > > + && dimension_contents_eq (m_val, elt_type, > > + elt_off_prev, elt_off)); > > + > > + if (repeated) > > + m_nrepeats++; > > + if (!repeated || last_p) > > + { > > + LONGEST nrepeats = m_nrepeats; > > + > > + m_nrepeats = 0; > > + if (nrepeats >= m_options->repeat_count_threshold) > > + { > > + annotate_elt_rep (nrepeats + 1); > > + fprintf_filtered (m_stream, "%p[%p]", > > + metadata_style.style ().ptr (), > > + plongest (nrepeats + 1), > > + nullptr); > > + annotate_elt_rep_end (); > > + if (!repeated) > > + fputs_filtered (" ", m_stream); > > + m_elts += nrepeats * m_stats[dim_indx + 1].nelts; > > + } > > + else > > + for (LONGEST i = nrepeats; i > 0; i--) > > + walk_1 (elt_type_prev, elt_off_prev, repeated && i == 1); > > + > > + if (!repeated) > > + { > > + /* We need to specially handle the case of hitting `print_max' > > + exactly as recursing would cause lone `(...)' to be printed. > > + And we need to print `...' by hand if the skipped element > > + would be the last one processed, because the subsequent call > > + to `continue_walking' from our caller won't do that. */ > > + if (m_elts < m_options->print_max) > > + { > > + walk_1 (elt_type, elt_off, last_p); > > + nrepeats++; > > + } > > + else if (last_p) > > + fputs_filtered ("...", m_stream); > > + } > > + > > + if (!m_stats[dim_indx].elts_counted) > > + m_stats[dim_indx].nelts += nrepeats * m_stats[dim_indx + 1].nelts; > > + } > > + > > + m_elt_type_prev = elt_type; > > + m_elt_off_prev = elt_off; > > + > > + if (last_p) > > + m_stats[dim_indx].elts_counted = true; > > } > > > > /* Called to process an element of ELT_TYPE at offset ELT_OFF from the > > start of the parent object. */ > > void process_element (struct type *elt_type, LONGEST elt_off, bool last_p) > > { > > - /* Extract the element value from the parent value. */ > > - struct value *e_val > > - = value_from_component (m_val, elt_type, elt_off); > > - common_val_print (e_val, m_stream, m_recurse, m_options, current_language); > > - if (!last_p) > > - fputs_filtered (", ", m_stream); > > + size_t dim_indx = m_dimension - 1; > > + struct type *elt_type_prev = m_elt_type_prev; > > + LONGEST elt_off_prev = m_elt_off_prev; > > + bool repeated = (m_options->repeat_count_threshold < UINT_MAX > > + && elt_type_prev != nullptr > > + && value_contents_eq (m_val, elt_off_prev, m_val, elt_off, > > + TYPE_LENGTH (elt_type))); > > + > > + if (repeated) > > + m_nrepeats++; > > + if (!repeated || last_p) > > + { > > + bool printed = false; > > I noticed that `printed` isn't actually needed, as we could say: > > bool printed = m_nrepeats; > > so you could just use m_nrepeats instead. Umm, no, because `process_outstanding_elements' clears `m_nrepeats' so we need to cache the state before the call. And then how we set `printed' it is a matter of style (I think it's clearer to the reader the way I chose as `printed = m_nrepeats' would really be a conditional in disguise; and it would have to be `printed = m_nrepeats != 0' to match our requirement not to use integers as booleans). Further inspired by the removal of the active element counter I have decided to remove the `process_outstanding_elements' as well. In case it wasn't obvious from code (or descriptions) it was only ever executed at the innermost dimension, having been factored out from an earlier version to avoid duplication. This, and especially the hairy handling of `m_index' needed to be added with 5/6, kept bothering me, so I chose to remove the call made from `continue_walking' (which the hairy handling was required for) and check for `print_max' explicitly in `process_element' similarly to how it's been handled in `process_dimension'. This way all the outstanding element processing needed where `print_max' is hit is done in a single place, simplifying code further. So the call to `process_outstanding_elements' has gone now, but I chose to keep the code mostly as is even though there's now a local `nrepeats' variable, because I still think the intent of this code should be clearer to the reader this way. There are enough corner cases already to make this piece complicated enough regardless. > > + > > + if (m_nrepeats) > > Missing '> 0' again. Fixed. > > + { > > + process_outstanding_elements (elt_type, elt_off_prev); > > + printed = true; > > + } > > + > > + if (!repeated) > > + { > > + /* Extract the element value from the parent value. */ > > + struct value *e_val > > + = value_from_component (m_val, elt_type, elt_off); > > + > > + if (printed) > > + fputs_filtered (", ", m_stream); > > + common_val_print (e_val, m_stream, m_recurse, m_options, > > + current_language); > > + } > > + if (!last_p) > > + fputs_filtered (", ", m_stream); > > + } > > + > > + m_elt_type_prev = elt_type; > > + m_elt_off_prev = elt_off; > > ++m_elts; > > + > > + if (last_p && !m_stats[dim_indx].elts_counted) > > + { > > + m_stats[dim_indx].nelts = m_elts; > > + m_stats[dim_indx].elts_counted = true; > > + } > > } > > > > private: > > + /* Called to print outstanding repeated elements of ELT_TYPE starting > > + at offset ELT_OFF from the start of the parent object. */ > > + void process_outstanding_elements (struct type *elt_type, LONGEST elt_off) > > + { > > + LONGEST nrepeats = m_nrepeats; > > + > > + m_nrepeats = 0; > > + if (nrepeats >= m_options->repeat_count_threshold) > > + { > > + annotate_elt_rep (nrepeats + 1); > > + fprintf_filtered (m_stream, "%p[%p]", > > + metadata_style.style ().ptr (), > > + plongest (nrepeats + 1), > > + nullptr); > > + annotate_elt_rep_end (); > > + } > > + else > > + { > > + /* Extract the element value from the parent value. */ > > + struct value *e_val = value_from_component (m_val, elt_type, elt_off); > > + > > + for (LONGEST i = nrepeats; i > 0; i--) > > + { > > + common_val_print (e_val, m_stream, m_recurse, m_options, > > + current_language); > > + if (i > 1) > > + fputs_filtered (", ", m_stream); > > + } > > + } > > + } > > + > > + /* Called to compare two VAL elements of ELT_TYPE at offsets OFFSET1 > > + and OFFSET2 each. Handle subarrays recursively, because they may > > + have been sliced and we do not want to compare any memory contents > > + present between the slices requested. */ > > + bool > > + dimension_contents_eq (const struct value *val, struct type *type, > > + LONGEST offset1, LONGEST offset2) > > + { > > + if (type->code () == TYPE_CODE_ARRAY > > + && TYPE_TARGET_TYPE (type)->code () != TYPE_CODE_CHAR) > > + { > > + /* Extract the range, and get lower and upper bounds. */ > > You have a space before tab here. Fixed, good catch! > > @@ -180,6 +387,19 @@ class fortran_array_printer_impl : publi > > /* The print control options. Gives us the maximum number of elements to > > print, and is passed through to each element that we print. */ > > const struct value_print_options *m_options = nullptr; > > + > > + /* Dimension tracker. */ > > + LONGEST m_dimension; > > + > > + /* Repetition tracker. */ > > + LONGEST m_nrepeats; > > + > > + /* Element tracker. */ > > + struct type *m_elt_type_prev; > > + LONGEST m_elt_off_prev; > > + > > + /* Per-dimension stats. */ > > + std::vector m_stats; > > I'd like to see some of these comments expanded slightly to give more > information about how they are used. Fixed (hopefully). Let me know if have any concerns about my explanations and/or the updates made. Maciej