[review v4] gdb/fortran: array stride support

["Simon Marchi (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/627
......................................................................


Patch Set 4:

(3 comments)

I can't assess the correctness of the patch, as I don't really know the domain, but I noted a few points.

| --- gdb/dwarf2read.c
| +++ gdb/dwarf2read.c
| @@ -18053,8 +18053,18 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
|    if (low.kind == PROP_CONST
|        && !TYPE_UNSIGNED (base_type) && (low.data.const_val & negative_mask))
|      low.data.const_val |= negative_mask;
|    if (high.kind == PROP_CONST
|        && !TYPE_UNSIGNED (base_type) && (high.data.const_val & negative_mask))
|      high.data.const_val |= negative_mask;
|  
| -  range_type = create_range_type (NULL, orig_base_type, &low, &high, bias);
| +  /* Check for bit and byte strides.  */
| +  struct attribute *attr_bit_stride, *attr_byte_stride;

PS4, Line 18061:

You might as well declare them on the line where you assign them.

| +  struct dynamic_prop bit_stride_prop, byte_stride_prop;
| +  attr_byte_stride = dwarf2_attr (die, DW_AT_byte_stride, cu);
| +  if (attr_byte_stride != nullptr)
| +    {
| +      struct type *prop_type
| +	= dwarf2_per_cu_addr_sized_int_type (cu->per_cu, false);
| +      attr_to_dynamic_prop (attr_byte_stride, die, cu, &byte_stride_prop,
| +			    prop_type);
| +    }
| --- gdb/gdbtypes.c
| +++ gdb/gdbtypes.c
| @@ -942,9 +946,21 @@ create_range_type (struct type *result_type, struct type *index_type,
|       less than the lower bound, so checking the lower bound is not
|       enough.  Make sure we do not mark a range type whose upper bound
|       is negative as unsigned.  */
|    if (high_bound->kind == PROP_CONST && high_bound->data.const_val < 0)
|      TYPE_UNSIGNED (result_type) = 0;
|  
|    return result_type;
|  }
|  
| +/* Like CREATE_RANGE_TYPE but also sets up a stride.  When BYTE_STRIDE_P
| +   is true the value in STRIDE is a byte stride, otherwise STRIDE is a bit
| +   stride.  */

PS4, Line 957:

This comment should be

 /* See gdbtypes.h.  */

I know that other functions around don't adhere to this rule, but I
think for a new function, or when modifying an existing function
comment, we should do it right.

| +
| +struct type *
| +create_range_type_with_stride (struct type *result_type,
| +			       struct type *index_type,
| +			       const struct dynamic_prop *low_bound,
| +			       const struct dynamic_prop *high_bound,
| +			       LONGEST bias,
| +			       const struct dynamic_prop *stride,
| +			       bool byte_stride_p)

 ...

| @@ -2017,0 +2061,19 @@ resolve_dynamic_range (struct type *dyn_range_type,
| +    {
| +      stride.kind = PROP_CONST;
| +      stride.data.const_val = value;
| +
| +      /* If we have a bit stride that is not a multiple of the byte stride
| +	 then I really don't think this is going to work with current GDB.
| +	 The array indexing code in GDB seems to be pretty heavily tied to
| +	 byte offsets right now.  If this comes up then we warn the user
| +	 and set up a known incorrect stride.  */
| +      if (!byte_stride_p && (value % HOST_CHAR_BIT) != 0)

PS4, Line 2070:

Using HOST_CHAR_BIT (a few occurences in the patch) when manipulating
target data is probably not right.  You probably want to use
gdbarch_addressable_memory_unit_size, using the gdbarch from the type.

| +	error (_("bit strides that are not a multiple of the byte size "
| +		 "are currently not supported"));
| +    }
| +  else
| +    {
| +      stride.kind = PROP_UNDEFINED;
| +      stride.data.const_val = 0;
| +      byte_stride_p = true;
| +    }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I9af2bcd1f2d4c56f76f5f3f9f89d8f06bef10d9a
Gerrit-Change-Number: 627
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 22 Nov 2019 13:06:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment