[RFC] DW_CFA_restore handling causes memory fault

[RFC] DW_CFA_restore handling causes memory fault

[Frederic RISS]

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

Hello,

The current handling of DW_CFA_restore in dwarf2-frame.c doesn't check
if the value it tries to restore has actually been allocated. This
produces strange results (from undeterministic behavour to a GDB crash).
The attached patch tries to fix that by following the GCC 'convention'
that an unspecified register implies "same value".

It's debatable wether the compiler is right to produce DW_CFA_restore
without specifying all the registers initial state in the CIE, but
that's another story, isn't it ?

Regards,
Fred.

[-- Attachment #2: dwarf2.patch --]
[-- Type: text/x-patch, Size: 707 bytes --]

2005-11-17  Frédéric Riss  <frederic.riss@st.com>
        
                * dwarf2-frame.c: (execute_cfa_program): Don't access 
		past the allocated dwarf2_frame_state.initial.regs.


Index: dwarf2-frame.c
===================================================================
--- dwarf2-frame.c	(revision 98)
+++ dwarf2-frame.c	(working copy)
@@ -294,7 +294,10 @@
 	  gdb_assert (fs->initial.reg);
 	  reg = insn & 0x3f;
 	  dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	  fs->regs.reg[reg] = fs->initial.reg[reg];
+	  if (reg < fs->initial.num_regs)
+	      fs->regs.reg[reg] = fs->initial.reg[reg];
+	  else 
+	      fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
 	}
       else
 	{

Re: [RFC] DW_CFA_restore handling causes memory fault

[Jim Blandy]

On 11/17/05, Frederic RISS <frederic.riss@st.com> wrote:
> The current handling of DW_CFA_restore in dwarf2-frame.c doesn't check
> if the value it tries to restore has actually been allocated. This
> produces strange results (from undeterministic behavour to a GDB crash).
> The attached patch tries to fix that by following the GCC 'convention'
> that an unspecified register implies "same value".

This replicates what we would have done had that register's slot been
allocated, but its value had been left unspecified --- right?

> It's debatable wether the compiler is right to produce DW_CFA_restore
> without specifying all the registers initial state in the CIE, but
> that's another story, isn't it ?

Right; we're supposed to be prepared for mis-formed input.  It would
be nice to have a brief comment explaining that the 'else' half of the
'if' does constitute questionable behavior on the part of the
compiler.

Re: [RFC] DW_CFA_restore handling causes memory fault

[Frederic RISS]

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

On Thu, 2005-11-17 at 11:32 -0800, Jim Blandy wrote:
> On 11/17/05, Frederic RISS <frederic.riss@st.com> wrote:
> > The current handling of DW_CFA_restore in dwarf2-frame.c doesn't check
> > if the value it tries to restore has actually been allocated. This
> > produces strange results (from undeterministic behavour to a GDB crash).
> > The attached patch tries to fix that by following the GCC 'convention'
> > that an unspecified register implies "same value".
> 
> This replicates what we would have done had that register's slot been
> allocated, but its value had been left unspecified --- right?

Yes, that's it. DWARF2_FRAME_REG_UNSPECIFIED describes an entry without
information, which is the case if we haven't allocated a slot in the
initial dwarf2_frame_state_reg_info. It turns out that GDB handles that
like DWARF2_FRAME_REG_SAME_VALUE in the unwinder, because that's what
GCC expects.

> > It's debatable wether the compiler is right to produce DW_CFA_restore
> > without specifying all the registers initial state in the CIE, but
> > that's another story, isn't it ?
> 
> Right; we're supposed to be prepared for mis-formed input.  It would
> be nice to have a brief comment explaining that the 'else' half of the
> 'if' does constitute questionable behavior on the part of the
> compiler.

Like in the attached patch ?


[-- Attachment #2: dwarf2.patch --]
[-- Type: text/x-patch, Size: 883 bytes --]

2005-11-17  Frédéric Riss  <frederic.riss@st.com>
        
                * dwarf2-frame.c: (execute_cfa_program): Don't access 
		past the allocated dwarf2_frame_state.initial.regs.


Index: dwarf2-frame.c
===================================================================
--- dwarf2-frame.c	(revision 98)
+++ dwarf2-frame.c	(working copy)
@@ -294,7 +294,13 @@
 	  gdb_assert (fs->initial.reg);
 	  reg = insn & 0x3f;
 	  dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	  fs->regs.reg[reg] = fs->initial.reg[reg];
+	  if (reg < fs->initial.num_regs)
+	      fs->regs.reg[reg] = fs->initial.reg[reg];
+	  else 
+	      /* We certainly shouldn't get here. If we do, then the compiler
+		 generated frame information asking for the restoration
+		 of something that wasn't initialized. */
+	      fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
 	}
       else
 	{

Re: [RFC] DW_CFA_restore handling causes memory fault

[Jim Blandy]

On 11/18/05, Frederic RISS <frederic.riss@st.com> wrote:
> > Right; we're supposed to be prepared for mis-formed input.  It would
> > be nice to have a brief comment explaining that the 'else' half of the
> > 'if' does constitute questionable behavior on the part of the
> > compiler.
>
> Like in the attached patch ?

Yes!  But --- I apologize for not noticing this from the beginning ---
as I see your comment, it occurs to me that probably a call to
'complaint' would be better than just a comment.  Mark or Daniel can
string me up if they feel this will yield another 'incomplete CFI
data; unspecified blah blah blah' annoyance, but I think it's probably
the right thing.

With this change, I think it's okay to commit, so let's get the
paperwork out of the way.  If you're doing this as part of your paying
job, then the assignment from STMicroelectronics already on file
covers your work, so you just need to get an account on
sourceware.org, add yourself to the "write after approval" list in
gdb/MAINTAINERS, and then you can commit the change.

For the sourceware account, see
http://sourceware.org/cgi-bin/pdw/ps_form.cgi.  List me as the person
approving the request.

Re: [RFC] DW_CFA_restore handling causes memory fault

[Frederic RISS]

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

On Fri, 2005-11-18 at 00:35 -0800, Jim Blandy wrote:
> Yes!  But --- I apologize for not noticing this from the beginning ---
> as I see your comment, it occurs to me that probably a call to
> 'complaint' would be better than just a comment.  Mark or Daniel can
> string me up if they feel this will yield another 'incomplete CFI
> data; unspecified blah blah blah' annoyance, but I think it's probably
> the right thing.

OK, third try attached. I mimiced the formatting of other complaints of
the file. I didn't put the complaint in the 'else' clause, but added a
check after the whole 'if', because an allocated register state isn't
necessarily set to a correct value.

Just a question: why are the i18n'ified strings formatted this way ?

>  If you're doing this as part of your paying
> job, then the assignment from STMicroelectronics already on file

That's the case.

> For the sourceware account, see
> http://sourceware.org/cgi-bin/pdw/ps_form.cgi.  List me as the person
> approving the request.

I'll do the request this evening (setting up the CVS access from work
will take some time, I'll use my home box for this patch.)

Thanks,
Fred.


[-- Attachment #2: dwarf2.patch --]
[-- Type: text/x-patch, Size: 917 bytes --]

2005-11-17  Frédéric Riss  <frederic.riss@st.com>
        
                * dwarf2-frame.c: (execute_cfa_program): Don't access 
		past the allocated dwarf2_frame_state.initial.regs.


Index: dwarf2-frame.c
===================================================================
--- dwarf2-frame.c	(revision 98)
+++ dwarf2-frame.c	(working copy)
@@ -294,8 +294,16 @@
 	  gdb_assert (fs->initial.reg);
 	  reg = insn & 0x3f;
 	  dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	  fs->regs.reg[reg] = fs->initial.reg[reg];
+	  if (reg < fs->initial.num_regs)
+	    fs->regs.reg[reg] = fs->initial.reg[reg];
+	  else 
+	    fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
+
+	  if (fs->regs.reg[reg].how == DWARF2_FRAME_REG_UNSPECIFIED)
+	    complaint (&symfile_complaints, _("\
+incomplete CFI data; DW_CFA_restore of unspecified register state at 0x%s."),
+		       paddr (fs->pc));
 	}
       else
 	{

Re: [RFC] DW_CFA_restore handling causes memory fault

[Mark Kettenis]

> X-From_: gdb-patches-return-41536-m.m.kettenis=alumnus.utwente.nl@sourceware.org  Fri Nov 18 13:38:27 2005
> From: Frederic RISS <frederic.riss@st.com>
> Date: Fri, 18 Nov 2005 13:33:03 +0100
> 
> On Fri, 2005-11-18 at 00:35 -0800, Jim Blandy wrote:
> > Yes!  But --- I apologize for not noticing this from the beginning ---
> > as I see your comment, it occurs to me that probably a call to
> > 'complaint' would be better than just a comment.  Mark or Daniel can
> > string me up if they feel this will yield another 'incomplete CFI
> > data; unspecified blah blah blah' annoyance, but I think it's probably
> > the right thing.

On the highest tree!  Seriously though, if it turns out to be a major
annoyance, we can always do something about it later.  So this is fine
to me ;-).

> OK, third try attached. I mimiced the formatting of other complaints of
> the file. I didn't put the complaint in the 'else' clause, but added a
> check after the whole 'if', because an allocated register state isn't
> necessarily set to a correct value.

That could possibly lead to two complaints about the same
"unspecified" register, but let's see what happens with this patch.

> Just a question: why are the i18n'ified strings formatted this way ?

Formatted in what way?  Starting in column 0?  Well, that's because
it's easier to see whether the string will be longer than 80
characters, which your string will be once the %s is filled in.  So
could you split the string over two lines?  You're going to have to do
that anyway, because I'm going to ask you to make a little change: can
you also print the register number for the register that the complaint
is about?

Thanks,

Mark

Re: [RFC] DW_CFA_restore handling causes memory fault

[Frederic RISS]

On Fri, 2005-11-18 at 14:45 +0100, Mark Kettenis wrote:
> > X-From_: gdb-patches-return-41536-m.m.kettenis=alumnus.utwente.nl@sourceware.org  Fri Nov 18 13:38:27 2005
> > From: Frederic RISS <frederic.riss@st.com>
> > Date: Fri, 18 Nov 2005 13:33:03 +0100
> > 
> > On Fri, 2005-11-18 at 00:35 -0800, Jim Blandy wrote:
> > > Yes!  But --- I apologize for not noticing this from the beginning ---
> > > as I see your comment, it occurs to me that probably a call to
> > > 'complaint' would be better than just a comment.  Mark or Daniel can
> > > string me up if they feel this will yield another 'incomplete CFI
> > > data; unspecified blah blah blah' annoyance, but I think it's probably
> > > the right thing.
> 
> On the highest tree!  Seriously though, if it turns out to be a major
> annoyance, we can always do something about it later.  So this is fine
> to me ;-).

Anyway, complaints are turned off by default, so I don't see how that
could be a major annoyance to the lambda user.

> > OK, third try attached. I mimiced the formatting of other complaints of
> > the file. I didn't put the complaint in the 'else' clause, but added a
> > check after the whole 'if', because an allocated register state isn't
> > necessarily set to a correct value.
> 
> That could possibly lead to two complaints about the same
> "unspecified" register, but let's see what happens with this patch.

Yes, the patch will produce a complaint in the DW_CFA_restore handling;
and as a consequence of having an UNSPECIFIED register, the complaint in
dwarf2_frame_cache will also trigger. 

> > Just a question: why are the i18n'ified strings formatted this way ?
> 
> Formatted in what way?  Starting in column 0?  Well, that's because
> it's easier to see whether the string will be longer than 80
> characters, which your string will be once the %s is filled in.

Thanks for the explanation.

> So
> could you split the string over two lines?  You're going to have to do
> that anyway, because I'm going to ask you to make a little change: can
> you also print the register number for the register that the complaint
> is about?

Initialy, I didn't put the register number because I felt there was an
issue with the line going over 80 chars :-) Didn't think about the %s
expansion, though... So what do you prefer to describe the register :
its dwarf number, its gdb regnum or its name ? 

