gdb: Remove an update of current_source_line and current_source_symtab
Commit Message
While reviewing some of the annotation code I noticed that
identify_source_line (in source.c) sets current_source_line,
current_source_symtab, and also calls clear_lines_listed_range. This
seems a little strange, identify_source_line is really a wrapper
around annotate_source, and is only called when annotation_level is
greater than 0 (so annotations are turned on).
It seems weird (to me) that when annotations are on we update GDB's
idea of the "current" line/symtab, but when they are off we don't,
given that annotations are really about communicating GDB's state to a
user (GUI) and surely shouldn't be changing GDB's behaviour.
This commit removes from identify_source_line all of the setting of
current line/symtab and the call to clear_lines_listed_range, after
doing this GDB still passes all tests, so I don't believe these lines
were actually required.
With this code removed identify_source_line is only a wrapper around
annotate_source, so I moved identify_source_line to annotate.c and
renamed it to annotate_source_line.
gdb/ChangeLog:
* annotate.c: Add 'source.h' and 'objfiles.h' includes.
(annotate_source): Make static.
(get_filename_and_charpos): Moved from source.c.
(annotate_source_line): Moved from source.c and renamed from
identify_source_line. Update the return type.
* annotate.h (annotate_source): Delete declaration.
(annotate_source_line): Declaration moved from source.h, and
renamed from identify_source_line. Return type updated.
* source.c (get_filename_and_charpos): Moved to annotate.c.
(identify_source_line): Moved to annotate.c and renamed to
annotate_source_line.
(info_line_command): Remove check of annotation_level.
* source.h (identify_source_line): Move declaration to annotate.h
and rename to annotate_source_line.
* stack.c: Add 'annotate.h' include.
(print_frame_info): Remove check of annotation_level.
---
gdb/ChangeLog | 19 +++++++++++++++++++
gdb/annotate.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
gdb/annotate.h | 12 ++++++++++--
gdb/source.c | 47 ++---------------------------------------------
gdb/source.h | 11 -----------
gdb/stack.c | 8 +++-----
6 files changed, 82 insertions(+), 64 deletions(-)
Comments
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
Andrew> It seems weird (to me) that when annotations are on we update GDB's
Andrew> idea of the "current" line/symtab, but when they are off we don't,
Andrew> given that annotations are really about communicating GDB's state to a
Andrew> user (GUI) and surely shouldn't be changing GDB's behaviour.
Andrew> This commit removes from identify_source_line all of the setting of
Andrew> current line/symtab and the call to clear_lines_listed_range, after
Andrew> doing this GDB still passes all tests, so I don't believe these lines
Andrew> were actually required.
Andrew> With this code removed identify_source_line is only a wrapper around
Andrew> annotate_source, so I moved identify_source_line to annotate.c and
Andrew> renamed it to annotate_source_line.
Thanks for doing this. This looks good.
Andrew> +get_filename_and_charpos (struct symtab *s, char **fullname)
The fullname parameter could be removed now, since the only caller
passes NULL.
Andrew> + bool done = annotate_source_line (sal.symtab, sal.line, mid_statement,
Andrew> + get_frame_pc (frame));
Andrew> if (!done)
Andrew> {
Andrew> if (deprecated_print_frame_info_listing_hook)
I wonder if this "!done" stuff even makes sense. It seems a bit strange
that annotations would cause the regular output to change.
Tom
This series addresses Tom's comments to my first patch.
#1 Removes the unused parameter from get_filename_and_charpos,
#2 Is a new clean up, creating a new function to load a symtab's
line_charpos data,
#3 This is the original patch, updated to take account of the above
two changes,
#4 This make annotate_source_line a void function, and changes GDB
to always print the source line, even when annotations are on.
I kept #4 separate as I worried that changing GDB in this way might
break some GUI that relies on annotations, and which we care about
enough to want to go back to the old behaviour - having a separate
patch allows that code to be reverted easily :)
Thanks,
Andrew
---
Andrew Burgess (4):
gdb: Remove unused parameter
gdb: New function to open source file and compute line charpos data
gdb: Remove an update of current_source_line and current_source_symtab
gdb: Don't allow annotations to influence what else GDB prints
gdb/ChangeLog | 51 ++++++++++++++++++++++++++++++
gdb/annotate.c | 27 +++++++++++++++-
gdb/annotate.h | 10 ++++--
gdb/source-cache.c | 8 ++---
gdb/source.c | 56 ++++++++-------------------------
gdb/source.h | 22 +++----------
gdb/stack.c | 63 ++++++++++++++++++--------------------
gdb/testsuite/ChangeLog | 6 ++++
gdb/testsuite/gdb.base/annota1.exp | 4 +--
gdb/testsuite/gdb.cp/annota2.exp | 1 +
gdb/testsuite/gdb.cp/annota3.exp | 2 +-
11 files changed, 144 insertions(+), 106 deletions(-)
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
Andrew> This series addresses Tom's comments to my first patch.
Thanks for doing all of this.
Andrew> I kept #4 separate as I worried that changing GDB in this way might
Andrew> break some GUI that relies on annotations, and which we care about
Andrew> enough to want to go back to the old behaviour - having a separate
Andrew> patch allows that code to be reverted easily :)
Makes sense. I hope we should be safe, as the idea behind annotations
is that the metadata is self-contained and the UI generally should not
be looking at the user text. I guess I wouldn't be surprised if some
front end were parsing output -- but on the other hand the reason MI
exists is precisely so we don't have to keep CLI output consistent.
I read through the patches and they all look good to me.
Tom
@@ -26,6 +26,8 @@
#include "inferior.h"
#include "infrun.h"
#include "top.h"
+#include "source.h"
+#include "objfiles.h"
/* Prototypes for local functions. */
@@ -431,7 +433,7 @@ annotate_arg_end (void)
printf_filtered (("\n\032\032arg-end\n"));
}
-void
+static void
annotate_source (const char *filename, int line, int character, int mid,
struct gdbarch *gdbarch, CORE_ADDR pc)
{
@@ -444,6 +446,51 @@ annotate_source (const char *filename, int line, int character, int mid,
mid ? "middle" : "beg", paddress (gdbarch, pc));
}
+/* Get full pathname and line number positions for a symtab. Set *FULLNAME
+ to the actual name of the file as extracted from S, or to NULL if the
+ file is not found (or could not be opened). */
+
+static void
+get_filename_and_charpos (struct symtab *s, char **fullname)
+{
+ scoped_fd desc = open_source_file (s);
+ if (desc.get () < 0)
+ {
+ if (fullname)
+ *fullname = NULL;
+ return;
+ }
+ if (fullname)
+ *fullname = s->fullname;
+ if (s->line_charpos == nullptr)
+ find_source_lines (s, desc.get ());
+}
+
+/* See annotate.h. */
+
+bool
+annotate_source_line (struct symtab *s, int line, int mid_statement,
+ CORE_ADDR pc)
+{
+ if (annotation_level > 0)
+ {
+ if (s->line_charpos == nullptr)
+ get_filename_and_charpos (s, (char **) NULL);
+ if (s->fullname == nullptr)
+ return false;
+ if (line > s->nlines)
+ /* Don't index off the end of the line_charpos array. */
+ return false;
+ annotate_source (s->fullname, line, s->line_charpos[line - 1],
+ mid_statement, get_objfile_arch (SYMTAB_OBJFILE (s)),
+ pc);
+ return true;
+ }
+
+ return false;
+}
+
+
void
annotate_frame_begin (int level, struct gdbarch *gdbarch, CORE_ADDR pc)
{
@@ -87,8 +87,16 @@ struct annotate_arg_emitter
DISABLE_COPY_AND_ASSIGN (annotate_arg_emitter);
};
-extern void annotate_source (const char *, int, int, int,
- struct gdbarch *, CORE_ADDR);
+/* If annotations are turned on then print annotation describing the full
+ name of the source file S and the line number LINE and its corresponding
+ character position.
+
+ MID_STATEMENT is nonzero if the PC is not at the beginning of that line.
+
+ Return 1 if successful, 0 if could not find the file or if annotations
+ are turned off. */
+extern bool annotate_source_line (struct symtab *s, int line,
+ int mid_statement, CORE_ADDR pc);
extern void annotate_frame_begin (int, struct gdbarch *, CORE_ADDR);
extern void annotate_function_call (void);
@@ -1192,49 +1192,6 @@ find_source_lines (struct symtab *s, int desc)
-/* Get full pathname and line number positions for a symtab.
- Set *FULLNAME to actual name of the file as found by `openp',
- or to 0 if the file is not found. */
-
-static void
-get_filename_and_charpos (struct symtab *s, char **fullname)
-{
- scoped_fd desc = open_source_file (s);
- if (desc.get () < 0)
- {
- if (fullname)
- *fullname = NULL;
- return;
- }
- if (fullname)
- *fullname = s->fullname;
- if (s->line_charpos == 0)
- find_source_lines (s, desc.get ());
-}
-
-/* See source.h. */
-
-int
-identify_source_line (struct symtab *s, int line, int mid_statement,
- CORE_ADDR pc)
-{
- if (s->line_charpos == 0)
- get_filename_and_charpos (s, (char **) NULL);
- if (s->fullname == 0)
- return 0;
- if (line > s->nlines)
- /* Don't index off the end of the line_charpos array. */
- return 0;
- annotate_source (s->fullname, line, s->line_charpos[line - 1],
- mid_statement, get_objfile_arch (SYMTAB_OBJFILE (s)), pc);
-
- current_source_line = line;
- current_source_symtab = s;
- clear_lines_listed_range ();
- return 1;
-}
-
-
/* Print source lines from the file of symtab S,
starting with line number LINE and stopping before line number STOPLINE. */
@@ -1519,8 +1476,8 @@ info_line_command (const char *arg, int from_tty)
/* If this is the only line, show the source code. If it could
not find the file, don't do anything special. */
- if (annotation_level && sals.size () == 1)
- identify_source_line (sal.symtab, sal.line, 0, start_pc);
+ if (sals.size () == 1)
+ annotate_source_line (sal.symtab, sal.line, 0, start_pc);
}
else
/* Is there any case in which we get here, and have an address
@@ -128,17 +128,6 @@ extern void clear_current_source_symtab_and_line (void);
/* Add a source path substitution rule. */
extern void add_substitute_path_rule (char *, char *);
-/* Print text describing the full name of the source file S
- and the line number LINE and its corresponding character position.
- The text starts with two Ctrl-z so that the Emacs-GDB interface
- can easily find it.
-
- MID_STATEMENT is nonzero if the PC is not at the beginning of that line.
-
- Return 1 if successful, 0 if could not find the file. */
-extern int identify_source_line (struct symtab *s, int line,
- int mid_statement, CORE_ADDR pc);
-
/* Flags passed as 4th argument to print_source_lines. */
enum print_source_lines_flag
{
@@ -47,6 +47,7 @@
#include "linespec.h"
#include "cli/cli-utils.h"
#include "objfiles.h"
+#include "annotate.h"
#include "symfile.h"
#include "extension.h"
@@ -869,13 +870,10 @@ print_frame_info (struct frame_info *frame, int print_level,
if (source_print && sal.symtab)
{
- int done = 0;
int mid_statement = ((print_what == SRC_LINE)
&& frame_show_address (frame, sal));
-
- if (annotation_level)
- done = identify_source_line (sal.symtab, sal.line, mid_statement,
- get_frame_pc (frame));
+ bool done = annotate_source_line (sal.symtab, sal.line, mid_statement,
+ get_frame_pc (frame));
if (!done)
{
if (deprecated_print_frame_info_listing_hook)