Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Eli Zaretskii]

> Date: Sat, 22 Sep 2001 19:08:50 -0400
> From: Andrew Cagney <ac131313@cygnus.com>
> 
> 
> Turns out there is already a ``maint info'' so a ``maint set/show'' 
> makes good sense.  Try the attached.
> 
> Jason, I think it is pretty clear where to add the profiling bit :-)
> Eli, if the doco entry is going in gdb.texinfo, can you suggest where?

Hmm.. I don't see any doco entry in the patch you posted.  Did you
mean Jason's patches?  In that case, the chapter "Controlling GDB"
seems like a good place.  I'd say either create a new section
"Profiling GDB" or add the text to the section "Debugging Output".

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Fernando Nasser]

Andrew Cagney wrote:
> 
> > On Wed, 19 Sep 2001, Kevin Buettner wrote:
> >
> >
> >> I like having the maintenance commands all lumped under "maint".  That
> >> way I can use tab completion to see all of the maintenance related commands
> >> when I do
> >>
> >> (gdb) maint <TAB>
> >>
> >> I think a set/show would be useful for some activities; couldn't
> >> we add "maint set ..." and "maint show ..."  ?
> >
> >
> > That seems to be a good idea, IMHO.
> 
> Turns out there is already a ``maint info'' so a ``maint set/show''
> makes good sense.  Try the attached.
> 

Nice.

> Jason, I think it is pretty clear where to add the profiling bit :-)
> Eli, if the doco entry is going in gdb.texinfo, can you suggest where?
> Fernando, is this part ok?
> 

Yes, sure.

P.S.: There are some maint commands that should be maint set/show in 
the arm rdi thing.  Can you add fixing those to the TODO list, please?  


-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Andrew Cagney]

> P.S.: There are some maint commands that should be maint set/show in 
> the arm rdi thing.  Can you add fixing those to the TODO list, please?  

Just file change-request PRs: http://sources.redhat.com/gdb/bugs
Andrew

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Andrew Cagney]

>> Fernando, is this part ok?
>> 
> 
> 
> Yes, sure.

Thanks.  I've checked it in but with a tweek:

+ #ifdef NOTYET
+   /* FIXME: cagney/2001-09-24: A patch introducing a
+      add_set_boolean_cmd() is pending, the below should probably use
+      it.  A patch implementing profiling is pending, this just sets up
+      the framework.  */
+   tmpcmd = add_set_cmd ("profile", class_maintenance,
+                       var_boolean, &maintenance_profile_p,
+                       "Set internal profiling.\n\
+ When enabled GDB is profiled.",
+                       &maintenance_set_cmdlist);
+   tmpcmd->function.sfunc = maintenance_set_profile_cmd;
+   add_show_from_set (tmpcmd, &maintenance_show_cmdlist);
+ #endif

I wrapped the ``maint set profile'' stuff in #ifdef NOTYET for the moment.

Andrew

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Andrew Cagney]

> 
> What's a CR?  You didn't mean CR the character, \r, did you? 

Sorry, Change Reqest, one of the PR categories in the bug database. If 
I'm not careful, I'll be refering to them as SPRs (system performance 
reports) next ....

Andrew

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Eli Zaretskii]

> Date: Mon, 24 Sep 2001 17:41:38 -0400
> From: Andrew Cagney <ac131313@cygnus.com>
> 
> For what its worth.   I think this part of this discussion thread should 
> be converted into a CR

What's a CR?  You didn't mean CR the character, \r, did you? ;-)

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Eli Zaretskii]

> Date: Mon, 24 Sep 2001 13:32:15 -0700
> From: Jason Molenda <jason@molenda.com>
> 
> I'm a little hesitant to use this emacs code... besides having an
> #ifdef FreeBSD around it, it uses the address of a certain function
> for the beginning of the text section -- hardcoded.  So unless
> someone can guess which one of gdb's functions is going to end up
> at the beginning of .text, I don't see how that'd work.

The first function in the binary is the first function the linker
sees.  If this is platform-dependent in GDB, we could simply arrange
for an empty function that every platform links in.

> Okay, I would have been happier if someone had just said "Approved!"
> and it had gone in immediately :-)

I can't be that someone: it's not my responsibility ;-)

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Andrew Cagney]

>> > > /* Gerd Moellmann <gerd@acm.org> says this makes profiling work on
>> > > FreeBSD.  It might work on some other systems too.
>> > > Give it a try and tell me if it works on your system.  */

For what its worth.   I think this part of this discussion thread should 
be converted into a CR - something about profile always creating a 
profile file and notes on possible solutions.

Andrew

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Jason Molenda]

Sorry for the delay following up, I was sidetracked by some other 
things for a few days.

