[vla v7 pushed] Re: [PATCH v6 00/15] Please have a final look

[Joel Brobecker <brobecker@adacore.com>]

> Keith, Joel, all, sorry for causing any inconvenience and thanks to
> you both for taking care of the issue on Friday.

No worries - it can happen to anyone.

> The root cause was this code fragment below. It was in my repo and I
> git-amend it accidently as I was playing around with an alternative to
> solve the pointer-to-vla issue.
> 
> @@ -2985,7 +2984,6 @@ evaluate_subexp_with_coercion (struct expression *exp,
>         {
>           (*pos) += 4;
>           val = address_of_variable (var, exp->elts[pc + 1].block);
> -         type = value_type (val);
>           return value_cast (lookup_pointer_type (TYPE_TARGET_TYPE (type)),
>                              val);

OK. That indeed solved the regressions outside of gdb.ada, so thanks
a lot for that. It saved me a ton of time not having to investigate
this issue.

This did not solve, on the other hand, the regressions with gdb.ada,
which turned out to be caused by a discrepancy between is_dynamic_type
which considers references to dynamic arrays as being dynamic, and
resolve_dynamic_bounds, which was handling the case of typedefs OK,
but otherwise expected the type to be an array type via the following
assert (which allowed us to find the bug, btw, so well placed one!):

  gdb_assert (TYPE_CODE (type) == TYPE_CODE_ARRAY);

So, when you have code like the following (resolve_dynamic_type)...

  if (!is_dynamic_type (real_type))
    return type;

  resolved_type = resolve_dynamic_bounds (type, addr);

... it is possible for a reference to dynamic array to get through
the check and then be passed to resolve_dynamic_bounds.

I ammended your patch as follow, essentially making resolve_dynamic_bounds
handle reference types as well:

| --- a/gdb/gdbtypes.c
| +++ b/gdb/gdbtypes.c
| @@ -40,6 +40,7 @@
|  #include "cp-support.h"
|  #include "bcache.h"
|  #include "dwarf2loc.h"
| +#include "gdbcore.h"
|  
|  /* Initialize BADNESS constants.  */
|  
| @@ -1665,6 +1666,16 @@ resolve_dynamic_bounds (struct type *type, CORE_ADDR addr)
|        return copy;
|      }
|  
| +  if (TYPE_CODE (type) == TYPE_CODE_REF)
| +    {
| +      struct type *copy = copy_type (type);
| +      CORE_ADDR target_addr = read_memory_typed_address (addr, type);
| +
| +      TYPE_TARGET_TYPE (copy)
| +	= resolve_dynamic_bounds (TYPE_TARGET_TYPE (type), target_addr);
| +      return copy;
| +    }
| +
|    gdb_assert (TYPE_CODE (type) == TYPE_CODE_ARRAY);
|  
|    elt_type = type;

I re-tested everything after making those changes, and everything
seems to be clean, now, so I pushed your patch series. In the process,
I had to fix a number of little nits, mostly related to the ChangeLog
file and the revision logs. From memory:

  - ChangeLog: The entries, when still part of the commit, were
    not located at the start of the ChangeLog, and the date still
    refered to the 11th. It's easier, IMO, to manipulate commits
    without ChangeLog entries, and to add them at the very last
    minute, when just about to push to the official repo. But
    some tips on how to management have been also shared on the gdb@
    mailing-list as well as on various blogs.

  - Some testsuite/ChangeLog entries were incorrect, missing
    the name of the gdb.[...] subdirectory in the file name,
    of mentioning it as the path to the (nonexistant) ChangeLog
    file.

  - The revision logs had traces of "Conflict:" sections, which
    are automatically added when you do a cherry-pick, I think,
    which results in a conflict that you resolve before commit.
    I removed those.

I think that's about it. Let's hope that we're in the clear now! :)

For the record, patch 01 and 02...

        [PATCH 01/12] type: add c99 variable length array support
        [PATCH 02/12] vla: enable sizeof operator to work with variable

... when individually tested on x86_64-linux, and then I only ran
the testsuite after applying the remaining commits. But I did test
each one of them in sequence last Friday, so I didn't see a reason
to do it again.

> 
> In addition I refactored the code for handling the sizeof operator to
> match Keith latest fix for PR c++/16675 (245a5f0) in:
> 
>   #02 vla: enable sizeof operator to work with variable length arrays
> 
> I used Jans diffgdb script to compare two runs (pre-/post vla) of 'make
> check' and now the patch series is free of regression as it should be.
> 
> You will find the latest patch series at:
> 
>   https://github.com/intel-gdb/vla/tree/vla-c99