Fwd: [patch]: Fix crash in objc and breakpoints

Fwd: [patch]: Fix crash in objc and breakpoints

[Kai Tietz]

ups, wrong mailing list.


---------- Forwarded message ----------
From: Kai Tietz <Kai.Tietz@onevision.com>
Date: 2010/2/18
Subject: [patch]: Fix crash in objc and breakpoints
To: gdb@sourceware.org
Cc: Joel Brobecker <brobecker@adacore.com>


Hello,

Sorry, that I came that late to this subject, but I was pretty busy last
weeks. As discussed in thread " [gdb-7.1] 10 days to branching..." there
are troubles about setting breakpoints in gdb.  As I found is the issue
related to recent use of init_sal function and missing initialization of
pspace member.

The following patch solves this for me. I regression tested it for
x86_64-pc-mingw32, i686-pc-linux, and i686-pc-cygwin without any
regressions.

2010-02-18 Kai Tietz  <kai.tietz@onevision.com>

       * source.c (line_info): Initialize pspace by default
       current_program_space.
       * frame.c (find_frame_sal): Likewise.
       * linespec.c (decode_line_2): Likewise.
       (decode_objc): Likewise.

Ok for apply?

Regards,
Kai

|  (\_/)  This is Bunny. Copy and paste Bunny
| (='.'=) into your signature to help him gain
| (")_(") world domination.

--------

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c        2010-01-29 16:28:43.000000000 +0100
+++ src/gdb/frame.c     2010-02-18 10:49:42.745803800 +0100
@@ -1857,6 +1857,8 @@
          the call site is.  Do not pretend to.  This is jarring, but
          we can't do much better.  */
       sal->pc = get_frame_pc (frame);
+      /* Initialize pspace by default.  */
+      sal->pspace = current_program_space;

      return;
    }
Index: src/gdb/linespec.c
===================================================================
--- src.orig/gdb/linespec.c     2010-02-18 10:41:31.000000000 +0100
+++ src/gdb/linespec.c  2010-02-18 10:52:50.980178800 +0100
@@ -513,7 +513,9 @@
  while (i < nelts)
    {
      init_sal (&return_values.sals[i]);       /* Initialize to zeroes.
*/
+      return_values.sals[i].pspace = current_program_space;
      init_sal (&values.sals[i]);
+      values.sals[i].pspace = current_program_space;
      if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
       values.sals[i] = find_function_start_sal (sym_arr[i],
funfirstline);
      i++;
@@ -1206,6 +1208,7 @@
                                                  &current_target);

         init_sal (&values.sals[0]);
+         values.sals[0].pspace = current_program_space;
         values.sals[0].pc = pc;
       }
      return values;
Index: src/gdb/source.c
===================================================================
--- src.orig/gdb/source.c       2010-01-12 16:54:43.000000000 +0100
+++ src/gdb/source.c    2010-02-18 10:46:36.183303800 +0100
@@ -1467,6 +1467,7 @@
  int i;

  init_sal (&sal);             /* initialize to zeroes */
