Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.

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

On Tue, Mar 7, 2017 at 5:01 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/07/2017 03:53 PM, Richard Biener wrote:
>> On Tue, Mar 7, 2017 at 3:48 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 03/07/2017 11:17 AM, Rainer Orth wrote:
>>>> marxin <mliska@suse.cz> writes:
>>>>
>>>>> diff --git a/gcc/testsuite/g++.dg/pr79769.C b/gcc/testsuite/g++.dg/pr79769.C
>>>>> new file mode 100644
>>>>> index 00000000000..f9223db1b2d
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/g++.dg/pr79769.C
>>>>> @@ -0,0 +1,4 @@
>>>>> +/* { dg-do compile { target { ! x32 } } } */
>>>>> +/* { dg-options "-fcheck-pointer-bounds -mmpx -mabi=ms" } */
>>>>
>>>> ... and again: make this x86-only.
>>>>
>>>>       Rainer
>>>>
>>>
>>> Thanks. I'm sending v2 of the patch.
>>
>> Hmm, not sure why we should handle REAL_CST here explicitely for example.
>>
>> Why not, instead of internal_error in the default: case do
>>
>>   bounds = chkp_get_invalid_op_bounds ();
>
> Because chkp_get_invalid_op_bounds() returns bounds that are always valid and as it's
> security extension, I would be strict here in order to not handle something that can bypass
> the checking.
>
>>
>> there?  For the testcase why do we invoke chkp_find_bounds_1 on sth that is
>> a REAL_CST for example?
>
> It's called when setting bounds in a call expr:
>
> #0  chkp_find_bounds_1 (ptr=0x7ffff6a03720, ptr_src=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3734
> #1  0x0000000000ec7c7d in chkp_find_bounds (ptr=0x7ffff6a03720, iter=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:3768
> #2  0x0000000000ec22e1 in chkp_add_bounds_to_call_stmt (gsi=0x7fffffffd5d0) at ../../gcc/tree-chkp.c:1901
> #3  0x0000000000ec9a1a in chkp_instrument_function () at ../../gcc/tree-chkp.c:4344
> #4  0x0000000000eca4cb in chkp_execute () at ../../gcc/tree-chkp.c:4528

So this happens for calls with mismatched arguments?  I believe that

      if (BOUNDED_TYPE_P (type)
          || pass_by_reference (NULL, TYPE_MODE (type), type, true))
        new_args.safe_push (chkp_find_bounds (call_arg, gsi));

may be misguided given pass_by_reference is not exposed at GIMPLE and thus
for the pass_by_reference case it should fall back to
"chkp_get_invalid_op_bounds"
and thus eventually pass this down as a flag to chkp_find_bounds (or check
on call_arg again).

Note that here 'type' and call_arg may not match (asking for trouble).  Plus
'fntype' is computed bogously (should use gimple_call_fntype).

It later does sth like

  /* For direct calls fndecl is replaced with instrumented version.  */
  if (fndecl)
    {
      tree new_decl = chkp_maybe_create_clone (fndecl)->decl;
      gimple_call_set_fndecl (new_call, new_decl);
      /* In case of a type cast we should modify used function
         type instead of using type of new fndecl.  */
      if (gimple_call_fntype (call) != TREE_TYPE (fndecl))
        {
          tree type = gimple_call_fntype (call);
          type = chkp_copy_function_type_adding_bounds (type);
          gimple_call_set_fntype (new_call, type);
        }
      else
        gimple_call_set_fntype (new_call, TREE_TYPE (new_decl));

but obviously if gimple_call_fntype and fndecl don't match cloning
the mismatched decl isn't of very much help ... (it may just generate
more mismatches with mismatching bounds...).

> ...
>
> Martin
>
>>
>> Richard.
>>
>>> Martin
>