FYI: copy/paste error in sol-thread.c ?

FYI: copy/paste error in sol-thread.c ?

[Joel Brobecker]

... while working on fixing the few compilation errors in that file,
I noticed:

      /* If thread is asleep, print function that went to sleep.  */
      if (ti.ti_state == TD_THR_SLEEP)
        {
          const struct bound_minimal_symbol msym
            = lookup_minimal_symbol_by_pc (ti.ti_pc);

          if (msym.minsym)
            printf_filtered (" - Sleep func: %s\n",
                             SYMBOL_PRINT_NAME (msym.minsym));
          else
            printf_filtered (" - Sleep func: %s\n",
                             paddress (target_gdbarch (), ti.ti_startfunc));
                                                          ^^^^^^^^^^^^^^^
                                                          |||||||||||||||
        }

I find the use of ti.ti_startfunc looks suspiciously like a copy/paste
of the code right above:

      if (ti.ti_startfunc != 0)
        {
          const struct bound_minimal_symbol msym
            = lookup_minimal_symbol_by_pc (ti.ti_startfunc);

          if (msym.minsym)
            printf_filtered ("   startfunc: %s\n",
                             SYMBOL_PRINT_NAME (msym.minsym));
          else
            printf_filtered ("   startfunc: %s\n",
                             paddress (target_gdbarch (), ti.ti_startfunc));
        }

In particular, the first block ("if (ti.ti_state == TD_THR_SLEEP)")
does a function name lookup using ti.ti_pc, not ti.ti_startfunc.

I am running against the clock today, but will try to look at it
again sometime this week, if I have a bit of time. A second pair
of eyes to confirm my understanding would be greatly appreciated.

-- 
Joel

Re: FYI: copy/paste error in sol-thread.c ?

[Pedro Alves]

On 04/30/2013 12:09 PM, Joel Brobecker wrote:
> ... while working on fixing the few compilation errors in that file,
> I noticed:
> 
>       /* If thread is asleep, print function that went to sleep.  */
>       if (ti.ti_state == TD_THR_SLEEP)
>         {
>           const struct bound_minimal_symbol msym
>             = lookup_minimal_symbol_by_pc (ti.ti_pc);
> 
>           if (msym.minsym)
>             printf_filtered (" - Sleep func: %s\n",
>                              SYMBOL_PRINT_NAME (msym.minsym));
>           else
>             printf_filtered (" - Sleep func: %s\n",
>                              paddress (target_gdbarch (), ti.ti_startfunc));
>                                                           ^^^^^^^^^^^^^^^
>                                                           |||||||||||||||
>         }
> 
> I find the use of ti.ti_startfunc looks suspiciously like a copy/paste
> of the code right above:

I agree it looks like a copy/paste.

Very very very minor, but the prints look a little odd to me:

>             printf_filtered ("   startfunc: %s\n",
>             printf_filtered (" - Sleep func: %s\n"

The first is lowercase, and doesn't start with '-', while the
other  line is uppercase, and has a '-'...  Looks odd.
It reads to me as if the original intention was to put this all
in the same line, but then the startfunc print includes \n,
breaking it...

-- 
Pedro Alves

[RFA 2/3] Improve output of "maintenance info sol-threads" command.

[Joel Brobecker]

This patch does the following:
  - Puts the startfunc and "Sleep func" info on the same line;
  - Renames "Sleep func" into "sleepfunc" to be consistent with
    "startfunc"
  - Avoids the use of a '-' as a separate before the "sleepfunc"
    output, because the '-' looks odd and out of place when the
    "startfunc" field is not printed (ti.ti_startfunc is nul).
  - Use a '=' instead of ':' and avoids the space between
    the name of the value and its value, mostly to help group
    the value with its name.

gdb/ChangeLog:

        * sol-thread.c (info_cb): Rework the output of the "maintenance
        info sol-threads" command a bit.

I could probably commit this one without approval, but someone else's
thoughts would be welcome, since this is a bit cosmetic.

---
 gdb/sol-thread.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index 2e76700..fb89723 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -1177,10 +1177,10 @@ info_cb (const td_thrhandle_t *th, void *s)
 	    = lookup_minimal_symbol_by_pc (ti.ti_startfunc);
 
 	  if (msym.minsym)
-	    printf_filtered ("   startfunc: %s\n",
+	    printf_filtered ("   startfunc=%s",
 			     SYMBOL_PRINT_NAME (msym.minsym));
 	  else
-	    printf_filtered ("   startfunc: %s\n",
+	    printf_filtered ("   startfunc=%s",
 			     paddress (target_gdbarch (), ti.ti_startfunc));
 	}
 
@@ -1191,16 +1191,14 @@ info_cb (const td_thrhandle_t *th, void *s)
 	    = lookup_minimal_symbol_by_pc (ti.ti_pc);
 
 	  if (msym.minsym)
-	    printf_filtered (" - Sleep func: %s\n",
+	    printf_filtered ("   sleepfunc=%s",
 			     SYMBOL_PRINT_NAME (msym.minsym));
 	  else
-	    printf_filtered (" - Sleep func: %s\n",
+	    printf_filtered ("   sleepfunc=%s",
 			     paddress (target_gdbarch (), ti.ti_pc));
 	}
 
-      /* Wrap up line, if necessary.  */
-      if (ti.ti_state != TD_THR_SLEEP && ti.ti_startfunc == 0)
-	printf_filtered ("\n");	/* don't you hate counting newlines?  */
+      printf_filtered ("\n");
     }
   else
     warning (_("info sol-thread: failed to get info for thread."));
-- 
1.7.0.4

Re: [RFA 2/3] Improve output of "maintenance info sol-threads" command.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel>         * sol-thread.c (info_cb): Rework the output of the "maintenance
Joel>         info sol-threads" command a bit.

Joel> I could probably commit this one without approval, but someone else's
Joel> thoughts would be welcome, since this is a bit cosmetic.

FWIW it seems fine to me.

Tom

Re: [RFA 2/3] Improve output of "maintenance info sol-threads" command.

[Pedro Alves]

On 05/07/2013 01:25 PM, Joel Brobecker wrote:
> This patch does the following:
>   - Puts the startfunc and "Sleep func" info on the same line;
>   - Renames "Sleep func" into "sleepfunc" to be consistent with
>     "startfunc"
>   - Avoids the use of a '-' as a separate before the "sleepfunc"
>     output, because the '-' looks odd and out of place when the
>     "startfunc" field is not printed (ti.ti_startfunc is nul).
>   - Use a '=' instead of ':' and avoids the space between
>     the name of the value and its value, mostly to help group
>     the value with its name.

I'm fine with what I'm imagining it looks like (or whatever, really),
but could you give an example of how things now look after the
patch, for the archives/log?  An output paste is worth a thousand words.  :-)

Thanks!
-- 
Pedro Alves

Re: [RFA 2/3] Improve output of "maintenance info sol-threads" command.

[Joel Brobecker]

> I'm fine with what I'm imagining it looks like (or whatever, really),
> but could you give an example of how things now look after the
> patch, for the archives/log?  An output paste is worth a thousand words.  :-)

Of course. I actually had done a special session while composing
the message in order to send that output, and then of coursed worked
on something else in parallel and promptly forgot :-(.

Here it is (also part of the commit message, for easy access):

    (gdb) maintenance info sol-threads
    user   thread #1, lwp 1, (active)
    system thread #2, lwp 2, (active)
    system thread #3, lwp 0, (asleep)    sleepfunc=0xff32d9e0
    user   thread #4, lwp 4, (asleep)    startfunc=[...].task_wrapper   sleepfunc=0xff3290f0
    system thread #5, lwp 7, (active)    startfunc=_co_timerset
    user   thread #6, lwp 8, (active)    startfunc=[...].task_wrapper

-- 
Joel

Checked in: [RFA 2/3] Improve output of "maintenance info sol-threads" command.

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * sol-thread.c (info_cb): Rework the output of the "maintenance
>         info sol-threads" command a bit.

Checked in.

-- 
Joel

[RFA/commit 1/3] ti.ti_pc vs ti.ti_startfunc copy-pasto in sol-thread.c:info_cb

[Joel Brobecker]

gdb/ChangeLog:

        * sol-thread.c (info_cb) [ti.ti_state == TD_THR_SLEEP]:
        Replace ti.ti_startfunc by ti.ti_pc.

Probably OK to commit without approval, so will likely do so in the
next few days.

---
 gdb/sol-thread.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index 6202997..2e76700 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -1195,7 +1195,7 @@ info_cb (const td_thrhandle_t *th, void *s)
 			     SYMBOL_PRINT_NAME (msym.minsym));
 	  else
 	    printf_filtered (" - Sleep func: %s\n",
-			     paddress (target_gdbarch (), ti.ti_startfunc));
+			     paddress (target_gdbarch (), ti.ti_pc));
 	}
 
       /* Wrap up line, if necessary.  */
-- 
1.7.0.4

Re: [RFA/commit 1/3] ti.ti_pc vs ti.ti_startfunc copy-pasto in sol-thread.c:info_cb

[Pedro Alves]

On 05/07/2013 01:25 PM, Joel Brobecker wrote:
> gdb/ChangeLog:
> 
>         * sol-thread.c (info_cb) [ti.ti_state == TD_THR_SLEEP]:
>         Replace ti.ti_startfunc by ti.ti_pc.
> 
> Probably OK to commit without approval, so will likely do so in the
> next few days.

Looks okay to me, but that could be inferred from the previous
discussion.  :-)

-- 
Pedro Alves

Checked in: [RFA/commit 1/3] ti.ti_pc vs ti.ti_startfunc copy-pasto in sol-thread.c:info_cb

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * sol-thread.c (info_cb) [ti.ti_state == TD_THR_SLEEP]:
>         Replace ti.ti_startfunc by ti.ti_pc.

