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

Richard Biener richard.guenther@gmail.com
Mon Mar 13 13:08:00 GMT 2017


On Mon, Mar 13, 2017 at 2:02 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/13/2017 01:28 PM, Richard Biener wrote:
>> On Thu, Mar 9, 2017 at 12:40 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 03/08/2017 12:00 PM, Richard Biener wrote:
>>>> 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
>>>
>>> Yes, as you pointed out, we've got 2 issues here:
>>>
>>>>
>>>> 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).
>>>
>>> Let's consider a VLA, we then call chkp_find_bounds for call_arg equal to
>>>
>>> (gdb) p debug_tree(call_arg)
>>>  <ssa_name 0x7ffff69d5120
>>>     type <pointer_type 0x7ffff69c9e70
>>>         type <array_type 0x7ffff69c9d20 type <integer_type 0x7ffff688e5e8 int>
>>>             sizes-gimplified type_1 BLK size <var_decl 0x7ffff69d3360 D.1836> unit size <var_decl 0x7ffff69d33f0 D.1837>
>>>             align 32 symtab 0 alias set -1 structural equality domain <integer_type 0x7ffff69c9dc8>
>>>             pointer_to_this <pointer_type 0x7ffff69c9e70>>
>>>         unsigned DI
>>>         size <integer_cst 0x7ffff6876cd8 constant 64>
>>>         unit size <integer_cst 0x7ffff6876cf0 constant 8>
>>>         align 64 symtab 0 alias set -1 structural equality>
>>>     visited
>>>     def_stmt x.2_7 = x.1_20;
>>>     version 7>
>>>
>>> where bounds result in:
>>>   _18 = _6 * 4;
>>>   x.1_20 = __builtin_alloca_with_align (_18, 32);
>>>   __bound_tmp.5_24 = __builtin_ia32_bndmk (x.1_20, _18);
>>>
>>> which is consider fine. I don't understand how can be that broken on gimple as it's not exposed
>>> by GIMPLE?
>>
>> Not sure how this relates to my question.
>
> Hi.
>
> You mentioned that GIMPLE does not properly represent passing arguments by reference. And I'm asking
> whether it can happen that pass_by_reference returns true (in tree-chkp) and eventually a call is not
> going to be a pass by reference?

No, that can't happen.  I said that for example for

struct S { ... } s;
foo (s);

pass_by_reference may be true but on gimple you see a struct s as
actual argument.  I'm not sure
what chkp_find_bounds does to 's' in this case.  Like if floats are
passed by reference you might
see a REAL_CST passed to chkp_find_bounds but in reality it will get a
pointer (with bounds
according to the argument slot that was used).

>>
>>>>
>>>> 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...).
>>>
>>> That second issue should be probably fixed by comparing each formal and actual arguments,
>>> where for a mismatch an invalid bounds should be returned? Am I right?
>>
>> It's hard to say what to do for calls through incompatible tyes.  I'd
>> say it should use
>> the not instrumented copy (which there may be none if the function was
>> static and we
>> didn't need a clone?).  After all we're likely passing garbage to the
>> function anyway.
>
> Agree with you, as I briefly read the pass, there's quite fancy how it adds the shadow arguments
> (bounds) to calls. What about always cloning fndecl and adding bounds just for arguments that
> really correspond to gimple call fntype? Others can be preserved. It's definitely next stage1 material
> and I'm wondering whether it worth for the afford?
>
> P.S. I had a chat with Martin J. and it looks we have multiple places in source code where we have to somehow
> deal with discrepancy between formal and actual arguments. He also mentioned that you tried to fix these issues
> some time ago? But it's probably a hard mission?

It's a hard mission indeed ;)

Richard.

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



More information about the Gcc-patches mailing list