[v2,2/7] Add "help news"

Message ID 20200623132006.15863-3-tom@tromey.com
State New
Headers show
Series
  • Some user-friendliness changes
Related show

Commit Message

Tom Tromey June 23, 2020, 1:20 p.m.
This adds a "help news" subcommand, which simply dumps the NEWS file.
The NEWS file is now installed.

gdb/ChangeLog
2020-06-22  Tom Tromey  <tom@tromey.com>

	* NEWS: Add entry.
	* data-directory/Makefile.in (GDB_FILES): New variable.
	(all): Add gdb-files.
	(gdb-files, install-gdb-files, uninstall-gdb-files): New targets.
	(install-only, uninstall): Update.
	* cli/cli-decode.c (help_news): New file.
	(help_cmd): Handle "news".
	(help_list): Mention "help news".

gdb/doc/ChangeLog
2020-06-22  Tom Tromey  <tom@tromey.com>

	* gdb.texinfo (Help): Mention help news.
---
 gdb/ChangeLog                  | 11 +++++++++
 gdb/NEWS                       |  3 +++
 gdb/cli/cli-decode.c           | 41 ++++++++++++++++++++++++++++++++++
 gdb/data-directory/Makefile.in | 28 ++++++++++++++++++++---
 gdb/doc/ChangeLog              |  4 ++++
 gdb/doc/gdb.texinfo            |  4 ++++
 6 files changed, 88 insertions(+), 3 deletions(-)

-- 
2.17.2

Comments

Eli Zaretskii June 23, 2020, 2:35 p.m. | #1
> From: Tom Tromey <tom@tromey.com>

> Date: Tue, 23 Jun 2020 07:20:01 -0600

> Cc: Tom Tromey <tom@tromey.com>

> 

> This adds a "help news" subcommand, which simply dumps the NEWS file.

> The NEWS file is now installed.

> 

> gdb/ChangeLog

> 2020-06-22  Tom Tromey  <tom@tromey.com>

> 

> 	* NEWS: Add entry.

> 	* data-directory/Makefile.in (GDB_FILES): New variable.

> 	(all): Add gdb-files.

> 	(gdb-files, install-gdb-files, uninstall-gdb-files): New targets.

> 	(install-only, uninstall): Update.

> 	* cli/cli-decode.c (help_news): New file.

> 	(help_cmd): Handle "news".

> 	(help_list): Mention "help news".

> 

> gdb/doc/ChangeLog

> 2020-06-22  Tom Tromey  <tom@tromey.com>

> 

> 	* gdb.texinfo (Help): Mention help news.

> ---

>  gdb/ChangeLog                  | 11 +++++++++

>  gdb/NEWS                       |  3 +++

>  gdb/cli/cli-decode.c           | 41 ++++++++++++++++++++++++++++++++++

>  gdb/data-directory/Makefile.in | 28 ++++++++++++++++++++---

>  gdb/doc/ChangeLog              |  4 ++++

>  gdb/doc/gdb.texinfo            |  4 ++++

>  6 files changed, 88 insertions(+), 3 deletions(-)


OK for the documentation parts.

Thanks.
Jose E. Marchesi via Gdb-patches June 23, 2020, 6:18 p.m. | #2
On Tue, Jun 23, 2020 at 8:20 AM Tom Tromey <tom@tromey.com> wrote:
> +static void

> +help_news (struct ui_file *stream)

