GDB 6.8.92 available for testing

GDB 6.8.92 available for testing

[Joel Brobecker]

Hello,

I have just finished creating the gdb-6.8.92 pre-release.
It is available for download at the following location:

    ftp://sourceware.org/pub/gdb/snapshots/branch/gdb-6.8.92.tar.bz2

A gzip'ed version is also available: gdb-6.8.92.tar.gz.

Please give it a test if you can and report any problems you might find.
We're trying to flush out any remaining issue by Monday, in order to
possibly make the 7.0 release on Tuesday.

On behalf of all the GDB contributors, thank you!
-- 
Joel

Re: GDB 6.8.92 available for testing

[Ralf Corsepius]

On 09/30/2009 10:48 PM, Joel Brobecker wrote:
> Hello,
>
> I have just finished creating the gdb-6.8.92 pre-release.
> It is available for download at the following location:
>
>      ftp://sourceware.org/pub/gdb/snapshots/branch/gdb-6.8.92.tar.bz2
>
> A gzip'ed version is also available: gdb-6.8.92.tar.gz.
>
> Please give it a test if you can and report any problems you might find.
> We're trying to flush out any remaining issue by Monday, in order to
> possibly make the 7.0 release on Tuesday.
>
> On behalf of all the GDB contributors, thank you!

None of the patches I had submitted to gdb-patches had been applied.

