[PATCH] Whack some dead code

[PATCH] Whack some dead code

[Stan Shebs]

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]

A while back, Vladimir Prus noticed a chunk of dead code in 
trace_find_line_command; basically, if you don't have symbolic info, 
there is no plausible fallback for line numbers, you just have to give 
up.  Committed to trunk.

Stan

2010-03-29  Stan Shebs  <stan@codesourcery.com>

    * tracepoint.c (trace_find_line_command): Remove dead code.



[-- Attachment #2: deadcode-patch-1 --]
[-- Type: text/plain, Size: 1719 bytes --]

Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.163
retrieving revision 1.164
diff -p -r1.163 -r1.164
*** tracepoint.c	29 Mar 2010 23:45:06 -0000	1.163
--- tracepoint.c	30 Mar 2010 00:19:43 -0000	1.164
*************** trace_find_line_command (char *args, int
*** 2043,2075 ****
        sals.sals[0] = sal;
      }
    else
!       {
        sals = decode_line_spec (args, 1);
        sal = sals.sals[0];
      }
    
    old_chain = make_cleanup (xfree, sals.sals);
    if (sal.symtab == 0)
!     {
!       printf_filtered ("TFIND: No line number information available");
!       if (sal.pc != 0)
! 	{
! 	  /* This is useful for "info line *0x7f34".  If we can't
! 	     tell the user about a source line, at least let them
! 	     have the symbolic address.  */
! 	  printf_filtered (" for address ");
! 	  wrap_here ("  ");
! 	  print_address (get_current_arch (), sal.pc, gdb_stdout);
! 	  printf_filtered (";\n -- will attempt to find by PC. \n");
!   	}
!         else
!   	{
! 	  printf_filtered (".\n");
! 	  return;		/* No line, no PC; what can we do?  */
!   	}
!     }
!   else if (sal.line > 0
! 	   && find_line_pc_range (sal, &start_pc, &end_pc))
      {
        if (start_pc == end_pc)
    	{
--- 2043,2058 ----
        sals.sals[0] = sal;
      }
    else
!     {
        sals = decode_line_spec (args, 1);
        sal = sals.sals[0];
      }
    
    old_chain = make_cleanup (xfree, sals.sals);
    if (sal.symtab == 0)
!     error (_("No line number information available."));
! 
!   if (sal.line > 0 && find_line_pc_range (sal, &start_pc, &end_pc))
      {
        if (start_pc == end_pc)
    	{

Re: [PATCH] Whack some dead code

[Pedro Alves]

On Tuesday 30 March 2010 01:25:11, Stan Shebs wrote:
> A while back, Vladimir Prus noticed a chunk of dead code in 
> trace_find_line_command; basically, if you don't have symbolic info, 
> there is no plausible fallback for line numbers, you just have to give 
> up.  Committed to trunk.

>     * tracepoint.c (trace_find_line_command): Remove dead code.

Errr, what does "dead" mean exactly?  It's certainly exercisable, just
like the comment in the code suggests:

 (top-gdb) help tfind line
 Select a trace frame by source line.
 Argument can be a line number (with optional source file),
 a function name, or '*' followed by an address.
 Default argument is 'the next source line that was traced'.

 (top-gdb) frame
 #0  0x00007ffff6b420a0 in memset () from /lib/libc.so.6

 (top-gdb) tfind  line *(long)$rip
 TFIND: No line number information available for address 0x7ffff6b420a0 <memset>;
  -- will attempt to find by PC.
 Found trace frame 0, tracepoint -1
 #0  0x00007ffff6b420a0 in memset () from /lib/libc.so.6

( nevermind the "found some trace frame" bug :-) )

>     if (sal.symtab == 0)
> !     {
> !       printf_filtered ("TFIND: No line number information available");
> !       if (sal.pc != 0)
> !       {
> !         /* This is useful for "info line *0x7f34".  If we can't
> !            tell the user about a source line, at least let them
> !            have the symbolic address.  */
> !         printf_filtered (" for address ");
> !         wrap_here ("  ");
> !         print_address (get_current_arch (), sal.pc, gdb_stdout);
> !         printf_filtered (";\n -- will attempt to find by PC. \n");
> !       }
> !         else
> !       {
> !         printf_filtered (".\n");
> !         return;               /* No line, no PC; what can we do?  */
> !       }
> !     }

This somewhat matches "info line" (and this code seems to have been
copied from there):

 (top-gdb) info line *(long)$rip
 No line number information available for address 0x7ffff6b420a0 <memset>

Now, if the "-- will attempt to find by PC." fallback, or even just
the the "at least let them have the symbolic address" bit
aren't useful, that's another story.

-- 
Pedro Alves

Re: [PATCH] Whack some dead code

[Stan Shebs]

Pedro Alves wrote:
> On Tuesday 30 March 2010 01:25:11, Stan Shebs wrote:
>   
>> A while back, Vladimir Prus noticed a chunk of dead code in 
>> trace_find_line_command; basically, if you don't have symbolic info, 
>> there is no plausible fallback for line numbers, you just have to give 
>> up.  Committed to trunk.
>>     
>
>   
>>     * tracepoint.c (trace_find_line_command): Remove dead code.
>>     
>
> Errr, what does "dead" mean exactly?  It's certainly exercisable, just
> like the comment in the code suggests:
>
>  (top-gdb) help tfind line
>  Select a trace frame by source line.
>  Argument can be a line number (with optional source file),
>  a function name, or '*' followed by an address.
>  Default argument is 'the next source line that was traced'.
>   

A better term is maybe "bogus code", and my explanation is misleading.  
If you look at the locals start_pc and end_pc, you see that the original 
code's sal.symtab == 0 case does not set them before they get passed to 
tfind_1 at the bottom.  I suppose one could try to salvage the sal.pc != 
0 case and treat it as a start_pc == end_pc situation; we'd would want 
to think how each case is supposed to be useful.  It would be a little 
annoying if a typo in tfind takes you to an unexpected trace frame, 
which in turn could disable your display commands, etc.  "info line" is 
not necessarily the right model, since as an info command it has no side 
effects and can make far-out interpretations.

(Incidentally, I notice that "help info line" doesn't mention the raw 
address option. Didn't it used to??)

Stan

Re: [PATCH] Whack some dead code

[Pedro Alves]

On Tuesday 30 March 2010 18:05:14, Stan Shebs wrote:
> A better term is maybe "bogus code", and my explanation is misleading.  
> If you look at the locals start_pc and end_pc, you see that the original 
> code's sal.symtab == 0 case does not set them before they get passed to 
> tfind_1 at the bottom.  I suppose one could try to salvage the sal.pc != 
> 0 case and treat it as a start_pc == end_pc situation; we'd would want 
> to think how each case is supposed to be useful.  

Whoops, yeah, there was a bug here.  The intention was certainly for
treating sal.pc != 0 case as start_pc == end_pc.  I don't know how
useful that would be.  The " for address 0x....... <functname>" part
did looks a bit useful, but I won't cry over it being gone.

> It would be a little 
> annoying if a typo in tfind takes you to an unexpected trace frame, 
> which in turn could disable your display commands, etc.

Well, there was an obvious bug.  That remark applies to all bugs.  :-)

> (Incidentally, I notice that "help info line" doesn't mention the raw 
> address option. Didn't it used to??)

(Hmm.  I've tried gdb 5.3 (2002), and it doesn't, the help text
looks the same.  Don't have older builds handy.)

-- 
Pedro Alves

Re: [PATCH] Whack some dead code

[Michael Snyder]

Pedro Alves wrote:
> On Tuesday 30 March 2010 18:05:14, Stan Shebs wrote:

>> (Incidentally, I notice that "help info line" doesn't mention the raw 
>> address option. Didn't it used to??)
> 
> (Hmm.  I've tried gdb 5.3 (2002), and it doesn't, the help text
> looks the same.  Don't have older builds handy.)

I only ever remember discovering that feature by accident.  ;-)