Use of uninitialised data in tc-sh.c

Use of uninitialised data in tc-sh.c

[Andrew STUBBS]

Hi Alexandre,

I have tested the assembler under valgrind and come up with the following
error:

==7730== Conditional jump or move depends on uninitialised value(s)
==7730==    at 0x8064D80: get_specific (src/gas/config/tc-sh.c:1576)
==7730==    by 0x80667A5: md_assemble (src/gas/config/tc-sh.c:2852)
==7730==    by 0x8058169: read_a_source_file (src/gas/read.c:889)
==7730==    by 0x804BB15: perform_an_assembly_pass (src/gas/as.c:1054)

Which corresponds to the code below:

          if (SH_MERGE_ARCH_SET_VALID (valid_arch, arch_sh2a_nofpu_up)
              && (   arg == A_DISP_REG_M
                  || arg == A_DISP_REG_N))
            {
              /* Check a few key IMM* fields for overflow.  */
              int opf;
              long val = user->immediate.X_add_number;

              for (opf = 0; opf < 4; opf ++)
                switch (this_try->nibbles[opf])
                  {
                  case IMM0_4:
                  case IMM1_4:
1576 ->             if (val < 0 || val > 15)
                      goto fail;
                    break;
                  case IMM0_4BY2:
                  case IMM1_4BY2:
                    if (val < 0 || val > 15 * 2)
                      goto fail;
                    break;
                  case IMM0_4BY4:
                  case IMM1_4BY4:
                    if (val < 0 || val > 15 * 4)
                      goto fail;
                    break;
                  default:
                    break;
                  }
            }

There are similar failures for the other two if statements.

This code was introduced during your recent SH-2A patch. The problem is that
the code is testing the 'immediate' field even when the operand is not of
immediate type, and thus when the immediate field has not been written.

It is not entirely clear to me what this code is trying to achieve. For
example, why is this range checking sh2a specific? And why only a few 'key'
fields? And which are the key ones anyway?

Please could you review what your patch was intended to do and sort it out.
It appears to be harmless (I haven't seen any valid programs rejected), but
I can't prove it to my satisfaction.

P.S. I think the type of 'val' really ought to be 'long long', to match the
type of 'X_add_number'.

Thanks

--
Andrew Stubbs
andrew.stubbs@st.com
andrew.stubbs@superh.com

Re: Use of uninitialised data in tc-sh.c

[Alexandre Oliva]

On Jan 12, 2005, Andrew STUBBS <andrew.stubbs@st.com> wrote:

> I have tested the assembler under valgrind and come up with the following
> error:

Do you have a testcase that triggers this error?  Thanks,

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

RE: Use of uninitialised data in tc-sh.c

[Andrew STUBBS]

> Do you have a testcase that triggers this error?  Thanks,

Apologies, I should have thought of that. My original testcase was from
plumhall so I couldn't include that, and I can't imagine any real program
not triggering the error.

It only takes one instruction to trigger it:

	mov.l r1,r2

does the trick, although I'm sure there are others.

I'm using valgrind 2.2.0 with --tool=memcheck and binutils 2.15.94.0.1

HTH

--
Andrew Stubbs
andrew.stubbs@st.com
andrew.stubbs@superh.com

Re: Use of uninitialised data in tc-sh.c

[Alexandre Oliva]

On Jan 18, 2005, Andrew STUBBS <andrew.stubbs@st.com> wrote:

> It only takes one instruction to trigger it:

> 	mov.l r1,r2

> does the trick, although I'm sure there are others.

Aah, thanks, I didn't realize it was so simple to trigger the problem.
I'll let DJ know (he wrote that part of the patch that I contributed).

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Use of uninitialised data in tc-sh.c

[DJ Delorie]

I think this solves it.  At least, it looks right, passes valgrind,
and passes make check.  Note that, for each case where this code
accidentally failed before, it ends up doing the right thing anyway.
(that's not an excuse, just noting this is a low priority patch)

2005-01-18  DJ Delorie  <dj@redhat.com>

	* config/tc-sh.c (get_specific): Only check limits when the
	operand matches the expected argument.

Index: config/tc-sh.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-sh.c,v
retrieving revision 1.103
diff -p -U3 -r1.103 tc-sh.c
--- config/tc-sh.c	17 Jan 2005 14:08:10 -0000	1.103
+++ config/tc-sh.c	18 Jan 2005 20:15:54 -0000
@@ -1562,7 +1562,9 @@ get_specific (sh_opcode_info *opcode, sh
 
 	  if (SH_MERGE_ARCH_SET_VALID (valid_arch, arch_sh2a_nofpu_up)
 	      && (   arg == A_DISP_REG_M
-		  || arg == A_DISP_REG_N))
+		  || arg == A_DISP_REG_N)
+	      && (   user->type == A_DISP_REG_M
+		  || user->type == A_DISP_REG_N))
 	    {
 	      /* Check a few key IMM* fields for overflow.  */
 	      int opf;

RE: Use of uninitialised data in tc-sh.c

[Andrew STUBBS]

> I think this solves it.  At least, it looks right, passes valgrind,
> and passes make check.  Note that, for each case where this code
> accidentally failed before, it ends up doing the right thing anyway.
> (that's not an excuse, just noting this is a low priority patch)

I have tested your patch and it does indeed pass both valgrind and plumhall.

I assume you have decided not to worry about the implicit long long to long
cast in the assignment of val.

--
Andrew Stubbs
andrew.stubbs@st.com
andrew.stubbs@superh.com

Re: Use of uninitialised data in tc-sh.c

[DJ Delorie]

> I assume you have decided not to worry about the implicit long long to long
> cast in the assignment of val.

I hadn't seen that warning yet.  It can be a long long type if you
wish.

RE: Use of uninitialised data in tc-sh.c

[Andrew STUBBS]

> > I assume you have decided not to worry about the implicit 
> long long to long
> > cast in the assignment of val.
> 
> I hadn't seen that warning yet.  It can be a long long type if you
> wish.

I haven't noticed whether there was a warning or not. I assume there was.

I noticed the discrepancy when I was tracking down the cause of the
uninitialised data with the debugger. I pointed it out in my original
posting so I assumed you were aware of it.

It is harmless for anybody other than SH2A users (AFAICT), and perhaps even
for them, so I'm not bothered one way or the other, but I thought I would
point it out.

Whatever you want. 

--
Andrew Stubbs
andrew.stubbs@st.com
andrew.stubbs@superh.com