Re: [PATCHv5 4/4] gdb/fortran: Add support for Fortran array slices at the GDB prompt

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Tom Tromey <tom@tromey.com> [2020-10-20 14:45:49 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> The problem is that, as I see it, the current value contents model
> Andrew> assumes that an object base address will be the lowest address within
> Andrew> that object, and that the contents of the object start at this base
> Andrew> address and occupy the TYPE_LENGTH bytes after that.
> 
> Andrew> ( We do have the embedded_offset, which is used for C++ sub-classes,
> Andrew> such that an object can start at some offset from the content buffer,
> Andrew> however, the assumption that the object then occupies the next
> Andrew> TYPE_LENGTH bytes is still true within GDB. )
> 
> Relatedly, we had a bug for a while where the Python val-print code
> could be given just a virtual base of an object, and it would then try
> to examine memory outside this buffer.
> 
> I've also considered separating values from memory in some ways.
> 
> Here's something weird in gdb... debug this program:
> 
>     #include <stdio.h>
>     char b[] = "hello";
>     struct x {
>       char *x;
>     };
>     int main()
>     {
>       struct x val; val.x = b;
>       printf("%s\n", val.x);
>       b[1] = 'q';
>       printf("%s\n", val.x);
>       return 0;
>     }
> 
> If you stop at the first printf and "print val" you get:
> 
>     (gdb) p val
>     $1 = {
>       x = 0x40200c <b> "hello"
>     }
> 
> Then at the second you can see:
> 
>     (gdb) p val
>     $2 = {
>       x = 0x40200c <b> "hqllo"
>     }
>     (gdb) p $1
>     $3 = {
>       x = 0x40200c <b> "hqllo"
>     }
> 
> That is, the apparent value of the string in "$1" changed.  This happens
> because the value only holds the pointer, the contents are read during
> printing.
> 
> So, sometimes I've wondered if we want to fix that, by say attaching
> more memory to the value, say as it is printed.

I would like that too I think.

I've wondered whether a model where the value provides some kind of
data cache like behaviour would work.  But we'd attach and detach
different value caches at different times.

So when we read the value of V1 we "attach" V1 as the current caching
value we'd then cache all memory that ends up being read within the
value - assuming of course that the content is not already cached.

After we've finished reading V1 and displayed it (or whatever) V1
would be "detached".

Later when we want to reread V1 we again "attach" V1, only this time
the content will already be cached on the value...

> 
> Another thing I've considered is maybe letting multiple values share
> some memory (to avoid duplicating large arrays); or maybe going the
> other way and lazily populating arrays when they are used purely as
> intermediate values.

This, please, yes!

> 
> Kind of random thoughts, though I suppose the lazy array thing is
> similar to something you've done in this patch.
> 
> Andrew> Where this hack will show through to the user is if they ask for the
> Andrew> address of an array in their program with a negative array stride, the
> Andrew> address they get from GDB will not match the address that would be
> Andrew> computed within the Fortran program.
> 
> Calls for a second hack ;)

Yes.  Or to maybe fix the value base address, content address issue
properly, which is what I'd like to do.

Getting this in first is good (I think) as (1) it adds some useful
functionality to GDB, even if it does have at least one known rough
edge, and (2) it provides more things to test a real fix for the base
address / content address issue against.

> 
> FWIW I don't think I really have a problem with your proposed hack.

Thanks.

> 
> Andrew> +  /* Create a new offset calculator for TYPE, which is either an array or a
> Andrew> +     string.  */
> Andrew> +  fortran_array_offset_calculator (struct type *type)
> 
> Single-argument constructors should normally be explicit.

Fixed.

> 
> Andrew> +/* A base class used by fortran_array_walker.  */
> Andrew> +class fortran_array_walker_base_impl
> Andrew> +{
> Andrew> +public:
> 
> A class with only public methods (and no data) can just be a "struct".

Fixed.

> 
> Andrew> +  /* Constructor.  */
> Andrew> +  explicit fortran_array_walker_base_impl ()
> Andrew> +  { /* Nothing.  */ }
> 
> Doesn't need the constructor.

Fixed.

> 
> Andrew> +/* A class to wrap up the process of iterating over a multi-dimensional
> Andrew> +   Fortran array.  IMPL is used to specialise what happens as we walk over
> Andrew> +   the array.  See class FORTRAN_ARRAY_WALKER_BASE_IMPL (above) for the
> Andrew> +   methods than can be used to customise the array walk.  */
> Andrew> +template<typename Impl>
> Andrew> +class fortran_array_walker
> 
> This seems to mix compile-time- and runtime- polymorphism.
> 
> Maybe the idea was not to have virtual methods?  But in that case this:

Unless I've mucked up I don't think there _are_ any virtual methods.

> 
> Andrew> +  /* Ensure that Impl is derived from the required base class.  This just
> Andrew> +     ensures that all of the required API methods are available and have a
> Andrew> +     sensible default implementation.  */
> Andrew> +  gdb_static_assert ((std::is_base_of<fortran_array_walker_base_impl,Impl>::value));
> 
> ... seems weird.  I guess the idea is to use method hiding as a kind of
> static overriding?  But if any method is missing, compilation will just
> fail anyway.

Your guess is exactly correct.  I could delete this line, this would
have the benefit that, like you say if any methods are missing things
will just fail at compile time, and I could use _any_ class, not just
sub-classes of fortran_array_walker_base_impl.

The benefit of keeping the line (that I was aiming for) is it I didn't
imagine any sensible use of this class where we didn't specialise
using a sub-class of fortran_array_walker_base_impl.  The static
assert has zero runtime cost, and, should a developer try using a
non-sub-class, they'll be told explicitly sub-class this thing.

Alternatively I can rewrite the comment to make things clearer.  Do
you think there's a rewritten comment that you'd be happy with?  I'm
really not that attached to this one line.  If you really don't like
it, it's gone!  Let me know your thoughts.

Thanks,
Andrew