--enable-system-readline also is still broken (Jan, please submit your 
gdb-readline-6.0.patch you have applied to Fedora's gdb).

Ralf

Re: GDB 6.8.92 available for testing

[Andreas Schwab]

Ralf Corsepius <ralf.corsepius@rtems.org> writes:

> --enable-system-readline also is still broken (Jan, please submit your
> gdb-readline-6.0.patch you have applied to Fedora's gdb).

Here is the patch I've been using for the openSUSE package.  I have
renamed the uses of readline_echoing_p to _rl_echoing_p to emphasize its
internal nature (future readline versions may even stop exporting it).

Andreas.

2009-10-01  Andreas Schwab  <schwab@redhat.com>

	* tui/tui-io.c (tui_rl_display_match_list): Use _rl_echoing_p
	instead of readline_echoing_p for version 6 or later of
	libreadline.

--- gdb/tui/tui-io.c.orig	2009-03-10 11:34:01.000000000 +0100
+++ gdb/tui/tui-io.c	2009-03-10 11:34:45.000000000 +0100
@@ -512,8 +512,13 @@ tui_rl_display_match_list (char **matche
 void
 tui_setup_io (int mode)
 {
+#if RL_VERSION_MAJOR >= 6
+  extern int _rl_echoing_p;
+#else
   extern int readline_echoing_p;
- 
+#define _rl_echoing_p readline_echoing_p
+#endif
+
   if (mode)
     {
       /* Redirect readline to TUI.  */
@@ -522,12 +527,12 @@ tui_setup_io (int mode)
       tui_old_rl_prep_terminal = rl_prep_term_function;
       tui_old_rl_getc_function = rl_getc_function;
       tui_old_rl_outstream = rl_outstream;
-      tui_old_readline_echoing_p = readline_echoing_p;
+      tui_old_readline_echoing_p = _rl_echoing_p;
       rl_redisplay_function = tui_redisplay_readline;
       rl_deprep_term_function = tui_deprep_terminal;
       rl_prep_term_function = tui_prep_terminal;
       rl_getc_function = tui_getc;
-      readline_echoing_p = 0;
+      _rl_echoing_p = 0;
       rl_outstream = tui_rl_outstream;
       rl_prompt = 0;
       rl_completion_display_matches_hook = tui_rl_display_match_list;
@@ -564,7 +569,7 @@ tui_setup_io (int mode)
       rl_getc_function = tui_old_rl_getc_function;
       rl_outstream = tui_old_rl_outstream;
       rl_completion_display_matches_hook = 0;
-      readline_echoing_p = tui_old_readline_echoing_p;
+      _rl_echoing_p = tui_old_readline_echoing_p;
       rl_already_prompted = 0;
 
       /* Save tty for SIGCONT.  */

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

RE: GDB 6.8.92 available for testing

[Pierre Muller]

Andreas,

why don't you simply use this?
#if RL_VERSION_MAJOR >= 6
  extern int _rl_echoing_p;
#define readline_echoing_p _rl_echoing_p 
#else
   extern int readline_echoing_p;
#endif

  This would make your patch simpler, no?

Is there a reason for replacing all
readline_echoing_p by _rl_echoing_p?


Pierre Muller
Pascal language support maintainer for GDB



> -----Message d'origine-----
> De : gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] De la
> part de Andreas Schwab
> Envoyé : Thursday, October 01, 2009 10:58 AM
> À : Ralf Corsepius
> Cc : Joel Brobecker; gdb@sourceware.org; Jan Kratochvil; Tom Tromey;
> gdb-patches@gnu.org
> Objet : Re: GDB 6.8.92 available for testing
> 
> Ralf Corsepius <ralf.corsepius@rtems.org> writes:
> 
> > --enable-system-readline also is still broken (Jan, please submit
> your
> > gdb-readline-6.0.patch you have applied to Fedora's gdb).
> 
> Here is the patch I've been using for the openSUSE package.  I have
> renamed the uses of readline_echoing_p to _rl_echoing_p to emphasize
> its
> internal nature (future readline versions may even stop exporting it).
> 
> Andreas.
> 
> 2009-10-01  Andreas Schwab  <schwab@redhat.com>
> 
> 	* tui/tui-io.c (tui_rl_display_match_list): Use _rl_echoing_p
> 	instead of readline_echoing_p for version 6 or later of
> 	libreadline.
> 
> --- gdb/tui/tui-io.c.orig	2009-03-10 11:34:01.000000000 +0100
> +++ gdb/tui/tui-io.c	2009-03-10 11:34:45.000000000 +0100
> @@ -512,8 +512,13 @@ tui_rl_display_match_list (char **matche
>  void
>  tui_setup_io (int mode)
>  {
> +#if RL_VERSION_MAJOR >= 6
> +  extern int _rl_echoing_p;
> +#else
>    extern int readline_echoing_p;
> -
> +#define _rl_echoing_p readline_echoing_p
> +#endif
> +
>    if (mode)
>      {
>        /* Redirect readline to TUI.  */
> @@ -522,12 +527,12 @@ tui_setup_io (int mode)
>        tui_old_rl_prep_terminal = rl_prep_term_function;
>        tui_old_rl_getc_function = rl_getc_function;
>        tui_old_rl_outstream = rl_outstream;
> -      tui_old_readline_echoing_p = readline_echoing_p;
> +      tui_old_readline_echoing_p = _rl_echoing_p;
>        rl_redisplay_function = tui_redisplay_readline;
>        rl_deprep_term_function = tui_deprep_terminal;
>        rl_prep_term_function = tui_prep_terminal;
>        rl_getc_function = tui_getc;
> -      readline_echoing_p = 0;
> +      _rl_echoing_p = 0;
>        rl_outstream = tui_rl_outstream;
>        rl_prompt = 0;
>        rl_completion_display_matches_hook = tui_rl_display_match_list;
> @@ -564,7 +569,7 @@ tui_setup_io (int mode)
>        rl_getc_function = tui_old_rl_getc_function;
>        rl_outstream = tui_old_rl_outstream;
>        rl_completion_display_matches_hook = 0;
> -      readline_echoing_p = tui_old_readline_echoing_p;
> +      _rl_echoing_p = tui_old_readline_echoing_p;
>        rl_already_prompted = 0;
> 
>        /* Save tty for SIGCONT.  */
> 
> --
> Andreas Schwab, schwab@redhat.com
> GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00
> 984E
> "And now for something completely different."

Re: GDB 6.8.92 available for testing

[Andreas Schwab]

"Pierre Muller" <muller@ics.u-strasbg.fr> writes:

> Andreas,
>
> why don't you simply use this?
> #if RL_VERSION_MAJOR >= 6
>   extern int _rl_echoing_p;
> #define readline_echoing_p _rl_echoing_p 
> #else
>    extern int readline_echoing_p;
> #endif
>
>   This would make your patch simpler, no?
>
> Is there a reason for replacing all
> readline_echoing_p by _rl_echoing_p?

As explained in the mail, I want to emphasize that it is an internal
readline variable (as noticeable from the leading underscore).

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: GDB 6.8.92 available for testing

[Ralf Corsepius]

On 10/01/2009 11:40 AM, Andreas Schwab wrote:
> "Pierre Muller"<muller@ics.u-strasbg.fr>  writes:
>
>> Andreas,
>>
>> why don't you simply use this?
>> #if RL_VERSION_MAJOR>= 6
>>    extern int _rl_echoing_p;
>> #define readline_echoing_p _rl_echoing_p
>> #else
>>     extern int readline_echoing_p;
>> #endif
>>
>>    This would make your patch simpler, no?
>>
>> Is there a reason for replacing all
>> readline_echoing_p by _rl_echoing_p?
>
> As explained in the mail, I want to emphasize that it is an internal
> readline variable (as noticeable from the leading underscore).

Besides this, version checks are evil.

Ralf

Re: GDB 6.8.92 available for testing

[Jan Kratochvil]

On Thu, 01 Oct 2009 14:11:19 +0200, Ralf Corsepius wrote:
> Besides this, version checks are evil.

There is still posted the autoconf-based patch:
	[patch] Fix --with-system-readline vs. readline-6.0+
	http://sourceware.org/ml/gdb-patches/2009-07/msg00755.html

while it is not The Right Thing to do it is better than the current state and
it contains no version dependencies.

Still in this case the version dependency should not hurt as AFAIK there were
no development snapshots of readline ever made public.


Regards,
Jan

Re: GDB 6.8.92 available for testing

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> There is still posted the autoconf-based patch:
Jan> 	[patch] Fix --with-system-readline vs. readline-6.0+
Jan> 	http://sourceware.org/ml/gdb-patches/2009-07/msg00755.html

Jan> while it is not The Right Thing to do it is better than the current
Jan> state and it contains no version dependencies.

I agree.  This patch is ok.

Tom

Re: GDB 6.8.92 available for testing

[Jan Kratochvil]

On Fri, 02 Oct 2009 17:39:40 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
[...]
> Jan> 	[patch] Fix --with-system-readline vs. readline-6.0+
> Jan> 	http://sourceware.org/ml/gdb-patches/2009-07/msg00755.html
[...]
> This patch is ok.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2009-10/msg00011.html


Thanks,
Jan

Re: GDB 6.8.92 available for testing

[Jan Kratochvil]

On Thu, 01 Oct 2009 05:17:24 +0200, Ralf Corsepius wrote:
> --enable-system-readline also is still broken (Jan, please submit
> your gdb-readline-6.0.patch you have applied to Fedora's gdb).

* I do not consider it appropriate for FSF GDB as the Fedora patch is just
  a hack, GDB should not make references to readline internal variables.

* FSF GDB does build with the default options.  Dependencies on external
  libraries are a task of distro vendors and the distros already contain GDB
  patches on their own.

And the fix is not straightforward as trying to avoid this internal variable
reference one has to face the echoing behavior which I find brokwn already
even with the bundled readline and the existing internal variable reference:

Standard terminal readline:
$ gdb -q
(gdb) <enter>
(gdb) 

TUI behavior:
$ gdb -q --tui
(gdb) <enter - nothing happens, it clears the same line>

The TUI window should emulate the standard terminal window so the behavior
should be the same, shouldn't it?  But I do not have a fix for this first
part.


Regards,
Jan

Re: GDB 6.8.92 available for testing

[Ralf Corsepius]

On 10/01/2009 12:09 PM, Jan Kratochvil wrote:
> On Thu, 01 Oct 2009 05:17:24 +0200, Ralf Corsepius wrote:
>> --enable-system-readline also is still broken (Jan, please submit
>> your gdb-readline-6.0.patch you have applied to Fedora's gdb).
>
> * I do not consider it appropriate for FSF GDB as the Fedora patch is just
>    a hack, GDB should not make references to readline internal variables.
Agreed, but isn't the actual hackish part in gdb?

> * FSF GDB does build with the default options.  Dependencies on external
>    libraries are a task of distro vendors and the distros already contain GDB
>    patches on their own.
You are forgetting about the 3rd parties, which apply distros as their 
basis.

Ralf

Re: GDB 6.8.92 available for testing

[Joel Brobecker]

> You are forgetting about the 3rd parties, which apply distros as their  
> basis.

Just to be clear: No one has any obligation towards you or us. We all
make the code available in some form, but we all have local modifications
that we have not submitted for one reason or another.  You probably have
your local modifications too, so why not just add Red Hat's patch in
your list, and then work on helping us fixing the problem that causes
this patch to be necessary?

-- 
Joel

Re: GDB 6.8.92 available for testing

[Ralf Corsepius]

On 10/01/2009 07:11 PM, Joel Brobecker wrote:
>> You are forgetting about the 3rd parties, which apply distros as their
>> basis.
>
> Just to be clear: No one has any obligation towards you or us.

Right, but ...

My definition of a bug is "something which doesn't work as advertised"
--enable-system-readline is of this kind

> We all
> make the code available in some form, but we all have local modifications
> that we have not submitted for one reason or another.  You probably have
> your local modifications too, so why not just add Red Hat's patch in
> your list,
That's what I already did.

> and then work on helping us fixing the problem that causes
> this patch to be necessary?
I am more than willing to assist you in doing so, but this is the long 
term solution.

A short term solution is to apply one of the proposals to upstream gdb, 
even though they are "hackerish".

Being perfectionistic and forcing users to cope with known bugs isn't 
helpful to anybody. Neither to the FSF, to upstream gdb, rtems.org nor 
OS distributors nor other arbitrary users (users: here: people building 
gdb from sources).

Ralf

Re: GDB 6.8.92 available for testing

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> * I do not consider it appropriate for FSF GDB as the Fedora patch is just
Jan>   a hack, GDB should not make references to readline internal variables.

Yes, that would be best.  What this needs is for someone to drive the
issue and contact upstream to see what they think.

Jan> And the fix is not straightforward as trying to avoid this internal
Jan> variable reference one has to face the echoing behavior which I
Jan> find brokwn already even with the bundled readline and the existing
Jan> internal variable reference:

If it is already broken, and the patch doesn't make it worse, then it
seems to me that it is just an independent bug.  Or did I misunderstand?
If this is indeed an existing TUI bug, would you file it in bugzilla?
Thanks.

Tom

Re: GDB 6.8.92 available for testing

[Jan Kratochvil]

On Fri, 02 Oct 2009 17:36:51 +0200, Tom Tromey wrote:
> If it is already broken, and the patch doesn't make it worse, then it
> seems to me that it is just an independent bug.  Or did I misunderstand?

Yes, it is an independent bug.  But changing the code for the internal
variable dependency fix cannot be verified for regressions when the current
behavior is already broken.


> If this is indeed an existing TUI bug, would you file it in bugzilla?

Filed as PR tui/10722 (the internal variable dependency as PR tui/10723).


Thanks,
Jan

Re: GDB 6.8.92 available for testing

[Joel Brobecker]

> None of the patches I had submitted to gdb-patches had been applied.

I am not aware of any patch from you that is still pending.
I remember one patch about a warning on Solaris that was rejected
because it turned out the warning was a GCC bug.

Regardless of the specific situation, submitting patches does not
mean that they will be applied, neither in the HEAD nor the branch.
If they had been applied, you would have known because, either:
you checked the patch in for you; or you asked someone else to do it,
at which point the commit would have been confirmed to you and the
gdb-patches list.

-- 
Joel

Re: GDB 6.8.92 available for testing

[Ralf Corsepius]

On 10/01/2009 07:07 PM, Joel Brobecker wrote:
>> None of the patches I had submitted to gdb-patches had been applied.
>
> I am not aware of any patch from you that is still pending.
> I remember one patch about a warning on Solaris that was rejected
> because it turned out the warning was a GCC bug.
>
> Regardless of the specific situation, submitting patches does not
> mean that they will be applied, neither in the HEAD nor the branch.
> If they had been applied, you would have known because, either:
> you checked the patch in for you; or you asked someone else to do it,
> at which point the commit would have been confirmed to you and the
> gdb-patches list.
>

Eg. this one:
http://sourceware.org/ml/gdb-patches/2009-09/msg00585.html

and this one:
http://sourceware.org/ml/gdb-patches/2009-09/msg00556.html

But I presume, I can consider the later one to be rejected - This 
doesn't help anybody, but ... you want it this way, so be it.

Ralf

Re: GDB 6.8.92 available for testing

[Mark Kettenis]

> Date: Thu, 01 Oct 2009 19:20:54 +0200
> From: Ralf Corsepius <ralf.corsepius@rtems.org>
> 
> and this one:
> http://sourceware.org/ml/gdb-patches/2009-09/msg00556.html
> 
> But I presume, I can consider the later one to be rejected - This 
> doesn't help anybody, but ... you want it this way, so be it.

Are we still turning off -Werror for releases?  In that case, there
really isn't a problem.

Re: GDB 6.8.92 available for testing

[Joel Brobecker]

> Are we still turning off -Werror for releases?  In that case, there
> really isn't a problem.

We don't anymore, but we can fix that. We have to decide whether to
do it for this release, and what we want to do for future releases
in general. It would make it simpler for non-contributors if the
releases were built without -Werror.

-- 
Joel

Re: GDB 6.8.92 available for testing

[Mark Kettenis]

> Date: Thu, 1 Oct 2009 11:27:40 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > Are we still turning off -Werror for releases?  In that case, there
> > really isn't a problem.
> 
> We don't anymore, but we can fix that. We have to decide whether to
> do it for this release, and what we want to do for future releases
> in general. It would make it simpler for non-contributors if the
> releases were built without -Werror.

Given that newer GCC versions get whinier and whinier and that we see
false positives from time to time, I would be in favour of disabling
-Werror for releases.

Re: GDB 6.8.92 available for testing

[Eli Zaretskii]

> Date: Thu, 1 Oct 2009 20:36:39 +0200 (CEST)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> CC: mark.kettenis@xs4all.nl, ralf.corsepius@rtems.org, gdb@sourceware.org,         jan.kratochvil@redhat.com, tromey@redhat.com
> 
> Given that newer GCC versions get whinier and whinier and that we see
> false positives from time to time, I would be in favour of disabling
> -Werror for releases.

Same here.

Re: GDB 6.8.92 available for testing

[Paul Pluzhnikov]

On Thu, Oct 1, 2009 at 11:27 AM, Joel Brobecker <brobecker@adacore.com> wrote:

> We don't anymore, but we can fix that. We have to decide whether to
> do it for this release, and what we want to do for future releases
> in general. It would make it simpler for non-contributors if the
> releases were built without -Werror.

FWIW, CVS Head generates a *ton* of warnings on Darwin (Leopard);
I had to turn -Werror off to build it (yesterday).

-- 
Paul Pluzhnikov

Re: GDB 6.8.92 available for testing

[Joel Brobecker]

> Eg. this one:
> http://sourceware.org/ml/gdb-patches/2009-09/msg00585.html

There is a misunderstanding. Your patch was reviewed, and approved,
but *you* either have to commit it, or ask someone to commit it for you.
If you have an FSF assignment on file for GDB, then it's preferable
for you to get write access to the GDB repository.

> and this one:
> http://sourceware.org/ml/gdb-patches/2009-09/msg00556.html
>
> But I presume, I can consider the later one to be rejected - This  
> doesn't help anybody, but ... you want it this way, so be it.

The patch looked suspicious when I looked at it, which is why I said
that I didn't understand what it was doing.  I prefer to not apply
any change that I don't understand.

-- 
Joel

Re: GDB 6.8.92 available for testing

[Tom Tromey]

>>>>> "Ralf" == Ralf Corsepius <ralf.corsepius@rtems.org> writes:

Ralf> Eg. this one:
Ralf> http://sourceware.org/ml/gdb-patches/2009-09/msg00585.html

I checked this in.

Ralf> and this one:
Ralf> http://sourceware.org/ml/gdb-patches/2009-09/msg00556.html

Ralf> But I presume, I can consider the later one to be rejected - This
Ralf> doesn't help anybody, but ... you want it this way, so be it.

I think 'len' should be 'unsigned int', not 'size_t'.
That conforms with the definition of struct type::length.
Does that change still fix the problem for you?

Tom

Re: GDB 6.8.92 available for testing

[Ralf Corsepius]

On 10/02/2009 05:47 PM, Tom Tromey wrote:
>>>>>> "Ralf" == Ralf Corsepius<ralf.corsepius@rtems.org>  writes:
>>>>>>              
> Ralf>  Eg. this one:
> Ralf>  http://sourceware.org/ml/gdb-patches/2009-09/msg00585.html
>
> I checked this in.
>
> Ralf>  and this one:
> Ralf>  http://sourceware.org/ml/gdb-patches/2009-09/msg00556.html
>
> Ralf>  But I presume, I can consider the later one to be rejected - This
> Ralf>  doesn't help anybody, but ... you want it this way, so be it.
>
> I think 'len' should be 'unsigned int', not 'size_t'.
>    
I think, len should be size_t, like any "sizeof"'s return type.


This also manifests in how "len" is being used in the fragment in question:

sparc32_store_return_value (struct type *type, struct regcache *regcache,
                             const gdb_byte *valbuf)
{
   size_t len = TYPE_LENGTH (type);
   gdb_byte buf[8];

   gdb_assert (!sparc_structure_or_union_p (type));
   gdb_assert (!(sparc_floating_p (type) && len == 16));

   if (sparc_floating_p (type))
     {
       /* Floating return values.  */
       memcpy (buf, valbuf, len);
       regcache_cooked_write (regcache, SPARC_F0_REGNUM, buf);
       if (len > 4)
         regcache_cooked_write (regcache, SPARC_F1_REGNUM, buf + 4);
     }
   else
     {
       /* Integral and pointer return values.  */
       gdb_assert (sparc_integral_or_pointer_p (type));

       if (len > 4)
         {
           gdb_assert (len == 8);
           memcpy (buf, valbuf, 8);
           regcache_cooked_write (regcache, SPARC_O1_REGNUM, buf + 4);
         }
       else
         {
           /* ??? Do we need to do any sign-extension here?  */
           memcpy (buf + 4 - len, valbuf, len);
         }
       regcache_cooked_write (regcache, SPARC_O0_REGNUM, buf);
     }
}
...

Note: memcpy(..., len)

Apart of this, I can't imagine changing this "int" into "size_t" to have 
any negative impact.

That is, though the origin for the patch might be a bug in GCC, I don't 
see how the patch can be harmful.

Ralf

Re: GDB 6.8.92 available for testing

[Tom Tromey]

>>>>> "Ralf" == Ralf Corsepius <ralf.corsepius@rtems.org> writes:

Ralf> But I presume, I can consider the later one to be rejected - This
Ralf> doesn't help anybody, but ... you want it this way, so be it.

Tom> I think 'len' should be 'unsigned int', not 'size_t'.

Ralf> I think, len should be size_t, like any "sizeof"'s return type.

TYPE_LENGTH does not expand to sizeof.

Ralf> Apart of this, I can't imagine changing this "int" into "size_t" to
Ralf> have any negative impact.

Yes, I agree.

I have a slight preference for locals such as this to be declared with
the correct-according-to-gdb type.

Tom

Re: GDB 6.8.92 available for testing

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Fri, 02 Oct 2009 11:27:12 -0600
>
> Ralf> I think, len should be size_t, like any "sizeof"'s return type.
> 
> TYPE_LENGTH does not expand to sizeof.
> 
> Ralf> Apart of this, I can't imagine changing this "int" into "size_t" to
> Ralf> have any negative impact.
> 
> Yes, I agree.
> 
> I have a slight preference for locals such as this to be declared with
> the correct-according-to-gdb type.

Seconded.

However, the idiom:

  int len = TYPE_LENGTH(...);

  memcpy(..., ..., len);

is used in several other places in the GDB tree.  I really think that
we should fix all these cases; not just this one.

Re: GDB 6.8.92 available for testing

[Joel Brobecker]

> However, the idiom:
> 
>   int len = TYPE_LENGTH(...);
> 
>   memcpy(..., ..., len);
> 
> is used in several other places in the GDB tree.  I really think that
> we should fix all these cases; not just this one.

Perhaps the type of length in struct type should be changed to size_t?

-- 
Joel

Re: GDB 6.8.92 available for testing

[Mark Kettenis]

> Date: Fri, 2 Oct 2009 11:14:47 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > However, the idiom:
> > 
> >   int len = TYPE_LENGTH(...);
> > 
> >   memcpy(..., ..., len);
> > 
> > is used in several other places in the GDB tree.  I really think that
> > we should fix all these cases; not just this one.
> 
> Perhaps the type of length in struct type should be changed to size_t?

I think that would be a bad idea since size_t will be a 64-bit type on
64-bit hosts.  This will increase memory usage on 6-bit hosts.

Re: GDB 6.8.92 available for testing

[Jan Kratochvil]

On Fri, 02 Oct 2009 20:23:31 +0200, Mark Kettenis wrote:
> I think that would be a bad idea since size_t will be a 64-bit type on
> 64-bit hosts.  This will increase memory usage on 6-bit hosts.

We do need 64-bit target TYPE_LENGTH support.  There exist >2GB debuginfos:
	http://cvs.fedora.redhat.com/viewvc/rpms/gdb/devel/gdb-6.3-bz231832-obstack-2gb.patch?view=co

and I guess some HPC (High Performance Computing) is already exceeding it for
runtime data object size.  With dynamic data type sizes
(archer-jankratochvil-vla) such current dynamic size is stored in TYPE_LENGTH.


Just it should not be size_t but target_size_t.  Yes, it should be CORE_ADDR.


Regards,
Jan

Re: GDB 6.8.92 available for testing

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> We do need 64-bit target TYPE_LENGTH support.

That will be a fun patch for somebody to write :)

This is ok to put on the archer task list, if you want to put it there.

Jan>  There exist >2GB debuginfos:
Jan> 	http://cvs.fedora.redhat.com/viewvc/rpms/gdb/devel/gdb-6.3-bz231832-obstack-2gb.patch?view=co

I didn't like this patch when I first saw it, but maybe I was misreading
it.  On first read it looked like it didn't fully update obstacks.

I wonder how important this is now that we mmap the debuginfo.
Do we have a test case for it?

Tom

Re: GDB 6.8.92 available for testing

[Jan Kratochvil]

On Fri, 02 Oct 2009 23:45:33 +0200, Tom Tromey wrote:
> This is ok to put on the archer task list, if you want to put it there.

Done.


> Jan>  There exist >2GB debuginfos:
> Jan> 	http://cvs.fedora.redhat.com/viewvc/rpms/gdb/devel/gdb-6.3-bz231832-obstack-2gb.patch?view=co
> 
> I didn't like this patch when I first saw it, but maybe I was misreading
> it.  On first read it looked like it didn't fully update obstacks.

The obstacks patch should go first to glibc but it never made it there, it
still should be pushed.
	http://sourceware.org/ml/libc-alpha/2007-01/msg00090.html


> I wonder how important this is now that we mmap the debuginfo.

Still the generated indexes can get large.


> Do we have a test case for it?

As it is very expensive to run it was just made for one-shot manual run.


Regards,
Jan


--- /dev/null	2009-09-27 23:35:12.323860784 +0200
+++ Makefile	2007-03-12 14:00:43.000000000 +0100
@@ -0,0 +1,19 @@
+SYMNUM = $$[0x880000]
+SYMLEN = $$[0x100]
+CFLAGS = -ggdb3 -Wall
+
+all: main
+
+macro.h: gen Makefile
+	./$< $(SYMNUM) $(SYMLEN) >$@
+
+library.so: library.c
+	$(LINK.c) $< $(LOADLIBES) $(LDLIBS) -o $@ -shared -fPIC
+
+main: main.c macro.h library.so
+	$(LINK.c) $< $(LOADLIBES) $(LDLIBS) -o $@ library.so -Wl,-rpath,.
+
+.PHONY: clean
+clean:
+	$(RM) main gen macro.h library.so
+
--- /dev/null	2009-09-27 23:35:12.323860784 +0200
+++ gen.c	2007-01-21 02:27:08.000000000 +0100
@@ -0,0 +1,23 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+
+int main (int argc, char **argv)
+{
+  int symnum, symlen;
+  int symi, symj;
+
+  symnum = atoi (argv[1]);
+  symlen = atoi (argv[2]);
+
+  for (symi = 0; symi < symnum; symi++)
+    {
+      printf ("#define MACRO%d ", symi);
+      for (symj = 1; symj <= symlen; symj++)
+        putchar ('0' + symj % 10);
+      putchar ('\n');
+    }
+  puts ("/* EOF */");
+
+  return 0;
+}
--- /dev/null	2009-09-27 23:35:12.323860784 +0200
+++ library.c	2007-03-11 16:25:00.000000000 +0100
@@ -0,0 +1,6 @@
+#include "macro.h"
+
+
+void libfunc (void)
+{
+}
--- /dev/null	2009-09-27 23:35:12.323860784 +0200
+++ main.c	2007-03-11 16:24:57.000000000 +0100
@@ -0,0 +1,7 @@
+extern void libfunc (void);
+
+int main (void)
+{
+  libfunc ();
+  return 0;
+}

Re: GDB 6.8.92 available for testing

[Tom Tromey]

>>>>> "Ralf" == Ralf Corsepius <ralf.corsepius@rtems.org> writes:

Ralf> None of the patches I had submitted to gdb-patches had been applied.

I don't recall whether you said that you didn't have write access.
Could you supply URLs for the patches that were approved but not
applied?  Or just email them to me?  Thanks.

Tom

RE: GDB 6.8.92 available for testing

[Pierre Muller]

Hi Joel,
I tried to test this on i686-pc-linux
(Ubuntu server running in a VMware box on a windows XP)

Linux vadmin 2.6.28-15-server #52-Ubuntu SMP Wed Sep 9 11:50:50 UTC 2009
i686 GNU/Linux
gcc (Ubuntu 4.3.3-5ubuntu4) 4.3.3

# of expected passes		13434
# of unexpected failures	49
# of expected failures		43
# of unknown successes		1
# of known failures		59
# of untested testcases		13
# of unsupported tests		22
/usr/local/src/gdb-7pre/build/gdb/testsuite/../../gdb/gdb version  6.8.92
-nw -nx 


The test results are not that bad, but
it seems that I got a lot of UNTESTED
which seem to be related to the fact
that some warning flags are on by default on
the gcc that is installed.

This creates compilation failures in
1) gdb.base/checkpoint.c (wrong format arg type)
2) gdb.base/fileio.c (wrong format arg type)
3) gdb.base/shr1.c (wrong format arg type)
4) gdb.base/unload.c (format arg not literal)
5) gdb.base/watchpoint-solib.c (format arg not literal)  
6) gdb.threads/attachstop-mt.c (wrong format arg type)

1), 2), and 6) are easy to fix with explicit typecasts

