Summary: | objdump --dwarf-start can be very slow | ||
---|---|---|---|
Product: | binutils | Reporter: | Tom Tromey <tromey> |
Component: | binutils | Assignee: | Nick Clifton <nickc> |
Status: | ASSIGNED --- | ||
Severity: | normal | CC: | mark |
Priority: | P2 | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Host: | Target: | ||
Build: | Last reconfirmed: |
Description
Tom Tromey
2017-10-04 15:08:21 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) 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 (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. (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. 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. 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 (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. 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 Thread starts here: https://sourceware.org/ml/binutils/2017-10/msg00082.html 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
(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. (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"? (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. |