Checked in.

-- 
Joel

[RFA 3/3] Avoid duplicating text in sol-thread.c:info_cb

[Joel Brobecker]

This is a minor cleanup that helps avoiding duplicating the output
of a command when handling alternative situations.

gdb/ChangeLog:

        * sol-thread.c (info_cb): Factorize the code a little.

Same as patch #2, I could probably self-approve, but there could be
merit in the current approach if the output was thought to one day
become different depending on some (future) factors...

---
 gdb/sol-thread.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/gdb/sol-thread.c b/gdb/sol-thread.c
index fb89723..a17a55b 100644
--- a/gdb/sol-thread.c
+++ b/gdb/sol-thread.c
@@ -1176,12 +1176,10 @@ info_cb (const td_thrhandle_t *th, void *s)
 	  const struct bound_minimal_symbol msym
 	    = lookup_minimal_symbol_by_pc (ti.ti_startfunc);
 
-	  if (msym.minsym)
-	    printf_filtered ("   startfunc=%s",
-			     SYMBOL_PRINT_NAME (msym.minsym));
-	  else
-	    printf_filtered ("   startfunc=%s",
-			     paddress (target_gdbarch (), ti.ti_startfunc));
+	  printf_filtered ("   startfunc=%s",
+			   msym.minsym
+			   ? SYMBOL_PRINT_NAME (msym.minsym)
+			   : paddress (target_gdbarch (), ti.ti_startfunc));
 	}
 
       /* If thread is asleep, print function that went to sleep.  */
@@ -1190,12 +1188,10 @@ info_cb (const td_thrhandle_t *th, void *s)
 	  const struct bound_minimal_symbol msym
 	    = lookup_minimal_symbol_by_pc (ti.ti_pc);
 
-	  if (msym.minsym)
-	    printf_filtered ("   sleepfunc=%s",
-			     SYMBOL_PRINT_NAME (msym.minsym));
-	  else
-	    printf_filtered ("   sleepfunc=%s",
-			     paddress (target_gdbarch (), ti.ti_pc));
+	  printf_filtered ("   sleepfunc=%s",
+			   msym.minsym
+			   ? SYMBOL_PRINT_NAME (msym.minsym)
+			   : paddress (target_gdbarch (), ti.ti_pc));
 	}
 
       printf_filtered ("\n");
-- 
1.7.0.4

Re: [RFA 3/3] Avoid duplicating text in sol-thread.c:info_cb

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel>         * sol-thread.c (info_cb): Factorize the code a little.

Joel> Same as patch #2, I could probably self-approve, but there could be
Joel> merit in the current approach if the output was thought to one day
Joel> become different depending on some (future) factors...

I wouldn't worry about it.
It's trivial to change back if someone should need to.

Tom

Checked in: [RFA 3/3] Avoid duplicating text in sol-thread.c:info_cb

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * sol-thread.c (info_cb): Factorize the code a little.

Checked in.

-- 
Joel

Re: FYI: copy/paste error in sol-thread.c ?

[Joel Brobecker]

> I agree it looks like a copy/paste.

Ok, the first patch of this series fixes the code.

> Very very very minor, but the prints look a little odd to me:
> 
> >             printf_filtered ("   startfunc: %s\n",
> >             printf_filtered (" - Sleep func: %s\n"
> 
> The first is lowercase, and doesn't start with '-', while the
> other  line is uppercase, and has a '-'...  Looks odd.
> It reads to me as if the original intention was to put this all
> in the same line, but then the startfunc print includes \n,
> breaking it...

You are right. This is a copy of the current output:

| (gdb) maintenance info sol-threads
| user   thread #1, lwp 1, (active)
| system thread #2, lwp 2, (active)
| system thread #3, lwp 0, (asleep)  - Sleep func: 0xff32d9e0
| user   thread #4, lwp 4, (asleep)    startfunc: system.tasking.stages.task_wrapper
|  - Sleep func: 0xff3290f0
|  system thread #5, lwp 7, (active)    startfunc: _co_timerset

Besides the lowercase/uppercase thing, two things are odd:
  - the '-' for thread #3
  - the fact that "sleep func" gets printed on the next line
    for thread #4, with an indentation that makes it hard to read
    the info.

It seems to me also that the initial intention was probably to have
everything on the same line, and that seems fine to me, as this is
only a maintenance command. So patch #2 makes a number of adjustments
to the formatting of the output.  In particular, for simplicity,
I just got rid of the '-' altogether, and to compensate for the
slight reduction in visual separation, I grouped the label and
the value a little more tightly.

And to boot, I wasn't very fond of the fact that the code printing
the info was duplicated, so I factorized it a bit. This is also
potentially a matter of taste - so I did not commit any of the patches
yet.

Tested on sparc-solaris by running the "maintenance info sol-threads"
command by hand. That's probably as much time as I am willing to
spend on this, and I think that's going to be sufficient anyway.