This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 1/5] Fix *_CST ICEs connected to MPX.
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?
>
>>>
>>> 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?
>
> Richard.
>
>> Martin
>>
>>>
>>>> ...
>>>>
>>>> Martin
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Martin
>>>>
>>