Cheers, 
Fred.

Re: [RFC] DW_CFA_restore handling causes memory fault

[Frederic RISS]

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

On Fri, 2005-11-18 at 14:45 +0100, Mark Kettenis wrote:
> Formatted in what way?  Starting in column 0?  Well, that's because
> it's easier to see whether the string will be longer than 80
> characters, which your string will be once the %s is filled in. 

I just noticed that other complaints of the file will wrap because
symfile complaints are prefixed with 'During symbol reading, '. I put
the line break in my patch so that the first line is no longer that 80
chars with the prefix.

>  So
> could you split the string over two lines?  You're going to have to do
> that anyway, because I'm going to ask you to make a little change: can
> you also print the register number for the register that the complaint
> is about?

Here's the latest version of the patch with the regnum printed out. Is
it ok to commit ?

Fred.

[-- Attachment #2: dwarf2_cfa_restore.patch --]
[-- Type: text/x-patch, Size: 959 bytes --]

2005-11-23  Frédéric Riss  <frederic.riss@st.com>
        
                * dwarf2-frame.c: (execute_cfa_program): Don't access 
		past the allocated dwarf2_frame_state.initial.regs.


--- dwarf2-frame.c.orig	2005-11-23 10:19:31.000000000 +0100
+++ dwarf2-frame.c	2005-11-23 10:28:22.000000000 +0100
@@ -294,7 +294,16 @@ execute_cfa_program (gdb_byte *insn_ptr,
 	  gdb_assert (fs->initial.reg);
 	  reg = insn & 0x3f;
 	  dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	  fs->regs.reg[reg] = fs->initial.reg[reg];
+	  if (reg < fs->initial.num_regs)
+	    fs->regs.reg[reg] = fs->initial.reg[reg];
+	  else 
+	    fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
+
+	  if (fs->regs.reg[reg].how == DWARF2_FRAME_REG_UNSPECIFIED)
+	    complaint (&symfile_complaints, _(
+"incomplete CFI data; DW_CFA_restore of unspecified\n"
+"registers (e.g., regnum %i) at 0x%s"),
+		       DWARF2_REG_TO_REGNUM(reg), paddr (fs->pc));
 	}
       else
 	{

Re: [RFC] DW_CFA_restore handling causes memory fault

[Mark Kettenis]

> From: Frederic RISS <frederic.riss@st.com>
> Date: Wed, 23 Nov 2005 10:35:31 +0100
> 
> On Fri, 2005-11-18 at 14:45 +0100, Mark Kettenis wrote:
> > Formatted in what way?  Starting in column 0?  Well, that's because
> > it's easier to see whether the string will be longer than 80
> > characters, which your string will be once the %s is filled in. 
> 
> I just noticed that other complaints of the file will wrap because
> symfile complaints are prefixed with 'During symbol reading, '. I put
> the line break in my patch so that the first line is no longer that 80
> chars with the prefix.
> 
> >  So
> > could you split the string over two lines?  You're going to have to do
> > that anyway, because I'm going to ask you to make a little change: can
> > you also print the register number for the register that the complaint
> > is about?
> 
> Here's the latest version of the patch with the regnum printed out. Is
> it ok to commit ?

Sorry to be such a prick, but could you change the message in

          complaint (&symfile_complaints, _("\
incomplete CFI data; DW_CFA_restore unspecified\n\
register %s (#%d) at 0x%s"),
                     REGISTER_NAME(DWARF2_REG_TO_REGNUM(reg)),
                     DWARF2_REG_TO_REGNUM(reg), paddr (fs->pc));

I think that's less confusing.  With that change you've got my ok.
You're not listed in MAINTAINERS though.  Do you've got a copyright
assignment in place?

Mark

Re: [RFC] DW_CFA_restore handling causes memory fault

[Frederic RISS]

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

On Thu, 2005-11-24 at 23:48 +0100, Mark Kettenis wrote:
> Sorry to be such a prick, but could you change the message 
[...]
> I think that's less confusing.  With that change you've got my ok.

Well I'm sorry that you have to rewrite my patch yourself :-)

I commited the attached patch containing your message to HEAD. Should
that go to the 6.4 branch also ? BTW, what's the policy for patches
being approved during the release process, do they have implicit
approval for HEAD and the branch or does the latter require explicit
approval ?

> You're not listed in MAINTAINERS though.  Do you've got a copyright
> assignment in place?

I've added myself to the 'Write after approval' section as Jim
requested. My work is covered by the STMicroelectronics copyright
assignment.
 
Fred.

[-- Attachment #2: dwarf2_cfa_restore.patch --]
[-- Type: text/x-patch, Size: 1140 bytes --]

2005-11-25 Frederic Riss <frederic.riss@st.com>
 	
	* dwarf2-frame.c: (execute_cfa_program): Don't access past the 
	allocated dwarf2_frame_state.initial.regs.

Index: dwarf2-frame.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2-frame.c,v
retrieving revision 1.54
diff -u -p -r1.54 dwarf2-frame.c
--- dwarf2-frame.c	1 Aug 2005 04:06:27 -0000	1.54
+++ dwarf2-frame.c	25 Nov 2005 06:40:31 -0000
@@ -294,7 +294,17 @@ execute_cfa_program (gdb_byte *insn_ptr,
 	  gdb_assert (fs->initial.reg);
 	  reg = insn & 0x3f;
 	  dwarf2_frame_state_alloc_regs (&fs->regs, reg + 1);
-	  fs->regs.reg[reg] = fs->initial.reg[reg];
+	  if (reg < fs->initial.num_regs)
+	    fs->regs.reg[reg] = fs->initial.reg[reg];
+	  else 
+	    fs->regs.reg[reg].how = DWARF2_FRAME_REG_UNSPECIFIED;
+
+	  if (fs->regs.reg[reg].how == DWARF2_FRAME_REG_UNSPECIFIED)
+	    complaint (&symfile_complaints, _("\
+incomplete CFI data; DW_CFA_restore unspecified\n\
+register %s (#%d) at 0x%s"),
+		       REGISTER_NAME(DWARF2_REG_TO_REGNUM(reg)),
+		       DWARF2_REG_TO_REGNUM(reg), paddr (fs->pc));
 	}
       else
 	{

Re: [RFC] DW_CFA_restore handling causes memory fault

[Daniel Jacobowitz]

On Fri, Nov 25, 2005 at 11:19:32AM +0100, Frederic RISS wrote:
> On Thu, 2005-11-24 at 23:48 +0100, Mark Kettenis wrote:
> > Sorry to be such a prick, but could you change the message 
> [...]
> > I think that's less confusing.  With that change you've got my ok.
> 
> Well I'm sorry that you have to rewrite my patch yourself :-)
> 
> I commited the attached patch containing your message to HEAD. Should
> that go to the 6.4 branch also ? BTW, what's the policy for patches
> being approved during the release process, do they have implicit
> approval for HEAD and the branch or does the latter require explicit
> approval ?

It depends on where we are in the release I suppose; please ask Joel
if you want this patch in 6.4.

> 2005-11-25 Frederic Riss <frederic.riss@st.com>
>  	
> 	* dwarf2-frame.c: (execute_cfa_program): Don't access past the 
> 	allocated dwarf2_frame_state.initial.regs.

FYI, stray colon there.  I've removed it.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC] DW_CFA_restore handling causes memory fault

[Mark Kettenis]

> From: Frederic RISS <frederic.riss@st.com>
> Date: Fri, 25 Nov 2005 11:19:32 +0100
> 
> On Thu, 2005-11-24 at 23:48 +0100, Mark Kettenis wrote:
> > Sorry to be such a prick, but could you change the message 
> [...]
> > I think that's less confusing.  With that change you've got my ok.
> 
> Well I'm sorry that you have to rewrite my patch yourself :-)

No problem, I was feeling a bit guilty about not thinking about this
in the first place.

> I commited the attached patch containing your message to HEAD. Should
> that go to the 6.4 branch also ? BTW, what's the policy for patches
> being approved during the release process, do they have implicit
> approval for HEAD and the branch or does the latter require explicit
> approval ?

I'm not sure the branch is still open, but yes I think this should go
into 6.4 or 6.4.1.  Joel?

Re: [RFC] DW_CFA_restore handling causes memory fault

[Joel Brobecker]

> I'm not sure the branch is still open, but yes I think this should go
> into 6.4 or 6.4.1.  Joel?

Yes, the branch is open for more commits, especially when they fix
a memory fault. Next tarball, which I expect will be 6.4 will be
created on Nov 30th.

-- 
Joel

Re: [RFC] DW_CFA_restore handling causes memory fault

[Frederic RISS]

On Fri, 2005-11-25 at 13:42 -0800, Joel Brobecker wrote:
> > I'm not sure the branch is still open, but yes I think this should go
> > into 6.4 or 6.4.1.  Joel?
> 
> Yes, the branch is open for more commits, especially when they fix
> a memory fault. Next tarball, which I expect will be 6.4 will be
> created on Nov 30th.

OK. The patch has been commited to the branch.

Re: [RFC] DW_CFA_restore handling causes memory fault

[Eli Zaretskii]

> From: Frederic RISS <frederic.riss@st.com>
> Cc: gdb-patches@sources.redhat.com
> Date: Fri, 18 Nov 2005 13:33:03 +0100
> 
> Just a question: why are the i18n'ified strings formatted this way ?

That special format marks translatable strings for Gettext utilities
that extract such strings into a string catalog.

Re: [RFC] DW_CFA_restore handling causes memory fault

[Frederic RISS]

On Fri, 2005-11-18 at 15:49 +0200, Eli Zaretskii wrote:
> > From: Frederic RISS <frederic.riss@st.com>
> > Cc: gdb-patches@sources.redhat.com
> > Date: Fri, 18 Nov 2005 13:33:03 +0100
> > 
> > Just a question: why are the i18n'ified strings formatted this way ?
> 
> That special format marks translatable strings for Gettext utilities
> that extract such strings into a string catalog.

Yeah, I'm quite familiar with gettext (did some translation work). I
wondered why they were all beginning at column 0, because I haden't seen
that in any other project. I've just seen Mark's explaination on this.

Regards,
Fred.