On Tue, Sep 18, 2001 at 09:51:16AM +0300, Eli Zaretskii wrote:

> > from emacs 20.7:
> > 
> > >   /* Gerd Moellmann <gerd@acm.org> says this makes profiling work on
> > >      FreeBSD.  It might work on some other systems too.
> > >      Give it a try and tell me if it works on your system.  */
> [...]
> > I get the feeling that this is work in progress.
> 
> This file is almost 2 years old.  Since then, this code was proven to
> run unmodified on quite a few platforms.

I'm a little hesitant to use this emacs code... besides having an
#ifdef FreeBSD around it, it uses the address of a certain function
for the beginning of the text section -- hardcoded.  So unless
someone can guess which one of gdb's functions is going to end up
at the beginning of .text, I don't see how that'd work.

It looks like emacs (in 20.7) is guarding all of these calls to
monstartup() and moncontrol() with #ifdef PROFILING.

I don't follow emacs development at all, so it's possible the
development sources are significantly different.

> Anyway, I don't want to continue arguing about this.  

I appreciate the time you're taking with this.  Okay, I would have
been happier if someone had just said "Approved!" and it had gone
in immediately :-), but it'll be a better patch by the time it does
get committed.

> As long as you
> know that the problems Jason raised can be solved with simple and
> quite portable code, all the rest is your judgement call.

I'm willing to believe that there are portable ways to do everything
we're talking about, but _I_ don't know them, and the emacs 20.7 code
doesn't show any examples of this...

I'll be updating the profiling patch and sending it again soon.

Jason

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Kevin Buettner]

On Sep 19, 12:26pm, Fernando Nasser wrote:

> Andrew has proposed creating a class of set/show commands "set/show maint"
> to replace those.  It would be used for instance to turn gdb self profiling
> on/off.  I proposed that we use the deprecate the existent maint commands that
> currently set things directly and add the new "set/show maint" for them.
> 
> So, for maintenance we would have:
> 
> maint <action> [<args>]
> set maint <control var> <value>
> show maint [<control var>]
> 
> All these are introspective commands that will examine/affect gdb in order
> to debug, test or even to understand it.
> 
> Anyway, it is a modification on the current "maintenance" interface, that is
> why I asked for some support beyond the two of us.

I like having the maintenance commands all lumped under "maint".  That
way I can use tab completion to see all of the maintenance related commands
when I do

    (gdb) maint <TAB>

I think a set/show would be useful for some activities; couldn't
we add "maint set ..." and "maint show ..."  ?

Kevin

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Andrew Cagney]

> maint <action> [<args>]
> set maint <control var> <value>
> show maint [<control var>]

just an aside to this, there is also

	set/show debug ....

but I don't think we need to change that to ``set/show maint debug 
...''.  ``set/show debug ...'' is very specific enable/disableing 
internal debug tracing.

enjoy,
Andrew

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Eli Zaretskii]

> Date: Wed, 19 Sep 2001 12:26:45 -0400
> From: Fernando Nasser <fnasser@cygnus.com>
> 
> So, for maintenance we would have:
> 
> maint <action> [<args>]
> set maint <control var> <value>
> show maint [<control var>]

Ah, this seems a good idea.  Thanks for the explanations.

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Fernando Nasser]

Eli Zaretskii wrote:
> 
> > Date: Wed, 19 Sep 2001 10:07:55 -0400
> > From: Fernando Nasser <fnasser@cygnus.com>
> >
> > Also, I would like someone else to support us on this move: Tom? Jason? Eli?
> 
> Sorry, I probably wasn't following this thread closely enough: what
> move did you have in mind?

We have some maint commands that actually set values.  Either turn things
on/off or set control variables (for instance, name of a log file).
As these are just plain commands, there is no way to examine the current
settings.

Andrew has proposed creating a class of set/show commands "set/show maint"
to replace those.  It would be used for instance to turn gdb self profiling
on/off.  I proposed that we use the deprecate the existent maint commands that
currently set things directly and add the new "set/show maint" for them.

So, for maintenance we would have:

maint <action> [<args>]
set maint <control var> <value>
show maint [<control var>]

All these are introspective commands that will examine/affect gdb in order
to debug, test or even to understand it.

Anyway, it is a modification on the current "maintenance" interface, that is
why I asked for some support beyond the two of us.

-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Eli Zaretskii]

> Date: Wed, 19 Sep 2001 10:07:55 -0400
> From: Fernando Nasser <fnasser@cygnus.com>
> 
> Also, I would like someone else to support us on this move: Tom? Jason? Eli?

Sorry, I probably wasn't following this thread closely enough: what
move did you have in mind?

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Fernando Nasser]

