[PATCH 3/6] Respect `set print repeats' with Fortran arrays

[Maciej W. Rozycki]

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<void (struct type *, int, bool)> 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 <http://www.gnu.org/licenses/>.  */
> >  
> >  #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<void (struct type *, int, bool)> 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[<repeats %s times>%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[<repeats %s times>%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<struct dimension_stats> 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