3) is about an address, but "%p" does not seem to be supported by all
C lib format implementation, so I don't know if this can be fixed correctly.

4) and 5) seem more difficult ...

Maybe adding some default GCC option to the
site.exp could fix this?


Pierre Muller
Pascal language support maintainer for GDB

Re: GDB 6.8.92 available for testing

[Joel Brobecker]

> 3) gdb.base/shr1.c (wrong format arg type)
> 3) is about an address, but "%p" does not seem to be supported by all
> C lib format implementation, so I don't know if this can be fixed correctly.

We already use %p in a couple of testcase, so let's use it again here.
Best to avoid printing stuff on stdout/stderr when writing a testcase,
as we don't always have access to the inferior output (for instance,
when doing remote debugging), so matching the inferior output in the
testcase won't work in those case.  But we'd have to dig deeper in
the history of this testcase to determine whether removing the printf
would not impact the effectiveness of the testcase.

For now, I vote for a group hug and the use of %p.

> 4) gdb.base/unload.c (format arg not literal)
> 5) gdb.base/watchpoint-solib.c (format arg not literal)  
> 4) and 5) seem more difficult ...

Is that the source of the problem?

        fprintf (stderr, dlerror ());

The following should take care of it:

        fprintf (stderr, "%s", dlerror ())