Andrew Cagney wrote:
> 
> >> Jason, should have thought of this earlier, sorry.  Should this be
> >> ``set/show maint profile-gdb''?  ``set/show remote ...'' provides
> >> cut/paste for implementing this.
> >>
> >> As far as I can tell it isn't possible to examine the value of profile-gdb.
> >>
> >
> >
> > It seems to be a general problem with "maint" things that are settings
> > (instead of an action).
> >
> > We should eventually create a "set/show maint" thing similar to
> > "set/show debug" (humm, what is the difference here?).
> > I would leave it for a separate patch which would do it for all
> > the maint related settings.
> 
> If you're ok with ``set/show maint ...'' I can clone ``set/show remote
> ...'' into what is needed.
> 

I think it is a good idea. But we should leave the things that are
currently set with the maint command marked as deprecated and
with the appropriate message telling to use "set maint" instead.

Also, I would like someone else to support us on this move: Tom? Jason? Eli?

-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Andrew Cagney]

>> Jason, should have thought of this earlier, sorry.  Should this be
>> ``set/show maint profile-gdb''?  ``set/show remote ...'' provides
>> cut/paste for implementing this.
>> 
>> As far as I can tell it isn't possible to examine the value of profile-gdb.
>> 
> 
> 
> It seems to be a general problem with "maint" things that are settings
> (instead of an action).
> 
> We should eventually create a "set/show maint" thing similar to
> "set/show debug" (humm, what is the difference here?).
> I would leave it for a separate patch which would do it for all
> the maint related settings.


If you're ok with ``set/show maint ...'' I can clone ``set/show remote 
...'' into what is needed.

Andrew

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Fernando Nasser]

Andrew Cagney wrote:
> 
> > +  add_cmd ("profile-gdb", class_maintenance, maint_profile_gdb,
> > +        "Enable or disable profiling.",
> > +        &maintenancelist);
> 
> Jason, should have thought of this earlier, sorry.  Should this be
> ``set/show maint profile-gdb''?  ``set/show remote ...'' provides
> cut/paste for implementing this.
> 
> As far as I can tell it isn't possible to examine the value of profile-gdb.
> 

It seems to be a general problem with "maint" things that are settings
(instead of an action).

We should eventually create a "set/show maint" thing similar to
"set/show debug" (humm, what is the difference here?).
I would leave it for a separate patch which would do it for all
the maint related settings.


BTW, "maint profile-gdb" is redundant.  maint commands are supposed to be
introspective already.  The target is always gdb itself, never the target.
Someone mentioned that the "-gdb" should be dropped and I agree.  But I do
not mind if it stays as it is as "maint profile" will work anyways.


-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Eli Zaretskii]

> Date: Tue, 18 Sep 2001 01:43:02 -0400
> From: Andrew Cagney <ac131313@cygnus.com>
> 
> > Emacs uses monstartup and manages to solve both of these problems.
> > See the file src/emacs.c (search for "etext") for the details.
> 
> from emacs 20.7:
> 
> >   /* Gerd Moellmann <gerd@acm.org> says this makes profiling work on
> >      FreeBSD.  It might work on some other systems too.
> >      Give it a try and tell me if it works on your system.  */
[...]
> I get the feeling that this is work in progress.

This file is almost 2 years old.  Since then, this code was proven to
run unmodified on quite a few platforms.

Anyway, I don't want to continue arguing about this.  As long as you
know that the problems Jason raised can be solved with simple and
quite portable code, all the rest is your judgement call.

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Andrew Cagney]

> 
> Emacs uses monstartup and manages to solve both of these problems.
> See the file src/emacs.c (search for "etext") for the details.

from emacs 20.7:

>   /* Gerd Moellmann <gerd@acm.org> says this makes profiling work on
>      FreeBSD.  It might work on some other systems too.
>      Give it a try and tell me if it works on your system.  */
> #ifdef __FreeBSD__
> #ifdef PROFILING
>   if (initialized)
>     {
>       extern void _mcleanup ();       
>       extern char etext;
>       extern Lisp_Object Fredraw_frame ();
>       atexit (_mcleanup);
>       /* This uses Fredraw_frame because that function
>          comes first in the Emacs executable.
>          It might be better to use something that gives
>          the start of the text segment, but start_of_text
>          is not defined on all systems now.  */
>       monstartup (Fredraw_frame, &etext);
>     }
>   else
>     moncontrol (0);
> #endif
> #endif

I get the feeling that this is work in progress.  I'd, for the moment, 
suggest using Jason's mechanism - it isn't perfect  but it works. 
Someone can later make it more fancy.

Andrew

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Andrew Cagney]

