[RFC/RFA] Set current language when dumping symtab

[RFC/RFA] Set current language when dumping symtab

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

Hello,

One of our users tried to use the "maint print symbols syms" command
to dump all the symbols in a text format, and that caused a crash on
x86-windows. The reason for the crash is that dump_symtab() indirectly
uses the current language vector to analyze the symbols and then do
the printing. The symtab language may be different from the current
language, so the current language needs to be temporarily adjusted
during the printing. This is what the attached patch does.

2005-05-02  Joel Brobecker  <brobecker@adacore.com>

        * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
        (dump_symtab): New function.
        * Makefile.in (symmisc.o): Add dependency on ui-out.h.

Tested on x86-linux, no regression.
You'll notice that I formally tested the change on x86-linux, as
running the testsuite on x86-windows just takes forever (I never had
the patience to wait till the end), but I did verify that this fixes
the problem, and that the output using the language assocated to each
symtab we're printing.

Also, I think we need to do the same for print_msymbols() and
print_psymbols(). I'm a bit in a rush at the moment, so I'm sending
this patch as is for now. I'll understand if I need to rework it later
to include the minsyms and psyms. I thought it might already be useful
to others as is. Just let me know.

Thanks,
-- 
Joel

[-- Attachment #2: symmisc.c.diff --]
[-- Type: text/plain, Size: 2248 bytes --]

Index: symmisc.c
===================================================================
RCS file: /cvs/src/src/gdb/symmisc.c,v
retrieving revision 1.38
diff -u -p -r1.38 symmisc.c
--- symmisc.c	14 Feb 2005 14:37:38 -0000	1.38
+++ symmisc.c	4 May 2005 00:10:06 -0000
@@ -37,6 +37,7 @@
 #include "gdb_regex.h"
 #include "gdb_stat.h"
 #include "dictionary.h"
+#include "ui-out.h"
 
 #include "gdb_string.h"
 #include "readline/readline.h"
@@ -440,8 +441,8 @@ dump_psymtab (struct objfile *objfile, s
 }
 
 static void
-dump_symtab (struct objfile *objfile, struct symtab *symtab,
-	     struct ui_file *outfile)
+dump_symtab_1 (struct objfile *objfile, struct symtab *symtab,
+	       struct ui_file *outfile)
 {
   int i;
   struct dict_iterator iter;
@@ -533,6 +534,26 @@ dump_symtab (struct objfile *objfile, st
     }
 }
 
+static void
+dump_symtab (struct objfile *objfile, struct symtab *symtab,
+	     struct ui_file *outfile)
+{
+  enum language saved_lang;
+  volatile struct gdb_exception except;
+
+  /* Set the current language to the language of the symtab we're dumping
+     because certain routines used during dump_symtab() use the current
+     language to print an image of the symbol.  We'll restore it later.  */
+  saved_lang = set_language (symtab->language);
+
+  TRY_CATCH (except, RETURN_MASK_ERROR)
+    {
+      dump_symtab_1 (objfile, symtab, outfile);
+    }
+
+  set_language (saved_lang);
+}
+
 void
 maintenance_print_symbols (char *args, int from_tty)
 {
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.726
diff -u -p -r1.726 Makefile.in
--- Makefile.in	2 May 2005 12:05:11 -0000	1.726
+++ Makefile.in	4 May 2005 00:10:31 -0000
@@ -2643,7 +2643,7 @@ symmisc.o: symmisc.c $(defs_h) $(symtab_
 	$(symfile_h) $(objfiles_h) $(breakpoint_h) $(command_h) \
 	$(gdb_obstack_h) $(exceptions_h) $(language_h) $(bcache_h) \
 	$(block_h) $(gdb_regex_h) $(dictionary_h) $(gdb_string_h) \
-	$(readline_h)
+	$(readline_h) $(ui_out_h)
 symtab.o: symtab.c $(defs_h) $(symtab_h) $(gdbtypes_h) $(gdbcore_h) \
 	$(frame_h) $(target_h) $(value_h) $(symfile_h) $(objfiles_h) \
 	$(gdbcmd_h) $(call_cmds_h) $(gdb_regex_h) $(expression_h) \

Re: [RFC/RFA] Set current language when dumping symtab

[Daniel Jacobowitz]

On Tue, May 03, 2005 at 05:19:55PM -0700, Joel Brobecker wrote:
> Hello,
> 
> One of our users tried to use the "maint print symbols syms" command
> to dump all the symbols in a text format, and that caused a crash on
> x86-windows. The reason for the crash is that dump_symtab() indirectly
> uses the current language vector to analyze the symbols and then do
> the printing. The symtab language may be different from the current
> language, so the current language needs to be temporarily adjusted
> during the printing. This is what the attached patch does.
> 
> 2005-05-02  Joel Brobecker  <brobecker@adacore.com>
> 
>         * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
>         (dump_symtab): New function.
>         * Makefile.in (symmisc.o): Add dependency on ui-out.h.

What's the new dependency on ui-out.h for?  I didn't see anything
obvious in the patch.

Also, what crashes?  i.e. why specifically is it harmful to have the
wrong language set?

Also, I am not convinced that the new TRY_CATCH is necessary.  The
only bit likely to throw is print_symbol, which is already wrapped in
catch_errors.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/RFA] Set current language when dumping symtab

[Joel Brobecker]

> > 2005-05-02  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
> >         (dump_symtab): New function.
> >         * Makefile.in (symmisc.o): Add dependency on ui-out.h.
> 
> What's the new dependency on ui-out.h for?  I didn't see anything
> obvious in the patch.

I agree it's not obvious. That's because of TRY_CATCH:

#define TRY_CATCH(EXCEPTION,MASK) \
     { \
       EXCEPTIONS_SIGJMP_BUF *buf = \
         exceptions_state_mc_init (uiout, &(EXCEPTION), (MASK)); \
       EXCEPTIONS_SIGSETJMP (*buf); \
     } \
     while (exceptions_state_mc_action_iter ()) \
       while (exceptions_state_mc_action_iter_1 ())

There is a dependency on "uiout". Perhaps it would be better to include
that file from exceptions.h, rather than requiring all clients to include
it themselves? I could send a separate RFA for that.

> Also, what crashes?  i.e. why specifically is it harmful to have the
> wrong language set?

In Ada, we rely on some special encoding to convey some information
that certain debugging formats such as stabs can not express. In our
case, one of the C symtabs had an entity whose name mislead the Ada
language, and caused it to try to access something that didn't exist.
This caused an internal-error, IIRC.

> Also, I am not convinced that the new TRY_CATCH is necessary.  The
> only bit likely to throw is print_symbol, which is already wrapped in
> catch_errors.

That's true, and I'd be happy to remove it. But I thought that it might
be safer to use it anyway, so that any change underneath that might cause
an exception to be thrown does not affect this code. This is a hard
guaranty that the language will never be changed as a side-effect of
this command.

-- 
Joel

Re: [RFC/RFA] Set current language when dumping symtab

[Daniel Jacobowitz]

On Sun, May 29, 2005 at 12:10:56PM +1000, Joel Brobecker wrote:
> > > 2005-05-02  Joel Brobecker  <brobecker@adacore.com>
> > > 
> > >         * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
> > >         (dump_symtab): New function.
> > >         * Makefile.in (symmisc.o): Add dependency on ui-out.h.
> > 
> > What's the new dependency on ui-out.h for?  I didn't see anything
> > obvious in the patch.
> 
> I agree it's not obvious. That's because of TRY_CATCH:
> 
> #define TRY_CATCH(EXCEPTION,MASK) \
>      { \
>        EXCEPTIONS_SIGJMP_BUF *buf = \
>          exceptions_state_mc_init (uiout, &(EXCEPTION), (MASK)); \
>        EXCEPTIONS_SIGSETJMP (*buf); \
>      } \
>      while (exceptions_state_mc_action_iter ()) \
>        while (exceptions_state_mc_action_iter_1 ())
> 
> There is a dependency on "uiout". Perhaps it would be better to include
> that file from exceptions.h, rather than requiring all clients to include
> it themselves? I could send a separate RFA for that.

Yes please.  I figured it was something like that, went looking at the
definition, and my eyes skipped right over it.

> > Also, what crashes?  i.e. why specifically is it harmful to have the
> > wrong language set?
> 
> In Ada, we rely on some special encoding to convey some information
> that certain debugging formats such as stabs can not express. In our
> case, one of the C symtabs had an entity whose name mislead the Ada
> language, and caused it to try to access something that didn't exist.
> This caused an internal-error, IIRC.

Hmm... I guess this bit is OK.

> > Also, I am not convinced that the new TRY_CATCH is necessary.  The
> > only bit likely to throw is print_symbol, which is already wrapped in
> > catch_errors.
> 
> That's true, and I'd be happy to remove it. But I thought that it might
> be safer to use it anyway, so that any change underneath that might cause
> an exception to be thrown does not affect this code. This is a hard
> guaranty that the language will never be changed as a side-effect of
> this command.

That's the sort of thinking that leads to hordes of exception regions
that no one can ever remove.  Please let's not.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/RFA] Set current language when dumping symtab

[Joel Brobecker]

> > #define TRY_CATCH(EXCEPTION,MASK) \
> >      { \
> >        EXCEPTIONS_SIGJMP_BUF *buf = \
> >          exceptions_state_mc_init (uiout, &(EXCEPTION), (MASK)); \
> >        EXCEPTIONS_SIGSETJMP (*buf); \
> >      } \
> >      while (exceptions_state_mc_action_iter ()) \
> >        while (exceptions_state_mc_action_iter_1 ())
> > 
> > There is a dependency on "uiout". Perhaps it would be better to include
> > that file from exceptions.h, rather than requiring all clients to include
> > it themselves? I could send a separate RFA for that.
> 
> Yes please.  I figured it was something like that, went looking at the
> definition, and my eyes skipped right over it.

Sure. Patch sent, although I can provide the URL yet, as sources.redhat.com
is unfortunately refusing connections on port 25.  Will send the URL as
a followup message as soon as the problem is fixed.

> > > Also, I am not convinced that the new TRY_CATCH is necessary.  The
> > > only bit likely to throw is print_symbol, which is already wrapped in
> > > catch_errors.
> > 
> > That's true, and I'd be happy to remove it. But I thought that it might
> > be safer to use it anyway, so that any change underneath that might cause
> > an exception to be thrown does not affect this code. This is a hard
> > guaranty that the language will never be changed as a side-effect of
> > this command.

Okidoke, understood. New patch attached.

2005-05-30  Joel Brobecker  <brobecker@adacore.com>

        * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
        (dump_symtab): New function.

Tested on x86-linux.

Thanks,
-- 
Joel

Re: [RFC/RFA] Set current language when dumping symtab

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]

Humpf, with the patch, this time. Thanks Daniel!

On Mon, May 30, 2005 at 03:45:38PM +1000, Joel Brobecker wrote:
> > > #define TRY_CATCH(EXCEPTION,MASK) \
> > >      { \
> > >        EXCEPTIONS_SIGJMP_BUF *buf = \
> > >          exceptions_state_mc_init (uiout, &(EXCEPTION), (MASK)); \
> > >        EXCEPTIONS_SIGSETJMP (*buf); \
> > >      } \
> > >      while (exceptions_state_mc_action_iter ()) \
> > >        while (exceptions_state_mc_action_iter_1 ())
> > > 
> > > There is a dependency on "uiout". Perhaps it would be better to include
> > > that file from exceptions.h, rather than requiring all clients to include
> > > it themselves? I could send a separate RFA for that.
> > 
> > Yes please.  I figured it was something like that, went looking at the
> > definition, and my eyes skipped right over it.
> 
> Sure. Patch sent, although I can provide the URL yet, as sources.redhat.com
> is unfortunately refusing connections on port 25.  Will send the URL as
> a followup message as soon as the problem is fixed.
> 
> > > > Also, I am not convinced that the new TRY_CATCH is necessary.  The
> > > > only bit likely to throw is print_symbol, which is already wrapped in
> > > > catch_errors.
> > > 
> > > That's true, and I'd be happy to remove it. But I thought that it might
> > > be safer to use it anyway, so that any change underneath that might cause
> > > an exception to be thrown does not affect this code. This is a hard
> > > guaranty that the language will never be changed as a side-effect of
> > > this command.
> 
> Okidoke, understood. New patch attached.
> 
> 2005-05-30  Joel Brobecker  <brobecker@adacore.com>
> 
>         * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
>         (dump_symtab): New function.
> 
> Tested on x86-linux.

-- 
Joel

[-- Attachment #2: symmisc.c.diff --]
[-- Type: text/plain, Size: 1255 bytes --]

Index: symmisc.c
===================================================================
RCS file: /cvs/src/src/gdb/symmisc.c,v
retrieving revision 1.38
diff -u -p -r1.38 symmisc.c
--- symmisc.c	14 Feb 2005 14:37:38 -0000	1.38
+++ symmisc.c	30 May 2005 05:16:41 -0000
@@ -440,8 +440,8 @@ dump_psymtab (struct objfile *objfile, s
 }
 
 static void
-dump_symtab (struct objfile *objfile, struct symtab *symtab,
-	     struct ui_file *outfile)
+dump_symtab_1 (struct objfile *objfile, struct symtab *symtab,
+	       struct ui_file *outfile)
 {
   int i;
   struct dict_iterator iter;
@@ -533,6 +533,23 @@ dump_symtab (struct objfile *objfile, st
     }
 }
 
+static void
+dump_symtab (struct objfile *objfile, struct symtab *symtab,
+	     struct ui_file *outfile)
+{
+  enum language saved_lang;
+  volatile struct gdb_exception except;
+
+  /* Set the current language to the language of the symtab we're dumping
+     because certain routines used during dump_symtab() use the current
+     language to print an image of the symbol.  We'll restore it later.  */
+  saved_lang = set_language (symtab->language);
+
+  dump_symtab_1 (objfile, symtab, outfile);
+
+  set_language (saved_lang);
+}
+
 void
 maintenance_print_symbols (char *args, int from_tty)
 {

Re: [RFC/RFA] Set current language when dumping symtab

[Daniel Jacobowitz]

On Tue, May 31, 2005 at 05:53:39AM +1000, Joel Brobecker wrote:
> +static void
> +dump_symtab (struct objfile *objfile, struct symtab *symtab,
> +	     struct ui_file *outfile)
> +{
> +  enum language saved_lang;
> +  volatile struct gdb_exception except;
> +
> +  /* Set the current language to the language of the symtab we're dumping
> +     because certain routines used during dump_symtab() use the current
> +     language to print an image of the symbol.  We'll restore it later.  */
> +  saved_lang = set_language (symtab->language);
> +
> +  dump_symtab_1 (objfile, symtab, outfile);
> +
> +  set_language (saved_lang);
> +}
> +

Except now except is unused, and there's no reason not to fold this
into dump_symtab.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/RFA] Set current language when dumping symtab

[Joel Brobecker]

(back online after 24h of flight :-)

On Mon, May 30, 2005 at 03:54:58PM -0400, Daniel Jacobowitz wrote:
> On Tue, May 31, 2005 at 05:53:39AM +1000, Joel Brobecker wrote:
> > +static void
> > +dump_symtab (struct objfile *objfile, struct symtab *symtab,
> > +	     struct ui_file *outfile)
> > +{
> > +  enum language saved_lang;
> > +  volatile struct gdb_exception except;
> > +
> > +  /* Set the current language to the language of the symtab we're dumping
> > +     because certain routines used during dump_symtab() use the current
> > +     language to print an image of the symbol.  We'll restore it later.  */
> > +  saved_lang = set_language (symtab->language);
> > +
> > +  dump_symtab_1 (objfile, symtab, outfile);
> > +
> > +  set_language (saved_lang);
> > +}
> > +
> 
> Except now except is unused,

Argh! I had removed it and some undo/redo manipulations put it back
without me noticing... I will fix this, thanks!

> and there's no reason not to fold this into dump_symtab.

I tend to still prefer the folding, because I find it clearer
that way. And if for any reason we change the implementation of
dump_symtab_1 to have an early return somewhere in the code, we're
not in danger of forgetting to reset the language.

But I'm happy either way. Let me know which you prefer, and I'll
submit a new patch.

-- 
Joel

Re: [RFC/RFA] Set current language when dumping symtab

[Daniel Jacobowitz]

On Tue, May 31, 2005 at 06:29:32PM -0700, Joel Brobecker wrote:
> > and there's no reason not to fold this into dump_symtab.
> 
> I tend to still prefer the folding, because I find it clearer
> that way. And if for any reason we change the implementation of
> dump_symtab_1 to have an early return somewhere in the code, we're
> not in danger of forgetting to reset the language.
> 
> But I'm happy either way. Let me know which you prefer, and I'll
> submit a new patch.

OK - you have a good reason, that's enough for me :-)

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/RFA] Set current language when dumping symtab

[Joel Brobecker]

> > > #define TRY_CATCH(EXCEPTION,MASK) \
> > >      { \
> > >        EXCEPTIONS_SIGJMP_BUF *buf = \
> > >          exceptions_state_mc_init (uiout, &(EXCEPTION), (MASK)); \
> > >        EXCEPTIONS_SIGSETJMP (*buf); \
> > >      } \
> > >      while (exceptions_state_mc_action_iter ()) \
> > >        while (exceptions_state_mc_action_iter_1 ())
> > > 
> > > There is a dependency on "uiout". Perhaps it would be better to include
> > > that file from exceptions.h, rather than requiring all clients to include
> > > it themselves? I could send a separate RFA for that.
> > 
> > Yes please.  I figured it was something like that, went looking at the
> > definition, and my eyes skipped right over it.
> 
> Sure. Patch sent, although I can provide the URL yet, as sources.redhat.com
> is unfortunately refusing connections on port 25.  Will send the URL as
> a followup message as soon as the problem is fixed.

Just for the record, the URL in question is:

        http://sources.redhat.com/ml/gdb-patches/2005-05/msg00632.html

-- 
Joel

Re: [RFC/RFA] Set current language when dumping symtab

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

> OK - you have a good reason, that's enough for me :-)

Cool :-). Here is the patch after removing the unused variable.