-- 
Joel

[RFA] more fixes to testsuite (was RE: GDB 6.8.92 available for testing)

[Pierre Muller]

  I here request approval 
for changes to the testsuite related to 
problems on a configuration where
gcc emits format warnings by default.

> -----Message d'origine-----
> De : gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] De la
> part de Joel Brobecker
> Envoyé : Thursday, October 01, 2009 7:29 PM
> À : Pierre Muller
> Cc : gdb@sourceware.org
> Objet : Re: GDB 6.8.92 available for testing
> 
> > 3) gdb.base/shr1.c (wrong format arg type)
> > 3) is about an address, but "%p" does not seem to be supported by all
> > C lib format implementation, so I don't know if this can be fixed
> correctly.
> 
> We already use %p in a couple of testcase, so let's use it again here.
> Best to avoid printing stuff on stdout/stderr when writing a testcase,
> as we don't always have access to the inferior output (for instance,
> when doing remote debugging), so matching the inferior output in the
> testcase won't work in those case.  But we'd have to dig deeper in
> the history of this testcase to determine whether removing the printf
> would not impact the effectiveness of the testcase.

 I used "%p" as suggested.
  
> For now, I vote for a group hug and the use of %p.
> 
> > 4) gdb.base/unload.c (format arg not literal)
> > 5) gdb.base/watchpoint-solib.c (format arg not literal)
> > 4) and 5) seem more difficult ...
> 
> Is that the source of the problem?
> 
>         fprintf (stderr, dlerror ());
> 
> The following should take care of it:
> 
>         fprintf (stderr, "%s", dlerror ())
> 
> --
> Joel
 This was a bit more tricky, because
