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

[Keven Boell <keven dot boell at linux dot intel dot com>]

On 09.02.2015 08:09, Joel Brobecker wrote:
> 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?

Basically the whole support for Fortran dynamic arrays is covered in this patch, not only the support allocated/associated states.

> 
> 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.

I thought it would be good to have one patch, which enables the dynamic array support completely and not having individual small patches, which might not functional at all. All the tests following this patch are testing various functionality in the area of dynamic arrays.
I will think about it if it's possible to split them.

> 
> 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.

I see, sounds reasonable to me.

> 
> 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.
> 

Yes, this assumption is done as this is the main idea behind the implementation.
Will add add an assert.

Thanks,
Keven