+  sal.pspace = current_program_space; /* initialize as default.  */

  if (arg == 0)
    {




-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: Fwd: [patch]: Fix crash in objc and breakpoints

[Pedro Alves]

On Thursday 18 February 2010 19:32:11, Kai Tietz wrote:

>        * source.c (line_info): Initialize pspace by default
>        current_program_space.
>        * frame.c (find_frame_sal): Likewise.
>        * linespec.c (decode_line_2): Likewise.
>        (decode_objc): Likewise.
> 
> Ok for apply?

Sorry, not yet.

The patch is mangled and I can't apply it as is,
but I'll try to make an effort to read it anyway.

Could you please, please also use (cvs) diff's -p switch
for future patches?  It makes diffs so much more readable.
Thanks.


> 
> Index: src/gdb/frame.c
> ===================================================================
> --- src.orig/gdb/frame.c        2010-01-29 16:28:43.000000000 +0100
> +++ src/gdb/frame.c     2010-02-18 10:49:42.745803800 +0100
> @@ -1857,6 +1857,8 @@
>           the call site is.  Do not pretend to.  This is jarring, but
>           we can't do much better.  */
>        sal->pc = get_frame_pc (frame);
> +      /* Initialize pspace by default.  */
> +      sal->pspace = current_program_space;
> 

Did you try frame->pspace?

>       return;
>     }
> Index: src/gdb/linespec.c
> ===================================================================
> --- src.orig/gdb/linespec.c     2010-02-18 10:41:31.000000000 +0100
> +++ src/gdb/linespec.c  2010-02-18 10:52:50.980178800 +0100
> @@ -513,7 +513,9 @@
>   while (i < nelts)
>     {
>       init_sal (&return_values.sals[i]);       /* Initialize to zeroes.
> */
> +      return_values.sals[i].pspace = current_program_space;
>       init_sal (&values.sals[i]);
> +      values.sals[i].pspace = current_program_space;
>       if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
>        values.sals[i] = find_function_start_sal (sym_arr[i],
> funfirstline);
>       i++;

Sorry, I don't like these workarounds.  Why was this needed? 
There must be a code path that didn't set the pspace on a
valid sal.  What was it?

I think you're patching this:

 i = 0;
  while (i < nelts)
    {
      init_sal (&return_values.sals[i]);	/* Initialize to zeroes.  */
      init_sal (&values.sals[i]);
      if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
	values.sals[i] = find_function_start_sal (sym_arr[i], funfirstline);
      i++;
    }

and find_function_start_sal should be returning a sal with
the correct pspace, so this must be about the sals that
aren't LOC_BLOCK?  What are those?  Below there's this

 while (i < nelts)
        {
          if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
            {
...
            }
          else
            printf_unfiltered (_("?HERE\n"));
          i++;

Are we hitting this?  Sounds like something similar to PR10966.  Does
the workaround that has been applied on the branch for this PR happen
to fix this?

> @@ -1206,6 +1208,7 @@
>                                                   &current_target);
> 
>          init_sal (&values.sals[0]);
> +         values.sals[0].pspace = current_program_space;
>          values.sals[0].pc = pc;
>        }
>       return values;



> Index: src/gdb/source.c
> ===================================================================
> --- src.orig/gdb/source.c       2010-01-12 16:54:43.000000000 +0100
> +++ src/gdb/source.c    2010-02-18 10:46:36.183303800 +0100
> @@ -1467,6 +1467,7 @@
>   int i;
> 
>   init_sal (&sal);             /* initialize to zeroes */
> +  sal.pspace = current_program_space; /* initialize as default.  */

Likewise.  Isn't this a problem with the `if (arg == 0)' branch below
only?  It looks like it.