dlerror is a macro if __WIN32__
is defined, 
  I removed the macro and
separated the fprintf.

Is this OK?

 
Tested on gcc16, no changes in the results of these tests.

PS: the #ifdef __WIN32__
should probably be replaced by 
something that is also defined for
64-bit windows, no?


2009-10-01  Pierre Muller  <muller@ics.u-strasbg.fr>

	* src/gdb/testsuite/gdb.base/shr1.c: Use %p in format string.
	* src/gdb/testsuite/gdb.base/unload.c: Avoid warning in fprintf.
	* src/gdb/testsuite/gdb.base/watchpoint-solib.c: Idem.

Index: src/gdb/testsuite/gdb.base/shr1.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/shr1.c,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 shr1.c
--- src/gdb/testsuite/gdb.base/shr1.c	28 Jun 1999 16:04:00 -0000
1.1.1.2
+++ src/gdb/testsuite/gdb.base/shr1.c	1 Oct 2009 20:58:06 -0000
@@ -25,7 +25,7 @@ int x;
   sg = 6.6;
   sgi++;
   sgs = 8;
-  printf("address of sgs is 0x%x\n", &sgs);
+  printf("address of sgs is %p\n", &sgs);
   return 2*x;
 }
 
Index: src/gdb/testsuite/gdb.base/unload.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/unload.c,v
retrieving revision 1.9
diff -u -p -r1.9 unload.c
--- src/gdb/testsuite/gdb.base/unload.c	3 Jan 2009 05:58:03 -0000	1.9
+++ src/gdb/testsuite/gdb.base/unload.c	1 Oct 2009 20:58:06 -0000
@@ -27,7 +27,6 @@
 # define dlsym(handle, func) GetProcAddress (handle, func)
 #endif
 #define dlclose(handle) FreeLibrary (handle)
