Re: [PATCH] Install and generate docs for gdb-add-index

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Thursday, January 11 2018, Simon Marchi wrote:

> On 2018-01-11 04:35 PM, Sergio Durigan Junior wrote:
>> The "gdb-add-index" script has been resurrected on:
>> 
>>   commit caf26be91a584ef141ac5d3cb31007731af8b8e3
>>   Author: Samuel Bronson <naesten@gmail.com>
>>   Date:   Fri Nov 15 16:09:33 2013 -0500
>> 
>>       Resurrect gdb-add-index as a contrib script
>> 
>> However, for some reason (I couldn't find it in the archives), only
>> the script has been checked-in; the Makefile parts responsible for
>> installing it in the system were left out.  This commit fixes that, by
>> also resurrecting the Makefile and documentation bits.
>> 
>> This commit is part of our effort to upstream the local Fedora GDB
>> changes.  With this commit, we'll only carry a very small
>> Fedora-specific modification to the script.
>
> Woo thanks!
>
> I have some comments, but I'm just being picky.

Thanks for the review, Simon.

>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index 17b71c6e7c..d63220db4e 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -1770,6 +1770,15 @@ install-only: $(CONFIG_INSTALL)
>>  		  $(INSTALL_SCRIPT) gcore \
>>  			  $(DESTDIR)$(bindir)/$$transformed_name; \
>>  	fi
>> +	transformed_name=`t='$(program_transform_name)'; \
>> +			  echo gdb-add-index | sed -e "$$t"` ; \
>> +		if test "x$$transformed_name" = x; then \
>> +		  transformed_name=gdb-add-index ; \
>> +		else \
>> +		  true ; \
>> +		fi ; \
>> +		$(INSTALL_PROGRAM) $(srcdir)/contrib/gdb-add-index.sh \
>> +			$(DESTDIR)$(bindir)/$$transformed_name$(EXEEXT)
>
> I know this is just copy pasted from above, but doesn't the indentation look odd?
> Shouldn't the if/else/fi and the INSTALL_PROGRAM lines be indented with one tab
> less?

Ah, you're right.  Strange, I don't know why it is indented this way
originally.  Anyway, fixed it.

>>  	@$(MAKE) DO=install "DODIRS=$(SUBDIRS)" $(FLAGS_TO_PASS) subdir_do
>>  
>>  install-strip:
>> diff --git a/gdb/doc/Makefile.in b/gdb/doc/Makefile.in
>> index eafadee899..0323bc4ecb 100644
>> --- a/gdb/doc/Makefile.in
>> +++ b/gdb/doc/Makefile.in
>> @@ -174,7 +174,7 @@ POD2MAN5 = pod2man --center="GNU Development Tools" \
>>  		   --release="gdb-`sed q version.subst`" --section=5
>>  
>>  # List of man pages generated from gdb.texi
>> -MAN1S = gdb.1 gdbserver.1 gcore.1
>> +MAN1S = gdb.1 gdbserver.1 gcore.1 gdb-add-index.1
>>  MAN5S = gdbinit.5
>>  MANS = $(MAN1S) $(MAN5S)
>>  
>> @@ -633,6 +633,13 @@ gcore.1: $(GDB_DOC_FILES)
>>  		mv -f $@.T$$$$ $@) || (rm -f $@.T$$$$ && exit 1)
>>  	rm -f gcore.pod
>>  
>> +gdb-add-index.1: $(GDB_DOC_FILES)
>> +	touch $@
>> +	-$(TEXI2POD) $(MANCONF) -Dgdb-add-index < $(srcdir)/gdb.texinfo > gdb-add-index.pod
>> +	-($(POD2MAN1) gdb-add-index.pod | sed -e '/^.if n .na/d' > $@.T$$$$ && \
>> +		mv -f $@.T$$$$ $@) || (rm -f $@.T$$$$ && exit 1)
>> +	rm -f gdb-add-index.pod
>> +
>>  gdbinit.5: $(GDB_DOC_FILES)
>>  	touch $@
>>  	-$(TEXI2POD) $(MANCONF) -Dgdbinit < $(srcdir)/gdb.texinfo > gdbinit.pod
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index dba2fa766a..62fcdd524e 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -19775,6 +19775,14 @@ There are currently some limitation on indices.  They only work when
>>  for DWARF debugging information, not stabs.  And, they do not
>>  currently work for programs using Ada.
>>  
>> +@value{GDBN} comes with a program, @command{gdb-add-index}, which can
>> +be used to add the index to a symbol file.  It takes the symbol file
>> +as its only argument:
>> +
>> +@smallexample
>> +$ gdb-add-index symfile
>> +@end smallexample
>> +
>
> Would it be good to put the part about gdb-add-index earlier in the page/section,
> for example just after the first paragraph?  The doc starts by telling you the
> manual method with the scary objcopy lines and all.  I think it would be nice if
> it started by telling: here's the easy and recommended way, and then here are all
> the gory details about how it's done internally, if you need it.  I could imagine
> users starting to read the section, and stopping midway because they think it's
> too complicated/not worth it, and never get to the gdb-add-index part.

That's a good point.  I moved the text up (after the first paragraph, as
suggested) and modified it a bit:

  ...
  @node Index Files
  @section Index Files Speed Up @value{GDBN}
  @cindex index files
  @cindex @samp{.gdb_index} section

  When @value{GDBN} finds a symbol file, it scans the symbols in the
  file in order to construct an internal symbol table.  This lets most
  @value{GDBN} operations work quickly---at the cost of a delay early
  on.  For large programs, this delay can be quite lengthy, so
  @value{GDBN} provides a way to build an index, which speeds up
  startup.

  For convenience, @value{GDBN} comes with a program,
  @command{gdb-add-index}, which can be used to add the index to a
  symbol file.  It takes the symbol file as its only argument:

  @smallexample
  $ gdb-add-index symfile
  @end smallexample

  It is also possible to do the work manually.  Here is what
  @command{gdb-add-index} does behind the curtains.
  ...

I will submit the full patch as a v2.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/