> 
>   if (arg == 0)
>     {
> 
> 

Do you have a testcase (doesn't have to be in .exp form, just
something I could try) for these issues yet?  You seem to
be covering different problems with the same patch.

-- 
Pedro Alves

Re: Fwd: [patch]: Fix crash in objc and breakpoints

[Kai Tietz]

Hello Pedro,

2010/2/22 Pedro Alves <pedro@codesourcery.com>:
> On Thursday 18 February 2010 19:32:11, Kai Tietz wrote:
>
>>        * source.c (line_info): Initialize pspace by default
>>        current_program_space.
>>        * frame.c (find_frame_sal): Likewise.
>>        * linespec.c (decode_line_2): Likewise.
>>        (decode_objc): Likewise.
>>
>> Ok for apply?
>
> Sorry, not yet.
>
> The patch is mangled and I can't apply it as is,
> but I'll try to make an effort to read it anyway.
>
> Could you please, please also use (cvs) diff's -p switch
> for future patches?  It makes diffs so much more readable.
> Thanks.
>
Ok, I use in general quilt for patches, but of course I can sent them
in future by using cvs -p for diff.

>>
>> Index: src/gdb/frame.c
>> ===================================================================
>> --- src.orig/gdb/frame.c        2010-01-29 16:28:43.000000000 +0100
>> +++ src/gdb/frame.c     2010-02-18 10:49:42.745803800 +0100
>> @@ -1857,6 +1857,8 @@
>>           the call site is.  Do not pretend to.  This is jarring, but
>>           we can't do much better.  */
>>        sal->pc = get_frame_pc (frame);
>> +      /* Initialize pspace by default.  */
>> +      sal->pspace = current_program_space;
>>
>
> Did you try frame->pspace?

Yes, I did. But it hadn't solved the issue.

>>       return;
>>     }
>> Index: src/gdb/linespec.c
>> ===================================================================
>> --- src.orig/gdb/linespec.c     2010-02-18 10:41:31.000000000 +0100
>> +++ src/gdb/linespec.c  2010-02-18 10:52:50.980178800 +0100
>> @@ -513,7 +513,9 @@
>>   while (i < nelts)
>>     {
>>       init_sal (&return_values.sals[i]);       /* Initialize to zeroes.
>> */
>> +      return_values.sals[i].pspace = current_program_space;
>>       init_sal (&values.sals[i]);
>> +      values.sals[i].pspace = current_program_space;
>>       if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
>>        values.sals[i] = find_function_start_sal (sym_arr[i],
>> funfirstline);
>>       i++;
>
> Sorry, I don't like these workarounds.  Why was this needed?
> There must be a code path that didn't set the pspace on a
> valid sal.  What was it?
>
> I think you're patching this:
>
>  i = 0;
>  while (i < nelts)
>    {
>      init_sal (&return_values.sals[i]);        /* Initialize to zeroes.  */
>      init_sal (&values.sals[i]);
>      if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
>        values.sals[i] = find_function_start_sal (sym_arr[i], funfirstline);
>      i++;
>    }
>
> and find_function_start_sal should be returning a sal with
> the correct pspace, so this must be about the sals that
> aren't LOC_BLOCK?  What are those?  Below there's this
>
>  while (i < nelts)
>        {
>          if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
>            {
> ...
>            }
>          else
>            printf_unfiltered (_("?HERE\n"));
>          i++;
>
> Are we hitting this?  Sounds like something similar to PR10966.  Does
> the workaround that has been applied on the branch for this PR happen
> to fix this?

The patch in PR/10966 looks similar but didn't solved the issue, For
some of those place I added a default initialization of sal's pspace
are maybe superflous, but I saw the issue just remaining for obj-c.

I tested that the necessary patches to solve my issue are in frame.c
(find_frame_sal) and linespec.c (decode_line_2), The other seems to be
not really necessary.

>> @@ -1206,6 +1208,7 @@
>>                                                   &current_target);
>>
>>          init_sal (&values.sals[0]);
>> +         values.sals[0].pspace = current_program_space;
>>          values.sals[0].pc = pc;
>>        }
>>       return values;
>
>
>
>> Index: src/gdb/source.c
>> ===================================================================
>> --- src.orig/gdb/source.c       2010-01-12 16:54:43.000000000 +0100
>> +++ src/gdb/source.c    2010-02-18 10:46:36.183303800 +0100
>> @@ -1467,6 +1467,7 @@
>>   int i;
>>
>>   init_sal (&sal);             /* initialize to zeroes */
>> +  sal.pspace = current_program_space; /* initialize as default.  */
>
> Likewise.  Isn't this a problem with the `if (arg == 0)' branch below
> only?  It looks like it.
>
>>
>>   if (arg == 0)
>>     {
>>
>>
>
> Do you have a testcase (doesn't have to be in .exp form, just
> something I could try) for these issues yet?  You seem to
> be covering different problems with the same patch.

Sadly the test-scenario for this issue is a bit big. I'll try to make
a testcase for it, but AFAI had seen it happens for me with Obj-C and
shared object files. I hope it isn't a Windows specific thing, but I
wouldn't assume so.

Kai


-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: Fwd: [patch]: Fix crash in objc and breakpoints

[Daniel Jacobowitz]

On Mon, Feb 22, 2010 at 08:13:58PM +0100, Kai Tietz wrote:
> Ok, I use in general quilt for patches, but of course I can sent them
> in future by using cvs -p for diff.

FYI:

drow@caradoc:~% env|grep QUILT
QUILT_DIFF_OPTS=-p
QUILT_PATCH_OPTS=--unified-reject-files
QUILT_REFRESH_ARGS=--diffstat

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fwd: [patch]: Fix crash in objc and breakpoints

[Kai Tietz]

2010/2/22 Daniel Jacobowitz <dan@codesourcery.com>:
> On Mon, Feb 22, 2010 at 08:13:58PM +0100, Kai Tietz wrote:
>> Ok, I use in general quilt for patches, but of course I can sent them
>> in future by using cvs -p for diff.
>
> FYI:
>
> drow@caradoc:~% env|grep QUILT
> QUILT_DIFF_OPTS=-p
> QUILT_PATCH_OPTS=--unified-reject-files
> QUILT_REFRESH_ARGS=--diffstat
>
> --
> Daniel Jacobowitz
> CodeSourcery
>

Thanks :)

Kai
-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination