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

[Richard Guenther]

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.

I'll leave this to the x86 people, they may know how this is all
supposed to work
(why can we bail out for a zero-sized field-decl - there may be further ones?)

Richard.