> +  add_cmd ("profile-gdb", class_maintenance, maint_profile_gdb,
> +	   "Enable or disable profiling.",
> +	   &maintenancelist);

Jason, should have thought of this earlier, sorry.  Should this be 
``set/show maint profile-gdb''?  ``set/show remote ...'' provides 
cut/paste for implementing this.

As far as I can tell it isn't possible to examine the value of profile-gdb.

Andrew

Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command

[Michael Snyder]

Eli Zaretskii wrote:

> I'd like the documentation of the "maint profile-gdb" command to be in
> gdb.texinfo, not gdbint.texinfo.  The latter doesn't document user
> commands, and users might not readily know to look in gdbint.  (It's
> okay to have it described in both manuals, if you think it will be
> useful.)

At first I thought this was weird, but it's not.
Maintainer commands are documented in the user docs, not
the internals docs.  It has to do with maintaining gdb, 
not with gdb internals.  Good call.

[RFA] Version 2 of patch to add 'maint profile-gdb' command

[Jason Molenda]

Here is a minor rewrite of the profiling patch.  I removed the
PARAMS, fixed the NEWS entry, and added a check for moncontrol()
with -pg - if this fails to compile (as it fails on sources.redhat.com),
an error message is spit out and the configure aborts.  If a warning
is desirable, it's a minor change from calling AC_MSG_ERROR to
AC_MSG_WARN, but I found that to be too subtle in my testing (the
warning was lost among the configure output and people would be
confused why their --enable-profiling gdb isn't actually profiling.)
I amended the gdbint.texinfo documentation to note that the gmon.out
file is overwritten each time gdb is started.

The one notable change I did not incorporate was compiling with
-pg, linking without -pg, and using monstartup() to enable the
profiling.  I have two problems with this - one, I _suspect_ (but
I do not know for certain) that linking without -pg and just calling
monstartup() is going to be unportable.  The second problem is
that, off-hand, I don't know how a program should determine the
start and end addresses of its TEXT section portably (which are
required to use monstartup).

I agree that using monstartup() would be convenient, as developers
would not constantly overwrite their existing gmon.out file whenever
they start gdb, but I'd like to see this patch committed and someone
who understands the mechanisms of profiling across different systems
could tackle it in the future.  In my opinion, monstartup() as a nice
refinement which a motivated person could tackle in the future.  I'd
rather not complicate the basic feature with this at this point.
This patch was written nearly two years ago - it'd be good to get
it integrated.

NB - autoconf and autoheader must both be run after applying this patch.

Thanks for your help,

Jason
From eliz@is.elta.co.il Sun Sep 16 22:36:00 2001
From: Eli Zaretskii <eliz@is.elta.co.il>
To: jason-swarelist@molenda.com
Cc: gdb-patches@sources.redhat.com, tromey@cygnus.com
Subject: Re: [RFA] Version 2 of patch to add 'maint profile-gdb' command
Date: Sun, 16 Sep 2001 22:36:00 -0000
Message-id: <200109170536.HAA21988@is.elta.co.il>
References: <20010910003022.A21681@shell17.ba.best.com> <3B9CE0C6.5060700@cygnus.com> <20010910115244.A25119@shell17.ba.best.com> <3B9FACDA.6070407@cygnus.com> <20010916174433.A3258@shell17.ba.best.com>
X-SW-Source: 2001-09/msg00204.html
Content-length: 1311

> Date: Sun, 16 Sep 2001 17:44:33 -0700
> From: Jason Molenda <jason-swarelist@molenda.com>
> 
> The one notable change I did not incorporate was compiling with
> -pg, linking without -pg, and using monstartup() to enable the
> profiling.  I have two problems with this - one, I _suspect_ (but
> I do not know for certain) that linking without -pg and just calling
> monstartup() is going to be unportable.  The second problem is
> that, off-hand, I don't know how a program should determine the
> start and end addresses of its TEXT section portably (which are
> required to use monstartup).

Emacs uses monstartup and manages to solve both of these problems.
See the file src/emacs.c (search for "etext") for the details.

> diff -u -p -r1.39 gdbint.texinfo
> --- doc/gdbint.texinfo	8 Sep 2001 10:53:45 -0000	1.39
> +++ doc/gdbint.texinfo	17 Sep 2001 00:34:26 -0000

I'd like the documentation of the "maint profile-gdb" command to be in
gdb.texinfo, not gdbint.texinfo.  The latter doesn't document user
commands, and users might not readily know to look in gdbint.  (It's
okay to have it described in both manuals, if you think it will be
useful.)

> +
> +@section Profiling GDB

Please replace "GDB" with "@value{GDBN}" everywhere.

Other than the last two comments, the patches to the docs are
approved.