[PATCH] avoid GDB crash on inspection of pascal arrays

[PATCH] avoid GDB crash on inspection of pascal arrays

[Pierre Muller]

  The is_pascal_string_type function in p-lang.c 
could sometimes be called with an type parameter being NULL.
  This caused crashes in the Free Pascal IDE,
that I finally debugged recently.

  Checked in as pascal language maintainer,

Pierre Muller



ChangeLog entry:

2020-03-08  Pierre Muller  <muller@ics.u-strasbg.fr>

       * p-lang.c (is_pascal_string_type): Check that TYPE arg is non NULL.



Index: p-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/p-lang.c,v
retrieving revision 1.50
diff -u -p -r1.50 p-lang.c
--- p-lang.c    14 Jan 2010 08:03:36 -0000      1.50
+++ p-lang.c    8 Mar 2010 16:49:20 -0000
@@ -101,7 +101,7 @@ is_pascal_string_type (struct type *type
                       struct type **char_type,
                       char **arrayname)
 {
-  if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
+  if ((type != NULL) && (TYPE_CODE (type) == TYPE_CODE_STRUCT))
     {
       /* Old Borland type pascal strings from Free Pascal Compiler.  */
       /* Two fields: length and st.  */
 2010-03-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
            Hui Zhu  <teawater@gmail.com>

Re: [PATCH] avoid GDB crash on inspection of pascal arrays

[Kevin Buettner]

On Mon, 8 Mar 2010 17:55:44 +0100
"Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr> wrote:

> 2020-03-08  Pierre Muller  <muller@ics.u-strasbg.fr>

s/2020/2010/

Kevin

Re: [PATCH] avoid GDB crash on inspection of pascal arrays

[Joel Brobecker]

Hi Pierre,

> 2020-03-08  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
>        * p-lang.c (is_pascal_string_type): Check that TYPE arg is non NULL.

Seems odd that you'd call a function whose job is to inspect a type with
a NULL type, but it's not hard to add a check indeed - and that would not.
be the first time ;-). Please do not consider this an objection, just
"speaking" aloud...

Just one nit:

> -  if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
> +  if ((type != NULL) && (TYPE_CODE (type) == TYPE_CODE_STRUCT))

Would you mind removing the extra parenthesis around each block?
I'd like for the code to be as consistent as possible, to help
readability. It's a question of taste, and I don't agree with all
our rules, but I'd like for things to stay as consistent as possible...

While I'm sending you an email, I started looking at the call sites
for your function, to see if I could see why the function is called
with a NULL pointer, in case there was something obvious to be found.
Nothing obvious, but I noticed that some code in p-valprint might need
a little reformatting?

>      elttype = check_typedef (TYPE_TARGET_TYPE (type));
>        {
>          addr = unpack_pointer (type, valaddr + embedded_offset);
>        print_unpacked_pointer:
>          elttype = check_typedef (TYPE_TARGET_TYPE (type));
>
>          if (TYPE_CODE (elttype) == TYPE_CODE_FUNC)

(this is around line 153).

Something else that caught my attention, as well, is the following
statement is repeated twice:

    elttype = check_typedef (TYPE_TARGET_TYPE (type));

It looks like the first instance is really unnecessary now?
(I am guessing there was a "if" before the mis-indented curly
brace before, and that this "if" got removed, but not its body,
to keep the patch readable - although there is always the diff -w
option).  How about the curly brace themselves - since the block
does not introduce new local variables, it looks like it can go too.

-- 
Joel

RE: [PATCH] avoid GDB crash on inspection of pascal arrays

[Pierre Muller]

Whoops,
sorry, fixed now.

Pierre Muller

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Kevin Buettner
> Envoyé : Monday, March 08, 2010 7:44 PM
> À : gdb-patches@sourceware.org
> Cc : Pierre Muller
> Objet : Re: [PATCH] avoid GDB crash on inspection of pascal arrays
> 
> On Mon, 8 Mar 2010 17:55:44 +0100
> "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr> wrote:
> 
> > 2020-03-08  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> s/2020/2010/
> 
> Kevin

RE: [PATCH] avoid GDB crash on inspection of pascal arrays

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Monday, March 08, 2010 7:55 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [PATCH] avoid GDB crash on inspection of pascal arrays
> 
> Hi Pierre,
> 
> > 2020-03-08  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> >        * p-lang.c (is_pascal_string_type): Check that TYPE arg is non
> NULL.
> 
> Seems odd that you'd call a function whose job is to inspect a type
> with
> a NULL type, but it's not hard to add a check indeed - and that would
> not.
> be the first time ;-). Please do not consider this an objection, just
> "speaking" aloud...

  No, you are perfectly right that it doesn't
seem logical to call this function with a NULL type.

  I fact, that happen probably only because of the p-exp.y 
code, but I tought that adding this test was the safest anyhow.
 
> Just one nit:
> 
> > -  if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
> > +  if ((type != NULL) && (TYPE_CODE (type) == TYPE_CODE_STRUCT))
> 
> Would you mind removing the extra parenthesis around each block?

  Sorry, I am always confused about operator precedence here
(pascal 'AND' binary operator has a higher precedence than
comparison operators, which means that similar pascal code
means those extra parentheses...)

> I'd like for the code to be as consistent as possible, to help
> readability. It's a question of taste, and I don't agree with all
> our rules, but I'd like for things to stay as consistent as possible...
> 
> While I'm sending you an email, I started looking at the call sites
> for your function, to see if I could see why the function is called
> with a NULL pointer, in case there was something obvious to be found.
> Nothing obvious, but I noticed that some code in p-valprint might need
> a little reformatting?
> 
> >      elttype = check_typedef (TYPE_TARGET_TYPE (type));
> >        {
> >          addr = unpack_pointer (type, valaddr + embedded_offset);
> >        print_unpacked_pointer:
> >          elttype = check_typedef (TYPE_TARGET_TYPE (type));
> >
> >          if (TYPE_CODE (elttype) == TYPE_CODE_FUNC)
> 
> (this is around line 153).
> 
> Something else that caught my attention, as well, is the following
> statement is repeated twice:
> 
>     elttype = check_typedef (TYPE_TARGET_TYPE (type));
> 
> It looks like the first instance is really unnecessary now?
> (I am guessing there was a "if" before the mis-indented curly
> brace before, and that this "if" got removed, but not its body,
> to keep the patch readable - although there is always the diff -w
> option).  How about the curly brace themselves - since the block
> does not introduce new local variables, it looks like it can go too.

  Formatting with the tab/spaces conversion is still a nightmare
for me...

  I really don't know vi enough to reformat correctly
an almost 100 lines long block... Is there a neat way to do this just with
vi
or do I need something more powerful?

  Could someone tell me the best way?

Pierre

Re: [PATCH] avoid GDB crash on inspection of pascal arrays

[Joel Brobecker]

>   Formatting with the tab/spaces conversion is still a nightmare
> for me...

(you could get me started on a rambling about the rid^H^H^Huse of
tabs instead of spaces in our source code - I just can't get over
these tabs)

> I really don't know vi enough to reformat correctly an almost 100
> lines long block... Is there a neat way to do this just with vi
> or do I need something more powerful?

I think that the canonical tool for formatting is emacs. Each time
I mentioned the idea of getting rid of tabs, some said that the
formatting rules need to match what emacs does. For a GNU project,
it's probably fair.

Unfortunately, I don't remember emacs anymore. There has to be an
equivalent way in vim (auto-formatting with a single command), but
I don't know, so here is how I do it with vim:

  1. Get rid of all tabs first, they get in the way of selecting
     the columns I want to delete or add:
        :set expandtab
        select all the lines I want to reformat (shift-v)
        while the selection is still active, enter
            :'<,'>retab!
        (the '<,'> should appear automatically after you pressed :)
  2. Delete the columns you want to delete:
        select the rectangle you want to delete (ctrl-v)
        while the selection is active, press d
  3. Re-introduce the tabs:
        :set noexpandtab
        re-select all the lines to be tabified (shift-v)
        while the selection is active, enter:
            :'<,'>retab!

Attached is a couple of patches:
  p-valprint-reformat.diff: The reformat itself;
  p-valprint-reformat-w: The same diff, but with -w, to show that there
                         were no other real change except the removal
                         of the curly braces.
Can you test p-valprint-reformat.diff and then commit if it's OK?

Cheers,
-- 
Joel

Re: [PATCH] avoid GDB crash on inspection of pascal arrays

[Joel Brobecker]

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

> Attached is a couple of patches:

[with the patches, this time]

-- 
Joel

[-- Attachment #2: p-valprint-reformat.diff --]
[-- Type: text/x-diff, Size: 6921 bytes --]

diff --git a/gdb/p-valprint.c b/gdb/p-valprint.c
index 260b97d..3bc9c55 100644
--- a/gdb/p-valprint.c
+++ b/gdb/p-valprint.c
@@ -151,105 +151,105 @@ pascal_val_print (struct type *type, const gdb_byte *valaddr,
 	  break;
 	}
       elttype = check_typedef (TYPE_TARGET_TYPE (type));
-	{
-	  addr = unpack_pointer (type, valaddr + embedded_offset);
-	print_unpacked_pointer:
-	  elttype = check_typedef (TYPE_TARGET_TYPE (type));
-
-	  if (TYPE_CODE (elttype) == TYPE_CODE_FUNC)
-	    {
-	      /* Try to print what function it points to.  */
-	      print_address_demangle (gdbarch, addr, stream, demangle);
-	      /* Return value is irrelevant except for string pointers.  */
-	      return (0);
-	    }
 
-	  if (options->addressprint && options->format != 's')
-	    {
-	      fputs_filtered (paddress (gdbarch, addr), stream);
-	    }
+      addr = unpack_pointer (type, valaddr + embedded_offset);
+    print_unpacked_pointer:
+      elttype = check_typedef (TYPE_TARGET_TYPE (type));
 
-	  /* For a pointer to char or unsigned char, also print the string
-	     pointed to, unless pointer is null.  */
-	  if (((TYPE_LENGTH (elttype) == 1
-	       && (TYPE_CODE (elttype) == TYPE_CODE_INT
-		  || TYPE_CODE (elttype) == TYPE_CODE_CHAR))
-	      || ((TYPE_LENGTH (elttype) == 2 || TYPE_LENGTH (elttype) == 4)
-		  && TYPE_CODE (elttype) == TYPE_CODE_CHAR))
-	      && (options->format == 0 || options->format == 's')
-	      && addr != 0)
-	    {
-	      /* no wide string yet */
-	      i = val_print_string (elttype, addr, -1, stream, options);
-	    }
-	  /* also for pointers to pascal strings */
-	  /* Note: this is Free Pascal specific:
-	     as GDB does not recognize stabs pascal strings
-	     Pascal strings are mapped to records
-	     with lowercase names PM  */
-          if (is_pascal_string_type (elttype, &length_pos, &length_size,
-                                     &string_pos, &char_type, NULL)
-	      && addr != 0)
+      if (TYPE_CODE (elttype) == TYPE_CODE_FUNC)
+	{
+	  /* Try to print what function it points to.  */
+	  print_address_demangle (gdbarch, addr, stream, demangle);
+	  /* Return value is irrelevant except for string pointers.  */
+	  return (0);
+	}
+
+      if (options->addressprint && options->format != 's')
+	{
+	  fputs_filtered (paddress (gdbarch, addr), stream);
+	}
+
+      /* For a pointer to char or unsigned char, also print the string
+	 pointed to, unless pointer is null.  */
+      if (((TYPE_LENGTH (elttype) == 1
+	   && (TYPE_CODE (elttype) == TYPE_CODE_INT
+	      || TYPE_CODE (elttype) == TYPE_CODE_CHAR))
+	  || ((TYPE_LENGTH (elttype) == 2 || TYPE_LENGTH (elttype) == 4)
+	      && TYPE_CODE (elttype) == TYPE_CODE_CHAR))
+	  && (options->format == 0 || options->format == 's')
+	  && addr != 0)
+	{
+	  /* no wide string yet */
+	  i = val_print_string (elttype, addr, -1, stream, options);
+	}
+      /* also for pointers to pascal strings */
+      /* Note: this is Free Pascal specific:
+	 as GDB does not recognize stabs pascal strings
+	 Pascal strings are mapped to records
+	 with lowercase names PM  */
+      if (is_pascal_string_type (elttype, &length_pos, &length_size,
+				 &string_pos, &char_type, NULL)
+	  && addr != 0)
+	{
+	  ULONGEST string_length;
+	  void *buffer;
+	  buffer = xmalloc (length_size);
+	  read_memory (addr + length_pos, buffer, length_size);
+	  string_length = extract_unsigned_integer (buffer, length_size,
+						    byte_order);
+	  xfree (buffer);
+	  i = val_print_string (char_type ,addr + string_pos, string_length, stream, options);
+	}
+      else if (pascal_object_is_vtbl_member (type))
+	{
+	  /* print vtbl's nicely */
+	  CORE_ADDR vt_address = unpack_pointer (type, valaddr + embedded_offset);
+
+	  struct minimal_symbol *msymbol =
+	  lookup_minimal_symbol_by_pc (vt_address);
+	  if ((msymbol != NULL)
+	      && (vt_address == SYMBOL_VALUE_ADDRESS (msymbol)))
 	    {
-	      ULONGEST string_length;
-              void *buffer;
-              buffer = xmalloc (length_size);
-              read_memory (addr + length_pos, buffer, length_size);
-	      string_length = extract_unsigned_integer (buffer, length_size,
-							byte_order);
-              xfree (buffer);
-              i = val_print_string (char_type ,addr + string_pos, string_length, stream, options);
+	      fputs_filtered (" <", stream);
+	      fputs_filtered (SYMBOL_PRINT_NAME (msymbol), stream);
+	      fputs_filtered (">", stream);
 	    }
-	  else if (pascal_object_is_vtbl_member (type))
+	  if (vt_address && options->vtblprint)
 	    {
-	      /* print vtbl's nicely */
-	      CORE_ADDR vt_address = unpack_pointer (type, valaddr + embedded_offset);
+	      struct value *vt_val;
+	      struct symbol *wsym = (struct symbol *) NULL;
+	      struct type *wtype;
+	      struct block *block = (struct block *) NULL;
+	      int is_this_fld;
+
+	      if (msymbol != NULL)
+		wsym = lookup_symbol (SYMBOL_LINKAGE_NAME (msymbol), block,
+				      VAR_DOMAIN, &is_this_fld);
 
-	      struct minimal_symbol *msymbol =
-	      lookup_minimal_symbol_by_pc (vt_address);
-	      if ((msymbol != NULL)
-		  && (vt_address == SYMBOL_VALUE_ADDRESS (msymbol)))
+	      if (wsym)
 		{
-		  fputs_filtered (" <", stream);
-		  fputs_filtered (SYMBOL_PRINT_NAME (msymbol), stream);
-		  fputs_filtered (">", stream);
+		  wtype = SYMBOL_TYPE (wsym);
 		}
-	      if (vt_address && options->vtblprint)
+	      else
 		{
-		  struct value *vt_val;
-		  struct symbol *wsym = (struct symbol *) NULL;
-		  struct type *wtype;
-		  struct block *block = (struct block *) NULL;
-		  int is_this_fld;
-
-		  if (msymbol != NULL)
-		    wsym = lookup_symbol (SYMBOL_LINKAGE_NAME (msymbol), block,
-					  VAR_DOMAIN, &is_this_fld);
-
-		  if (wsym)
-		    {
-		      wtype = SYMBOL_TYPE (wsym);
-		    }
-		  else
-		    {
-		      wtype = TYPE_TARGET_TYPE (type);
-		    }
-		  vt_val = value_at (wtype, vt_address);
-		  common_val_print (vt_val, stream, recurse + 1, options,
-				    current_language);
-		  if (options->pretty)
-		    {
-		      fprintf_filtered (stream, "\n");
-		      print_spaces_filtered (2 + 2 * recurse, stream);
-		    }
+		  wtype = TYPE_TARGET_TYPE (type);
+		}
+	      vt_val = value_at (wtype, vt_address);
+	      common_val_print (vt_val, stream, recurse + 1, options,
+				current_language);
+	      if (options->pretty)
+		{
+		  fprintf_filtered (stream, "\n");
+		  print_spaces_filtered (2 + 2 * recurse, stream);
 		}
 	    }
-
-	  /* Return number of characters printed, including the terminating
-	     '\0' if we reached the end.  val_print_string takes care including
-	     the terminating '\0' if necessary.  */
-	  return i;
 	}
+
+      /* Return number of characters printed, including the terminating
+	 '\0' if we reached the end.  val_print_string takes care including
+	 the terminating '\0' if necessary.  */
+      return i;
+
       break;
 
     case TYPE_CODE_REF:

[-- Attachment #3: p-valprint-reformat-w.diff --]
[-- Type: text/x-diff, Size: 691 bytes --]

diff --git a/gdb/p-valprint.c b/gdb/p-valprint.c
index 260b97d..3bc9c55 100644
--- a/gdb/p-valprint.c
+++ b/gdb/p-valprint.c
@@ -151,7 +151,7 @@ pascal_val_print (struct type *type, const gdb_byte *valaddr,
 	  break;
 	}
       elttype = check_typedef (TYPE_TARGET_TYPE (type));
-	{
+
 	  addr = unpack_pointer (type, valaddr + embedded_offset);
 	print_unpacked_pointer:
 	  elttype = check_typedef (TYPE_TARGET_TYPE (type));
@@ -249,7 +249,7 @@ pascal_val_print (struct type *type, const gdb_byte *valaddr,
 	     '\0' if we reached the end.  val_print_string takes care including
 	     the terminating '\0' if necessary.  */
 	  return i;
-	}
+
       break;
 
     case TYPE_CODE_REF:

RE: [PATCH] avoid GDB crash on inspection of pascal arrays

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Tuesday, March 09, 2010 6:17 AM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [PATCH] avoid GDB crash on inspection of pascal arrays
> 
> >   Formatting with the tab/spaces conversion is still a nightmare
> > for me...
> 
> (you could get me started on a rambling about the rid^H^H^Huse of
> tabs instead of spaces in our source code - I just can't get over
> these tabs)
> 
> > I really don't know vi enough to reformat correctly an almost 100
> > lines long block... Is there a neat way to do this just with vi
> > or do I need something more powerful?
> 
> I think that the canonical tool for formatting is emacs. Each time
> I mentioned the idea of getting rid of tabs, some said that the
> formatting rules need to match what emacs does. For a GNU project,
> it's probably fair.
> 
> Unfortunately, I don't remember emacs anymore. There has to be an
> equivalent way in vim (auto-formatting with a single command), but
> I don't know, so here is how I do it with vim:
> 
>   1. Get rid of all tabs first, they get in the way of selecting
>      the columns I want to delete or add:
>         :set expandtab
>         select all the lines I want to reformat (shift-v)
>         while the selection is still active, enter
>             :'<,'>retab!
>         (the '<,'> should appear automatically after you pressed :)
>   2. Delete the columns you want to delete:
>         select the rectangle you want to delete (ctrl-v)
>         while the selection is active, press d
>   3. Re-introduce the tabs:
>         :set noexpandtab
>         re-select all the lines to be tabified (shift-v)
>         while the selection is active, enter:
>             :'<,'>retab!

  This is quite tricky and I will need time to learn this ...
Thanks for sharing this!


> Attached is a couple of patches:
>   p-valprint-reformat.diff: The reformat itself;
>   p-valprint-reformat-w: The same diff, but with -w, to show that there
>                          were no other real change except the removal
>                          of the curly braces.
> Can you test p-valprint-reformat.diff and then commit if it's OK?

  Checked and committed
http://sourceware.org/ml/gdb-cvs/2010-03/msg00079.html

  Thanks Joel,

Pierre

Re: [PATCH] avoid GDB crash on inspection of pascal arrays

[Eli Zaretskii]

> Date: Tue, 9 Mar 2010 09:16:51 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> >   Formatting with the tab/spaces conversion is still a nightmare
> > for me...
> 
> (you could get me started on a rambling about the rid^H^H^Huse of
> tabs instead of spaces in our source code - I just can't get over
> these tabs)
> 
> > I really don't know vi enough to reformat correctly an almost 100
> > lines long block... Is there a neat way to do this just with vi
> > or do I need something more powerful?
> 
> I think that the canonical tool for formatting is emacs. Each time
> I mentioned the idea of getting rid of tabs, some said that the
> formatting rules need to match what emacs does. For a GNU project,
> it's probably fair.
> 
> Unfortunately, I don't remember emacs anymore.

If it helps someone, here's the Emacs recipe for converting all tabs
into the equivalent number of spaces:

  . Step 1: type "M-x set-variable RET tab-width RET 8 RET"

  . Step 2: mark the region of text where you want to get rid of tabs;
    if that's the whole buffer, type "C-x h" to mark all of it

  . Step 3: type "M-x untabify RET", then save the buffer

The first step makes sure each tab stop is 8 columns, the default
width of a TAB character.  It is there because some people (and some
files) override that default, and Emacs will honor such settings by
expanding each tab into that number of spaces.

Re: [PATCH] avoid GDB crash on inspection of pascal arrays

[Joel Brobecker]

> If it helps someone, here's the Emacs recipe for converting all tabs
> into the equivalent number of spaces:

Wasn't there a way to simply get emacs to reformat automatically?
I kind of remember something like ctrl-tab, or something like that
would reformat the current line. So if you select a region, and then
apply ctrl-tab, the whole region would be reformatted...

-- 
Joel

Re: [PATCH] avoid GDB crash on inspection of pascal arrays

[Eli Zaretskii]

> Date: Tue, 9 Mar 2010 21:55:56 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: pierre.muller@ics-cnrs.unistra.fr, gdb-patches@sourceware.org
> 
> > If it helps someone, here's the Emacs recipe for converting all tabs
> > into the equivalent number of spaces:
> 
> Wasn't there a way to simply get emacs to reformat automatically?
> I kind of remember something like ctrl-tab, or something like that
> would reformat the current line. So if you select a region, and then
> apply ctrl-tab, the whole region would be reformatted...

You are talking about Ctrl-Alt-\ (or "C-M-\" in Emacs notation).  But
it only re-indents, it does not untabify.

Re: [PATCH] avoid GDB crash on inspection of pascal arrays

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> If it helps someone, here's the Emacs recipe for converting all tabs
>> into the equivalent number of spaces:

Joel> Wasn't there a way to simply get emacs to reformat automatically?
Joel> I kind of remember something like ctrl-tab, or something like that
Joel> would reformat the current line. So if you select a region, and then
Joel> apply ctrl-tab, the whole region would be reformatted...

You can use indent-region to indent the current region.
This is bound to M-C-\ by default.
TAB will reindent the current line.

Tom