Re: [V4 02/18] vla: make dynamic fortran arrays functional.

[Joel Brobecker <brobecker@adacore.com>]

On Wed, Jan 14, 2015 at 02:49:34PM +0100, Keven Boell wrote:
> This patch enables GDB to print the value of a dynamic
> array (VLA) if allocated/associated in fortran. If not the
> allocation status will be printed to the command line.
> 
> (gdb) p vla_not_allocated
> $1 = <not allocated>
> 
> (gdb) p vla_allocated
> $1 = (1, 2, 3)
> 
> (gdb) p vla_not_associated
> $1 = <not associated>
> 
> (gdb) p vla_associated
> $1 = (3, 2, 1)
> 
> The patch covers various locations where the allocation/
> association status makes sense to print.

There may be places where it easily makes sense, but there are
several hunks that I do not understand. I suspect that those
bits may have something to do with the 10 testcases that follow
this patch.

First of all, it's unclear to me which pieces of functionality
this patch is trying to address: It's clear that handling of
non-associated & non-allocated types is one. But are there
other pieces of functionality being added here as well?

What I propose is that we first clarify the question above, and
that once the question above is clarified, we work on the bare
minimum patch to provide the first piece of functionality on
the list, whichever one makes most sence to you. That patch
should provide not just this bare-minimum set of code changes,
but also the corresponding tests that verify the new piece of
functionality.That way, I can review code and test together,
and see why certain hunks are necessary.

Limitations that allow us to reduce the size of each patch are OK.
We can then have small patches at each iteration that lifts one
limitation at a time.

During this iterative process, I would stop and wait at each
iterative process. I think you'll be wasting a fair amout of
time at each iteration if you try to submit 20 patches each time.

Some general comments below:

> 2014-05-28  Keven Boell  <keven.boell@intel.com>
>             Sanimir Agovic  <sanimir.agovic@intel.com>
> 
> 	* dwarf2loc.c (dwarf2_address_data_valid): New
> 	function.
> 	* dwarf2loc.h (dwarf2_address_data_valid): New
> 	function.
> 	* f-typeprint.c (f_print_type): Print allocation/
> 	association status.
> 	(f_type_print_varspec_suffix): Print allocation/
> 	association status for &-operator usages.
> 	* gdbtypes.c (create_array_type_with_stride): Add
> 	query for valid data location.
> 	(is_dynamic_type): Extend dynamic type detection
> 	with allocated/associated. Add type detection for
> 	fields.
> 	(resolve_dynamic_range): Copy type before resolving
> 	it as dynamic attributes need to be preserved.
> 	(resolve_dynamic_array): Copy type before resolving
> 	it as dynamic attributes need to be preserved. Add
> 	resolving of allocated/associated attributes.
> 	(resolve_dynamic_type): Add call to nested
> 	type resolving.
> 	(copy_type_recursive): Add allocated/associated
> 	attributes to be copied.
> 	(copy_type): Copy allocated/associated/data_location
> 	as well as the fields structure if available.
> 	* valarith.c (value_subscripted_rvalue): Print allocated/
> 	associated status when indexing a VLA.
> 	* valprint.c (valprint_check_validity): Print allocated/
> 	associated status.
> 	(val_print_not_allocated): New function.
> 	(val_print_not_associated): New function.
> 	* valprint.h (val_print_not_allocated): New function.
> 	(val_print_not_associated): New function.
> 	* value.c (set_value_component_location): Adjust the value
> 	address for single value prints.
>  	    do_cleanups (value_chain);
> +
> +	    /* Select right frame to correctly evaluate VLA's during a backtrace.  */
> +	    if (is_dynamic_type (type))
> +	      select_frame (frame);

The comment has a line that's too long. There are several other
parts of this patch where lines are too long too.

> +
>  	    retval = value_at_lazy (type, address + byte_offset);
>  	    if (in_stack_memory)
>  	      set_value_stack (retval, 1);
> @@ -2546,6 +2551,17 @@ dwarf2_compile_property_to_c (struct ui_file *stream,
>  			     data, data + size, per_cu);
>  }
>  
> +int
> +dwarf2_address_data_valid (const struct type *type)

When the function documentation is in the .h file, we prefer
that comment explains where the function is documented. Eg:

/* See dwarf2loc.h.  */

This way, we do know at a single glance that the function is
documented, and where that documentation is.

> +{
> +  if (TYPE_NOT_ASSOCIATED (type))
> +    return 0;
> +
> +  if (TYPE_NOT_ALLOCATED (type))
> +    return 0;

Based on the implementation of those macros, you are definitely
making an assumption that for types that do have these properties,
those properties have been resolved. This needs to be documented,
and I am also wondering whether an assert might also be appropriate.

-- 
Joel