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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.



-- 
H.J.