Bug 22249 - objdump --dwarf-start can be very slow
Summary: objdump --dwarf-start can be very slow
Status: ASSIGNED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Nick Clifton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-04 15:08 UTC by Tom Tromey
Modified: 2024-02-29 07:52 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Tromey 2017-10-04 15:08:21 UTC
I have a very large debuginfo file.
I'm trying to examine a CU somewhere in the middle to see if there
is a bug in some debuginfo generation.
(See https://github.com/rust-lang/rust/issues/44412 for the gory details)

I wanted to use objdump --dwarf-start, but this is very slow in this
case.

It could be a bit more intelligent by noticing when the start offset
doesn't appear in the current CU -- it could just skip to the next CU.
This would be much faster.
Comment 1 Tom Tromey 2017-10-06 14:49:39 UTC
I think it's as easy as:

diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index 7ded1bf..f9c93ae 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -2666,6 +2666,13 @@ process_debug_info (struct dwarf_section *section,
 	  SAFE_BYTE_GET_AND_INC (type_offset, hdrptr, offset_size, end);
 	}
 
+      if (dwarf_start_die > (cu_offset + compunit.cu_length
+			     + initial_length_size))
+	{
+	  start = section_begin + cu_offset + compunit.cu_length + initial_length_size;
+	  continue;
+	}
+
       if ((do_loc || do_debug_loc || do_debug_ranges)
 	  && num_debug_info_entries == 0
 	  && ! do_types)
Comment 2 Nick Clifton 2017-10-09 09:38:58 UTC
Hi Tom,

  Thanks for suggesting a patch.  There are a couple of potential problems however:

  * According to the documentation the --dwarf-start=N command line option (which sets the dwarf_start_die variable) specifies the *number* of the DIE at which output should start, not the starting address referenced by the DIE.  

  I think that this may be a mistake in the documentation however, as this
does not appear to match the behaviour of the code.  Either that, or the documentation is correct and the code is wrong.  From the way that the documentation is written however, it would appear that the code may be at fault.  Ie I think that the author's intention was that --dwarf-start would specify a starting depth for DIE printing and --dwarf-depth would specify a maximum depth for DIE printing.  If that is correct, then the code needs to be fixed, and maybe we should consider adding a new option, eg --dwarf-starting-offset=N to specify the starting address.

  * For completeness sake if nothing else, shouldn't we also be able to specify an end address for CU dumping ?

  What do you think ?

Cheers
  Nick
Comment 3 Mark Wielaard 2017-10-09 09:50:21 UTC
(In reply to Nick Clifton from comment #2)
>   * According to the documentation the --dwarf-start=N command line option
> (which sets the dwarf_start_die variable) specifies the *number* of the DIE
> at which output should start, not the starting address referenced by the
> DIE.  
> 
>   I think that this may be a mistake in the documentation however, as this
> does not appear to match the behaviour of the code.  Either that, or the
> documentation is correct and the code is wrong.  From the way that the
> documentation is written however, it would appear that the code may be at
> fault.

I made the same observations when a request was made to add similar options to eu-readelf. There are a couple of other odd corner case behaviours too:
https://sourceware.org/bugzilla/show_bug.cgi?id=22250#c1
I think the documentation could certainly be improved/corrected.
Making it stop at the end of the CU also seems like a good improvement.
Comment 4 Tom Tromey 2017-10-09 14:55:23 UTC
(In reply to Nick Clifton from comment #2)

>   * According to the documentation the --dwarf-start=N command line option
> (which sets the dwarf_start_die variable) specifies the *number* of the DIE
> at which output should start, not the starting address referenced by the
> DIE.  
> 
>   I think that this may be a mistake in the documentation however, as this
> does not appear to match the behaviour of the code.

It is, because the intent (I added this feature initially) was to pass in the
DIE offset -- I suppose I used the term "number" because DIEs are always referred
to by offset, so I didn't consider any possible confusion here.

> fault.  Ie I think that the author's intention was that --dwarf-start would
> specify a starting depth for DIE printing

No, that's definitely not what I intended.

>   * For completeness sake if nothing else, shouldn't we also be able to
> specify an end address for CU dumping ?

There are really two cases here.

One is dumping the CU headers but no DIEs.  That's --dwarf-depth=0.
In this case you want to dump everything.

The other is expanding some DIE.  In this case, --dwarf-depth=N --dwarf-start=DIE
is used; the printing stops when the next DIE to be printed would have
a depth below N.  You can see this in action with the Emacs mode -- if it did not
stop, you'd get the whole CU inserted when expanding a "...".

FWIW I've sent this and other fixes to the list.
Comment 5 Sourceware Commits 2017-10-10 12:39:16 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ae7e78255666733d238e676a0bab14986a1483dd

commit ae7e78255666733d238e676a0bab14986a1483dd
Author: Nick Clifton <nickc@redhat.com>
Date:   Tue Oct 10 13:37:58 2017 +0100

    Improve the speed of the --dwarf-start option by skipping processing of any comp unit that ends before the specified start address.
    
    	PR 22249
    	* dwarf.c (process_debug_info): Skip any comp unit that ends
    	before dwarf_start_die.
Comment 6 Nick Clifton 2017-10-10 12:40:27 UTC
Hi Tom,

  OK, well your patch is fine, so I have checked that in.  I still think that we need to improve the documentation however.  Do you have any suggestions ?

Cheers
  Nick
Comment 7 Tom Tromey 2017-10-10 14:05:43 UTC
(In reply to Nick Clifton from comment #6)
> Hi Tom,
> 
>   OK, well your patch is fine, so I have checked that in.  I still think
> that we need to improve the documentation however.  Do you have any
> suggestions ?

I will look, but I'd appreciate it if you could look at the dwarf-mode.el
patches that I also posted to the list.  Alan had already approved
this dwarf.c patch; but I wanted to push them all together, because
the whole reason I wrote this change is to work in conjunction with the
mode changes so I can actually dig into this problem I'm having...
Thanks.
Comment 8 Nick Clifton 2017-10-10 14:12:26 UTC
Hi Tom,

> https://sourceware.org/bugzilla/show_bug.cgi?id=22249

> I will look, but I'd appreciate it if you could look at the dwarf-mode.el
> patches that I also posted to the list.  Alan had already approved
> this dwarf.c patch; but I wanted to push them all together, because
> the whole reason I wrote this change is to work in conjunction with the
> mode changes so I can actually dig into this problem I'm having...

Sure - I will take a look, but please could you point me at the email ?

Cheers
  Nick
Comment 9 Tom Tromey 2017-10-10 14:34:30 UTC
Thread starts here:

https://sourceware.org/ml/binutils/2017-10/msg00082.html
Comment 10 Nick Clifton 2017-10-10 15:37:41 UTC
Hi Tom,

> Thread starts here:
> 
> https://sourceware.org/ml/binutils/2017-10/msg00082.html

All the changes look fine to me, apart from one.  Patch 2/5
has:

+(defvar dwarf-mode-map
+  (let ((map (make-sparse-keymap)))
+    (set-keymap-parent special-mode-map)
+    (define-key map [(control ?m)] #'dwarf-insert-substructure)
+    map)
+  "Keymap for dwarf-mode buffers.")

But when I tried to byte compile this function (using emacs 25.3.1) 
there was this warning message:

  dwarf-mode.el:186:6:Warning: set-keymap-parent called with 1 argument, but
    requires 2

I am not an emacs expert, but maybe this could be a problem ?

Actually - on a related note - would you be interested in becoming
the official maintainer for dwarf-mode.el ?  Then you could make
changes to the file without having to prod slow pokes like me. :-)

Cheers
  Nick
Comment 11 Tom Tromey 2017-10-10 19:00:29 UTC
(In reply to Nick Clifton from comment #10)

>   dwarf-mode.el:186:6:Warning: set-keymap-parent called with 1 argument, but
>     requires 2
> 
> I am not an emacs expert, but maybe this could be a problem ?

Yeah.  I wonder why I didn't see this.
I've fixed it.

> Actually - on a related note - would you be interested in becoming
> the official maintainer for dwarf-mode.el ? 

That would be great.
Comment 12 Mark Wielaard 2017-10-11 12:13:08 UTC
(In reply to Tom Tromey from comment #4)
> (In reply to Nick Clifton from comment #2)
> >   I think that this may be a mistake in the documentation however, as this
> > does not appear to match the behaviour of the code.
> 
> It is, because the intent (I added this feature initially) was to pass in the
> DIE offset -- I suppose I used the term "number" because DIEs are always
> referred to by offset, so I didn't consider any possible confusion here.

Not every offset is a DIE "number". There is some slight confusion what happens if the offset is in the middle of a DIE, should it back up and print that one or should it start printing at the first offset after that is a DIE "number"?
Comment 13 Mark Wielaard 2017-10-11 12:24:35 UTC
(In reply to Tom Tromey from comment #4)
> (In reply to Nick Clifton from comment #2)
> >   * For completeness sake if nothing else, shouldn't we also be able to
> > specify an end address for CU dumping ?
> 
> There are really two cases here.
> 
> One is dumping the CU headers but no DIEs.  That's --dwarf-depth=0.
> In this case you want to dump everything.

IMHO it is totally confusing that printing CU headers or not is specified with --dwarf-depth=0. Why isn't this a separate option?

> The other is expanding some DIE.  In this case, --dwarf-depth=N
> --dwarf-start=DIE is used;
> the printing stops when the next DIE to be printed would have
> a depth below N.  You can see this in action with the Emacs mode -- if it
> did not stop, you'd get the whole CU inserted when expanding a "...".

This seems somewhat user unfriendly.
How is one supposed to pick the "correct" N?
In the case that --dwarf-start=DIE is given, but --dwarf-depth=N is not,
it really should default to --dwarf-depth=DIE-depth. Otherwise you just
keep dumping completely unrelated DIEs.