2005-06-02  Joel Brobecker  <brobecker@adacore.com>

        * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
        (dump_symtab): New function.

Tested on x86-linux. OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: symmisc.c.diff --]
[-- Type: text/plain, Size: 1213 bytes --]

Index: symmisc.c
===================================================================
RCS file: /cvs/src/src/gdb/symmisc.c,v
retrieving revision 1.38
diff -u -p -r1.38 symmisc.c
--- symmisc.c	14 Feb 2005 14:37:38 -0000	1.38
+++ symmisc.c	2 Jun 2005 16:57:53 -0000
@@ -440,8 +440,8 @@ dump_psymtab (struct objfile *objfile, s
 }
 
 static void
-dump_symtab (struct objfile *objfile, struct symtab *symtab,
-	     struct ui_file *outfile)
+dump_symtab_1 (struct objfile *objfile, struct symtab *symtab,
+	       struct ui_file *outfile)
 {
   int i;
   struct dict_iterator iter;
@@ -533,6 +533,22 @@ dump_symtab (struct objfile *objfile, st
     }
 }
 
+static void
+dump_symtab (struct objfile *objfile, struct symtab *symtab,
+	     struct ui_file *outfile)
+{
+  enum language saved_lang;
+
+  /* Set the current language to the language of the symtab we're dumping
+     because certain routines used during dump_symtab() use the current
+     language to print an image of the symbol.  We'll restore it later.  */
+  saved_lang = set_language (symtab->language);
+
+  dump_symtab_1 (objfile, symtab, outfile);
+
+  set_language (saved_lang);
+}
+
 void
 maintenance_print_symbols (char *args, int from_tty)
 {

Re: [RFC/RFA] Set current language when dumping symtab

[Daniel Jacobowitz]

On Thu, Jun 02, 2005 at 05:10:07PM -0700, Joel Brobecker wrote:
> > OK - you have a good reason, that's enough for me :-)
> 
> Cool :-). Here is the patch after removing the unused variable.
> 
> 2005-06-02  Joel Brobecker  <brobecker@adacore.com>
> 
>         * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
>         (dump_symtab): New function.
> 
> Tested on x86-linux. OK to apply?

This is OK.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/RFA] Set current language when dumping symtab

[Joel Brobecker]

> > 2005-06-02  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * symmisc.c (dump_symtab_1): Renamed from dump_symtab.
> >         (dump_symtab): New function.
> > 
> > Tested on x86-linux. OK to apply?
> 
> This is OK.

Thank you, checked in.

-- 
Joel