-#define dlerror() "error %d occurred", GetLastError ()
 #else
 #include <dlfcn.h>
 #endif
@@ -53,7 +52,11 @@ int main()
 
   if (!unloadshr)
     {
-      fprintf (stderr, dlerror ());
+#ifdef __WIN32__
+      fprintf (stderr, "error %d occurred", GetLastError ());
+#else
+      fprintf (stderr, "%s", dlerror ());
+#endif
       exit (1);
     }
 
Index: src/gdb/testsuite/gdb.base/watchpoint-solib.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/watchpoint-solib.c,v
retrieving revision 1.2
diff -u -p -r1.2 watchpoint-solib.c
--- src/gdb/testsuite/gdb.base/watchpoint-solib.c	3 Jan 2009 05:58:03
-0000	1.2
+++ src/gdb/testsuite/gdb.base/watchpoint-solib.c	1 Oct 2009 20:58:06
-0000
@@ -27,7 +27,6 @@
 # define dlsym(handle, func) GetProcAddress (handle, func)
 #endif
 #define dlclose(handle) FreeLibrary (handle)
-#define dlerror() "error %d occurred", GetLastError ()
 #else
 #include <dlfcn.h>
 #endif
@@ -42,7 +41,11 @@ void open_shlib ()
   
   if (!handle)
     {
-      fprintf (stderr, dlerror ());
+#ifdef __WIN32__
+      fprintf (stderr, "error %d occurred", GetLastError ());
+#else
+      fprintf (stderr, "%s", dlerror ());
+#endif
       exit (1);
     }
 

Re: [RFA] more fixes to testsuite (was RE: GDB 6.8.92 available for testing)

[Joel Brobecker]

> PS: the #ifdef __WIN32__
> should probably be replaced by 
> something that is also defined for
> 64-bit windows, no?

We're "lucky" in this case, because __WIN32__ happens to be defined
on Windows64.

> 2009-10-01  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	* src/gdb/testsuite/gdb.base/shr1.c: Use %p in format string.
> 	* src/gdb/testsuite/gdb.base/unload.c: Avoid warning in fprintf.
> 	* src/gdb/testsuite/gdb.base/watchpoint-solib.c: Idem.

I think it would have been simpler to defile a dlerror *function*
that returns a string, but this is fine too.

-- 
Joel