On Fri, 2009-10-30 at 10:47 +0100, Jan Kratochvil wrote:
> On Wed, 28 Oct 2009 18:34:46 +0100, Joost van der Sluis wrote:

> > * tekhex.c (move_section_contents): fixed usage of offset parameter
> 
> > --- a/bfd/tekhex.c
> > +++ b/bfd/tekhex.c
> > @@ -583,8 +583,7 @@ move_section_contents (bfd *abfd,
> >    bfd_vma prev_number = 1;	/* Nothing can have this as a high bit.  */
> >    struct data_struct *d = NULL;
> >  
> > -  BFD_ASSERT (offset == 0);
> > -  for (addr = section->vma; count != 0; count--, addr++)
> > +  for (addr = section->vma + offset; count != 0; count--, addr++)
> >      {
> >        /* Get high bits of address.  */
> >        bfd_vma chunk_number = addr & ~(bfd_vma) CHUNK_MASK;
> 
> (a) You use tekhex binary format for some builds wrt Pascal?
> (b) OFFSET is already asserted as 0, this change must be a nop.

This code caused some regressions, unrelated to pascal. There are some
tests that write large arrays to disk and then reads them again.

Point is that this function has a parameter 'offset', but I think that
someone saw that it was never implemented, and thus added the assert to
check that it was always 0. But with my patch, this was not the case, so
I implemented the offset for other values then 0.

I still think that this is a valid fix, but I saw that I can omit this
change without a regression anymore. So that's no problem.

> > 2009 Jan Kratochvil <jan.kratochvil@redhat.com>>
> > * ada-valprint.c (print_optional_low_bound): no idea
> 
> > diff --git a/gdb/ada-valprint.c b/gdb/ada-valprint.c
> > index 565172c..af5def1 100644
> > --- a/gdb/ada-valprint.c
> > +++ b/gdb/ada-valprint.c
> > @@ -90,7 +90,8 @@ print_optional_low_bound (struct ui_file *stream, struct type *type,
> >    if (options->print_array_indexes)
> >      return 0;
> >  
> > -  if (!get_array_bounds (type, &low_bound, &high_bound))
> > +gdb_assert (0);        /* type vs. val */
> > +  if (!get_array_bounds (NULL, &low_bound, &high_bound))
> >      return 0;
> >  
> >    /* If this is an empty array, then don't print the lower bound.
> 
> This patch I wrote to be able to build GDB first the first variant of the
> patch as you built it without -Wall -Werror and thus the code was invalid.
> 
> As you changed the get_array_bounds prototype to require now `struct value *'
> and not `struct type *' as befoer the ADA code was no longer compatible.
> 
> I did not find simple enough to fix it, there is no `struct value *' available
> at that specific point of ADA code.  Either (a) one should pass
> `struct value *' there somehow or (b) one should still provide
> get_array_bounds variant accepting just `struct type *' for such cases.
> Whether (b) is a feasible way or whether one should go the (a) way I found to
> be more decided by the author of the patch - you.

Attached is a completely other way. This change from 'struct type' to
'struct value' was not strictly needed anymore. So I removed it. 

> One needs to care of all the targets and supported languages in the GNU
> projects as even the generic code benefits from the code contributed by the
> people around such specific targets/languages as ADA.

Or Pascal. I do understand this very well.. ;)

> > * cp-valprint.c (cp_print_value_fields): when the address is 0, do not pass
> > the 0 value increased with some offset to val_print, but pass 0 instead
> 
> > --- a/gdb/cp-valprint.c
> > +++ b/gdb/cp-valprint.c
> > -		  val_print (TYPE_FIELD_TYPE (type, i),
> > -			     valaddr, offset + TYPE_FIELD_BITPOS (type, i) / 8,
> > -			     address + TYPE_FIELD_BITPOS (type, i) / 8,
> > -			     stream, recurse + 1, &opts,
> > -			     current_language);
> > +                  if (address != 0)
> > +		    val_print (TYPE_FIELD_TYPE (type, i),
> > +		               valaddr, offset + TYPE_FIELD_BITPOS (type, i) / 8,
> > +			       address + TYPE_FIELD_BITPOS (type, i) / 8,
> > +			       stream, recurse + 1, &opts,
> > +			       current_language);
> > +                  else
> > +		    val_print (TYPE_FIELD_TYPE (type, i),
> > +		               valaddr, offset + TYPE_FIELD_BITPOS (type, i) / 8,
> > +			       0,
> > +			       stream, recurse + 1, &opts,
> > +			       current_language);
> 
> ADDRESS zero is a valid address of a variable on some targets, it must not be
> treated as a specific "undefined" case.  GDB for example uses for this purpose
> value ~0 (INVALID_ENTRY_POINT) which is also not right but less probable to be
> hit in real world cases.

> (Still this part is OK for archer-jankratochvil-vla but not for FSF GDB.)

Do you have a suggestion how to fix this? I did this to fix one single
regression:

FAIL: gdb.base/call-rt-st.exp: print print_one_large_struct(*list1)

The problem is that cp_print_value_fields is called with an invalid
address value of 0. And not ~0. That's not really my fault... Maybe I
can fix this one call from the test, but I thought that is was better to
fix this for the more general case.

> > @@ -240,7 +243,6 @@ static struct value_history_chunk *value_history_chain;
> >  
> >  static int value_history_count;	/* Abs number of last entry stored */
> >  
> > -
> >  /* List of all value objects currently allocated
> >     (except for those released by calls to release_value)
> >     This is so they can be freed after each command.  */
> 
> Please drop such excessive patch changes.

A remaining of the few ours I tried to use Eclipse. And I don't know how
to get rid of it using git. But I can delete it from the patch. ;)

Joost