> +{

> +  std::string news_name = std::string (gdb_datadir) + SLASH_STRING + "NEWS";

> +  gdb_file_up news_file = gdb_fopen_cloexec (news_name.c_str (), "r");

> +  if (news_file == nullptr)

> +    perror_with_name (_("could not open the NEWS file"));

> +

> +  char buffer[1024];

> +  size_t offset = 0;

> +  while (true)

> +    {

> +      size_t nbytes = fread (&buffer[offset], 1, sizeof (buffer) - offset,

> +                            news_file.get ());



Why not use read_entire_file from your other patch?

Christian
Tom Tromey July 5, 2020, 3:59 p.m. | #3
>>>>> "Christian" == Christian Biesinger via Gdb-patches <gdb-patches@sourceware.org> writes:


Christian> On Tue, Jun 23, 2020 at 8:20 AM Tom Tromey <tom@tromey.com> wrote:
>> +static void

>> +help_news (struct ui_file *stream)

>> +{

>> +  std::string news_name = std::string (gdb_datadir) + SLASH_STRING + "NEWS";

>> +  gdb_file_up news_file = gdb_fopen_cloexec (news_name.c_str (), "r");

>> +  if (news_file == nullptr)

>> +    perror_with_name (_("could not open the NEWS file"));

>> +

>> +  char buffer[1024];

>> +  size_t offset = 0;

>> +  while (true)

>> +    {

>> +      size_t nbytes = fread (&buffer[offset], 1, sizeof (buffer) - offset,

>> +                            news_file.get ());



Christian> Why not use read_entire_file from your other patch?

The NEWS file seems large-ish, and I figured the normal thing for users
would be to use the pager to view the top, then stop reading.

Tom
Simon Marchi July 6, 2020, 2:06 p.m. | #4
On 2020-06-23 9:20 a.m., Tom Tromey wrote:
> This adds a "help news" subcommand, which simply dumps the NEWS file.

> The NEWS file is now installed.


(1) I first tried "help news" with GDB in its build directory without passing
--data-directory, expecting it to fail, just to see how it would fail.  It
says:

  (gdb) help news
  could not open the NEWS file: No such file or directory.

Maybe this message should say which path it's trying to open.  If this
happens, the cause would probably be a broken GDB installation, and it
would be useful to know where is GDB looking for that file.

(2) When trying to "make install", I get this error:

  $ make install DESTDIR=/tmp/install
  ...
  make[4]: *** No rule to make target 'install-news', needed by 'install-only'.  Stop.

Indeed, the install-news target is used but does not exist.

(3) Since we look for the NEWS file at <data-directory>/NEWS, I presume it won't
work when running GDB in its build directory, either with

  ./gdb --data-directory=data-directory

or

  make run

since the NEWS file is not there.  It's not a big deal, but if you think of an easy
way to make it work, it would be nice.  Can we put a symlink from data-directory/NEWS
to NEWS?

(4) I tried what I mentioned just above and I get some sanitizer error:

  $ ./gdb -q -nx --data-directory=data-directory
  (gdb) help news
  /home/simark/src/binutils-gdb/gdb/cli/cli-decode.c:1214:60: runtime error: index 1024 out of bounds for type 'char [1024]'

Not sure if it's ASan or UBSan.

Simon
Simon Marchi July 6, 2020, 2:14 p.m. | #5
On 2020-07-05 11:59 a.m., Tom Tromey wrote:
>>>>>> "Christian" == Christian Biesinger via Gdb-patches <gdb-patches@sourceware.org> writes:

> 

> Christian> On Tue, Jun 23, 2020 at 8:20 AM Tom Tromey <tom@tromey.com> wrote:

>>> +static void

>>> +help_news (struct ui_file *stream)

>>> +{

>>> +  std::string news_name = std::string (gdb_datadir) + SLASH_STRING + "NEWS";

>>> +  gdb_file_up news_file = gdb_fopen_cloexec (news_name.c_str (), "r");

>>> +  if (news_file == nullptr)

>>> +    perror_with_name (_("could not open the NEWS file"));

>>> +

>>> +  char buffer[1024];

>>> +  size_t offset = 0;

>>> +  while (true)

>>> +    {

>>> +      size_t nbytes = fread (&buffer[offset], 1, sizeof (buffer) - offset,

>>> +                            news_file.get ());

> 

> 

> Christian> Why not use read_entire_file from your other patch?

> 

> The NEWS file seems large-ish, and I figured the normal thing for users

> would be to use the pager to view the top, then stop reading.


The NEWS file is 281k and contains all NEWS since GDB 4.0 (pre year 2000).  So
I think it's safe to say that it grows much slower than the available memory
on the average computer grows.  That amount in memory is minimal compared to
what is typically used for reading DWARF information of large-ish programs,
and it's just transient use anyway.

So I think it's safe to read it all, and preferable if it makes the code
simpler.  I don't think reading 281k will cause any delay noticeable to the
user of "help news".

Simon
Simon Marchi July 6, 2020, 2:18 p.m. | #6
On 2020-07-06 10:06 a.m., Simon Marchi wrote:
> On 2020-06-23 9:20 a.m., Tom Tromey wrote:

>> This adds a "help news" subcommand, which simply dumps the NEWS file.

>> The NEWS file is now installed.

> 

> (1) I first tried "help news" with GDB in its build directory without passing

> --data-directory, expecting it to fail, just to see how it would fail.  It

> says:

> 

>   (gdb) help news

>   could not open the NEWS file: No such file or directory.

> 

> Maybe this message should say which path it's trying to open.  If this

> happens, the cause would probably be a broken GDB installation, and it

> would be useful to know where is GDB looking for that file.

> 

> (2) When trying to "make install", I get this error:

> 

>   $ make install DESTDIR=/tmp/install

>   ...

>   make[4]: *** No rule to make target 'install-news', needed by 'install-only'.  Stop.

> 

> Indeed, the install-news target is used but does not exist.

> 

> (3) Since we look for the NEWS file at <data-directory>/NEWS, I presume it won't

> work when running GDB in its build directory, either with

> 

>   ./gdb --data-directory=data-directory

> 

> or

> 

>   make run

> 

> since the NEWS file is not there.  It's not a big deal, but if you think of an easy

> way to make it work, it would be nice.  Can we put a symlink from data-directory/NEWS

> to NEWS?

> 

> (4) I tried what I mentioned just above and I get some sanitizer error:

> 

>   $ ./gdb -q -nx --data-directory=data-directory

>   (gdb) help news

>   /home/simark/src/binutils-gdb/gdb/cli/cli-decode.c:1214:60: runtime error: index 1024 out of bounds for type 'char [1024]'

> 

> Not sure if it's ASan or UBSan.

> 

> Simon

> 


Oh and:

(5) the way "help news" is implemented means the "news" part is not discoverable
through tab-completion.  I'm not super familiar with how commands work, but would
it work if you registered the "help news" command as a separate command?

That's not a deal-breaker, but I thought I would mention it just in case it's easy
to fix.

Simon
Simon Marchi July 6, 2020, 2:22 p.m. | #7
On 2020-07-06 10:06 a.m., Simon Marchi wrote:
> (3) Since we look for the NEWS file at <data-directory>/NEWS, I presume it won't

> work when running GDB in its build directory, either with

> 

>   ./gdb --data-directory=data-directory

> 

> or

> 

>   make run

> 

> since the NEWS file is not there.  It's not a big deal, but if you think of an easy

> way to make it work, it would be nice.  Can we put a symlink from data-directory/NEWS

> to NEWS?


Ah scratch that, I see that your Makefile change copies NEWS to the data-directory in
the build directory,  so you already thought about that.

Simon
Tom Tromey July 11, 2020, 3:30 p.m. | #8
[...]
Christian> Why not use read_entire_file from your other patch?

Since Simon also mentioned this, I went ahead and made the change.

Tom
Tom Tromey July 11, 2020, 3:31 p.m. | #9
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon>   $ make install DESTDIR=/tmp/install
Simon>   ...
Simon>   make[4]: *** No rule to make target 'install-news', needed by 'install-only'.  Stop.

Oops, that's a leftover from an earlier iteration.
I fixed this.

Simon> (4) I tried what I mentioned just above and I get some sanitizer error:

Simon>   $ ./gdb -q -nx --data-directory=data-directory
Simon>   (gdb) help news
Simon>   /home/simark/src/binutils-gdb/gdb/cli/cli-decode.c:1214:60: runtime error: index 1024 out of bounds for type 'char [1024]'

Simon> Not sure if it's ASan or UBSan.

Probably fixed by using read_entire_file, but I'll check.

Tom
Tom Tromey July 11, 2020, 3:56 p.m. | #10
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> (5) the way "help news" is implemented means the "news" part is not discoverable
Simon> through tab-completion.  I'm not super familiar with how commands work, but would
Simon> it work if you registered the "help news" command as a separate command?

I wasn't sure if that would work, and gdb already fails to list "help all"
as a possible completion.  So, in v3 of the series I've added a new
completion function for "help".

Tom

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index f7585133c51..0fd39857326 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -65,6 +65,9 @@ 
 
 * New commands
 
+help news
+  Show this NEWS file.
+
 set exec-file-mismatch -- Set exec-file-mismatch handling (ask|warn|off).
 show exec-file-mismatch -- Show exec-file-mismatch handling (ask|warn|off).
   Set or show the option 'exec-file-mismatch'.  When GDB attaches to a
diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 85f50aa8e48..c986a1c45fb 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -1197,6 +1197,38 @@  apropos_cmd (struct ui_file *stream,
     }
 }
 
+/* Implement "help news".  */
+
+static void
+help_news (struct ui_file *stream)
+{
+  std::string news_name = std::string (gdb_datadir) + SLASH_STRING + "NEWS";
+  gdb_file_up news_file = gdb_fopen_cloexec (news_name.c_str (), "r");
+  if (news_file == nullptr)
+    perror_with_name (_("could not open the NEWS file"));
+
+  char buffer[1024];
+  size_t offset = 0;
+  while (true)
+    {
+      size_t nbytes = fread (&buffer[offset], 1, sizeof (buffer) - offset,
+			     news_file.get ());
+      if (nbytes == 0)
+	break;
+      size_t n_valid = offset + nbytes;
+      size_t newline;
+      for (newline = n_valid; newline > 0 && buffer[newline] != '\n'; --newline)
+	;
+      if (newline == 0)
+	error (_("NEWS file is malformed"));
+      buffer[newline] = '\0';
+      fputs_filtered (buffer, stream);
+      fputs_filtered ("\n", stream);
+      offset = n_valid - (newline + 1);
+      memmove (buffer, &buffer[newline + 1], offset);
+    }
+}
+
 /* This command really has to deal with two things:
    1) I want documentation on *this string* (usually called by
       "help commandname").
@@ -1225,6 +1257,12 @@  help_cmd (const char *command, struct ui_file *stream)
       return;
     }
 
+  if (strcmp (command, "news") == 0)
+    {
+      help_news (stream);
+      return;
+    }
+
   const char *orig_command = command;
   c = lookup_cmd (&command, cmdlist, "", NULL, 0, 0);
 
@@ -1330,6 +1368,9 @@  Type \"help%s\" followed by a class name for a list of commands in ",
 
       fprintf_filtered (stream, "\n\
 Type \"help all\" for the list of all commands.");
+
+      fprintf_filtered (stream, "\n\
+Type \"help news\" to see what is new in GDB.");
     }
 
   fprintf_filtered (stream, "\nType \"help%s\" followed by %scommand name ",
diff --git a/gdb/data-directory/Makefile.in b/gdb/data-directory/Makefile.in
index 3f0c729404b..85a2f41c351 100644
--- a/gdb/data-directory/Makefile.in
+++ b/gdb/data-directory/Makefile.in
@@ -139,6 +139,8 @@  SYSTEM_GDBINIT_FILES = \
 	elinos.py \
 	wrs-linux.py
 
+GDB_FILES = NEWS
+
 FLAGS_TO_PASS = \
 	"prefix=$(prefix)" \
 	"exec_prefix=$(exec_prefix)" \
@@ -172,7 +174,7 @@  FLAGS_TO_PASS = \
 	"RUNTESTFLAGS=$(RUNTESTFLAGS)"
 
 .PHONY: all
-all: stamp-syscalls stamp-python stamp-guile stamp-system-gdbinit
+all: stamp-syscalls stamp-python stamp-guile stamp-system-gdbinit gdb-files
 
 %.xml: @MAINTAINER_MODE_TRUE@ %.xml.in apply-defaults.xsl linux-defaults.xml.in
 	$(XSLTPROC) -o $(SYSCALLS_SRCDIR)/$@ $(SYSCALLS_SRCDIR)/apply-defaults.xsl\
@@ -234,6 +236,26 @@  uninstall-syscalls:
 	  done \
 	done
 
+gdb-files: $(addprefix $(srcdir)/../,$(GDB_FILES))
+	files='$(GDB_FILES)'; \
+	for file in $$files; do \
+	  cp $(srcdir)/../$$file .; \
+	done
+
+.PHONY: install-gdb-files
+install-gdb-files:
+	files='$(GDB_FILES)'; \
+	for file in $$files; do \
+	  $(INSTALL_DATA) $(srcdir)/../$$file  $(DESTDIR)$(GDB_DATADIR); \
+	done
+
+.PHONY: uninstall-gdb-files
+uninstall-gdb-files:
+	files='$(GDB_FILES)'; \
+	for file in $$files; do \
+	  rm -f $(DESTDIR)$(GDB_DATADIR)/$$file; \
+	done
+
 stamp-python: Makefile $(PYTHON_FILES)
 	rm -rf ./$(PYTHON_DIR)
 	files='$(PYTHON_FILES)' ; \
@@ -379,11 +401,11 @@  install: all
 
 .PHONY: install-only
 install-only: install-syscalls install-python install-guile \
-	install-system-gdbinit
+	install-system-gdbinit install-news
 
 .PHONY: uninstall
 uninstall: uninstall-syscalls uninstall-python uninstall-guile \
-	uninstall-system-gdbinit
+	uninstall-system-gdbinit uninstall-news
 
 .PHONY: clean
 clean: clean-syscalls clean-python clean-guile clean-system-gdbinit
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 7f572c37c5c..37d5fe1e4cd 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -2163,6 +2163,10 @@  the command name and all its aliases separated by commas.
 This first line will be followed by the full definition of all aliases
 having default arguments.
 
+@item help news
+Show the news file.  Notable features in each release are documented
+in this file.
+
 @kindex apropos
 @item apropos [-v] @var{regexp}
 The @code{apropos} command searches through all of the @value{GDBN}