Re: PATCH: PR target/38781: PR38151: valgrind finds problem

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Jan 13, 2009 at 10:53 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jan 13, 2009 at 1:46 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Jan 13, 2009 at 10:35 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Tue, Jan 13, 2009 at 1:33 PM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Tue, Jan 13, 2009 at 10:24 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Tue, Jan 13, 2009 at 1:17 PM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Tue, Jan 13, 2009 at 6:30 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> On Fri, Jan 9, 2009 at 9:36 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Zero sized arrays or structures are special cases for
>>>>>>>> x86-64. We shouldn't check classes on them. OK for
>>>>>>>> trunk?
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> H.J.
>>>>>>>> ---
>>>>>>>> 2008-01-09  H.J. Lu  <hongjiu.lu@intel.com>
>>>>>>>>
>>>>>>>>        PR target/38781
>>>>>>>>        * config/i386/i386.c (classify_argument): Check zero sized
>>>>>>>>        arrays or structures.
>>>>>>>>
>>>>>>>
>>>>>>> Ping.
>>>>>>
>>>>>> I don't see a testcase, so what is exactly the problem?
>>>>>>
>>>>>
>>>>> Valgrind finds that for gcc.c-torture/execute/pr38151.c:
>>>>>
>>>>> --
>>>>> void abort (void);
>>>>>
>>>>> struct S2848
>>>>> {
>>>>>  unsigned int a;
>>>>>  _Complex int b;
>>>>>  struct
>>>>>  {
>>>>>  } __attribute__ ((aligned)) c;
>>>>> };
>>>>>
>>>>> struct S2848 s2848;
>>>>>
>>>>> int fails;
>>>>>
>>>>> void  __attribute__((noinline))
>>>>> check2848va (int z, ...)
>>>>> {
>>>>>  struct S2848 arg;
>>>>>  __builtin_va_list ap;
>>>>>
>>>>>  __builtin_va_start (ap, z);
>>>>>
>>>>>  arg = __builtin_va_arg (ap, struct S2848);
>>>>>
>>>>>  if (s2848.a != arg.a)
>>>>>    ++fails;
>>>>>  if (s2848.b != arg.b)
>>>>>    ++fails;
>>>>>
>>>>>  __builtin_va_end (ap);
>>>>> }
>>>>>
>>>>> int main (void)
>>>>> {
>>>>>  s2848.a = 4027477739U;
>>>>>  s2848.b = (723419448 + -218144346 * __extension__ 1i);
>>>>>
>>>>>  check2848va (1, s2848);
>>>>>
>>>>>  if (fails)
>>>>>    abort ();
>>>>>
>>>>>  return 0;
>>>>> }
>>>>> --
>>>>>
>>>>> We are reading beyond array of classes in
>>>>>
>>>>>                      num = classify_argument (TYPE_MODE (TREE_TYPE (field)),
>>>>>                                               TREE_TYPE (field), subclasses,
>>>>>                                               (int_bit_position (field)
>>>>>                                                + bit_offset) % 256);
>>>>>                      if (!num)
>>>>>                        return 0;
>>>>>                      for (i = 0; i < num; i++)
>>>>>                        {
>>>>>                          int pos =
>>>>>                            (int_bit_position (field) + (bit_offset %
>>>>> 64)) / 8 / 8;
>>>>>                          classes[i + pos] =
>>>>>                            merge_classes (subclasses[i], classes[i + pos]);
>>>>>                        }
>>>>>                    }
>>>>>
>>>>> The problem is classify_argument may return:
>>>>>
>>>>>      /* Zero sized arrays or structures are NO_CLASS.  We return 0 to
>>>>>         signalize memory class, so handle it as special case.  */
>>>>>      if (!words)
>>>>>        {
>>>>>          classes[0] = X86_64_NO_CLASS;
>>>>>          return 1;
>>>>>        }
>>>>>
>>>>> When num == 1, we should check classes[0] before accessing
>>>>> classes[1].
>>>>>
>>>>> Even if we were reading garbage, we didn't crash.
>>>>
>>>> I see.  Shouldn't the other places that recurse also check this?
>>>>
>>>
>>> I checked other places. They check classes[0] == X86_64_NO_CLASS
>>> before accessing classes beyond index 0.
>>
>> On a second look I don't see how classes and subclasses are connected here.
>> classify_argument fills subclasses and your patch checks that, but the following
>> loop will not access subclasses beyond index 0.
>
> The code is:
>
>                      num = classify_argument (TYPE_MODE (TREE_TYPE (field)),
>                                               TREE_TYPE (field), subclasses,
>                                               (int_bit_position (field)
>                                                + bit_offset) % 256);
>                      if (!num)
>                        return 0;
>                      for (i = 0; i < num; i++)
>                        {
>                          int pos =
>                            (int_bit_position (field) + (bit_offset %
> 64)) / 8 / 8;
>                          classes[i + pos] =
>                            merge_classes (subclasses[i], classes[i + pos]);
>
> (gdb) call debug_tree (field)
>  <field_decl 0x2a984bd960 c
>    type <record_type 0x2a985196c0 sizes-gimplified type_0 BLK
>        size <integer_cst 0x2a984053c0 constant 0>
>        unit size <integer_cst 0x2a983e7720 constant 0>
>        user align 128 symtab 0 alias set -1 canonical type 0x2a985196c0
>        attributes <tree_list 0x2a985281e0
>            purpose <identifier_node 0x2a98517480 aligned>>
>        chain <type_decl 0x2a98519780 D.1594>>
>    BLK file /net/gnu-13/export/gnu/src/gcc/gcc/gcc/testsuite/gcc.c-torture/execute/pr38151.c
> line 9 col 31 size <integer_cst 0x2a984053c0 0> unit size <integer_cst
> 0x2a983e7720 0>
>    user align 128 offset_align 128
>    offset <integer_cst 0x2a983e7e10 type <integer_type 0x2a983f8000
> long unsigned int> constant 16> bit offset <integer_cst 0x2a984053c0
> 0> context <record_type 0x2a98519480 S2848>>
> (gdb) call int_bit_position (field)
> $6 = 128
> (gdb) p (128  + (bit_offset % 64)) / 8 / 8
> $7 = 2
> (gdb)
>
> POS here is 2.

But i is at most 0 and we access subclasses[i] and classify_argument is passed
subclasses - which is also what you check.  That said - the patch is unobvious
enough to me ;)  (it looked like it may be an obvious one ...)

Richard.