Re: [PATCH] MPX: fix PR middle-end/79753

[Ilya Enkovich]

2017-03-15 20:09 GMT+03:00 Jeff Law :
> On 03/15/2017 04:15 AM, Martin Liška wrote:
>>
>> On 03/14/2017 11:27 PM, Ilya Enkovich wrote:
>>>
>>> 2017-03-10 16:15 GMT+03:00 Martin Liška :

 Hello.

 Currently, __builtin_ia32_bndret is used for all functions that have
 non-void
 return type. I think the right fix is to return bounds just for a
 bounded type.

 Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

 Ready to be installed?
 Martin
>>>
>>>
>>> Hi,
>>>
>>> The fix makes sense and allows you to pass the test but it seems like we
>>> still have the issue in inlining which can result in bndret call with
>>> wrong arg.
>>
>>
>> Hi.
>>
>> The test I added does probably what you mean: a return value of integer
>> is casted
>> to a pointer type. Or?
>>
>>> Do you avoid all such cases by this fix? Can we still have similar
>>> problem
>>> if cast function with integer return type to a function with pointer
>>> return type
>>> and the inline the call (not sure if we can inline such calls)?
>>>
>>> I think this fix still should go to trunk anyway.
>>
>>
>> Good, may I consider this as patch approval?
>
> Yes.  Ilya knows this stuff better than anyone, even if he's not listed as
> an official maintainer.

I'm still in the list :)

Ilya

>
> jeff
>

Re: [PATCH] MPX: fix PR middle-end/79753

[Ilya Enkovich]

2017-03-15 13:15 GMT+03:00 Martin Liška :
> On 03/14/2017 11:27 PM, Ilya Enkovich wrote:
>>
>> 2017-03-10 16:15 GMT+03:00 Martin Liška :
>>>
>>> Hello.
>>>
>>> Currently, __builtin_ia32_bndret is used for all functions that have
>>> non-void
>>> return type. I think the right fix is to return bounds just for a bounded
>>> type.
>>>
>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>>
>>> Ready to be installed?
>>> Martin
>>
>>
>> Hi,
>>
>> The fix makes sense and allows you to pass the test but it seems like we
>> still have the issue in inlining which can result in bndret call with
>> wrong arg.
>
>
> Hi.
>
> The test I added does probably what you mean: a return value of integer is
> casted
> to a pointer type. Or?

I suspect problem may still exist when you cast function, not just
returned value.
Inline is supposed to always remove retbnd corresponding to expanded call.
You avoid the problem in the testcase by not producing retbnd call. But in
case of calling a function casted to another function type we might
still hit this
issue in inline. I'll try to make a test.

Thanks,
Ilya

>
>> Do you avoid all such cases by this fix? Can we still have similar problem
>> if cast function with integer return type to a function with pointer
>> return type
>> and the inline the call (not sure if we can inline such calls)?
>>
>> I think this fix still should go to trunk anyway.
>
>
> Good, may I consider this as patch approval?
> Thanks,
> Martin
>
>>
>> Thanks,
>> Ilya
>>
>

Re: [PATCH] MPX: fix PR middle-end/79753

[Jeff Law]

On 03/15/2017 04:15 AM, Martin Liška wrote:

On 03/14/2017 11:27 PM, Ilya Enkovich wrote:

2017-03-10 16:15 GMT+03:00 Martin Liška :

Hello.

Currently, __builtin_ia32_bndret is used for all functions that have
non-void
return type. I think the right fix is to return bounds just for a
bounded type.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Martin


Hi,

The fix makes sense and allows you to pass the test but it seems like we
still have the issue in inlining which can result in bndret call with
wrong arg.


Hi.

The test I added does probably what you mean: a return value of integer
is casted
to a pointer type. Or?


Do you avoid all such cases by this fix? Can we still have similar
problem
if cast function with integer return type to a function with pointer
return type
and the inline the call (not sure if we can inline such calls)?

I think this fix still should go to trunk anyway.


Good, may I consider this as patch approval?
Yes.  Ilya knows this stuff better than anyone, even if he's not listed 
as an official maintainer.


jeff

Re: [PATCH] MPX: fix PR middle-end/79753

[Martin Liška]

On 03/14/2017 11:27 PM, Ilya Enkovich wrote:

2017-03-10 16:15 GMT+03:00 Martin Liška :

Hello.

Currently, __builtin_ia32_bndret is used for all functions that have non-void
return type. I think the right fix is to return bounds just for a bounded type.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Martin


Hi,

The fix makes sense and allows you to pass the test but it seems like we
still have the issue in inlining which can result in bndret call with wrong arg.


Hi.

The test I added does probably what you mean: a return value of integer is 
casted
to a pointer type. Or?


Do you avoid all such cases by this fix? Can we still have similar problem
if cast function with integer return type to a function with pointer return type
and the inline the call (not sure if we can inline such calls)?

I think this fix still should go to trunk anyway.


Good, may I consider this as patch approval?
Thanks,
Martin



Thanks,
Ilya

Re: [PATCH] MPX: fix PR middle-end/79753

[Ilya Enkovich]

2017-03-10 16:15 GMT+03:00 Martin Liška :
> Hello.
>
> Currently, __builtin_ia32_bndret is used for all functions that have non-void
> return type. I think the right fix is to return bounds just for a bounded 
> type.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Martin

Hi,

The fix makes sense and allows you to pass the test but it seems like we
still have the issue in inlining which can result in bndret call with wrong arg.
Do you avoid all such cases by this fix? Can we still have similar problem
if cast function with integer return type to a function with pointer return type
and the inline the call (not sure if we can inline such calls)?

I think this fix still should go to trunk anyway.

Thanks,
Ilya

[PATCH] MPX: fix PR middle-end/79753

[Martin Liška]

Hello.

Currently, __builtin_ia32_bndret is used for all functions that have non-void
return type. I think the right fix is to return bounds just for a bounded type.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Martin
>From f4ad5003a42ca95187305a9b201656c7da2aaa01 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Thu, 9 Mar 2017 15:42:49 +0100
Subject: [PATCH] MPX: fix PR middle-end/79753

gcc/ChangeLog:

2017-03-09  Martin Liska  <mli...@suse.cz>

	PR middle-end/79753
	* tree-chkp.c (chkp_build_returned_bound): Do not build
	returned bounds for a LHS that's not a BOUNDED_P type.

gcc/testsuite/ChangeLog:

2017-03-10  Martin Liska  <mli...@suse.cz>

	* gcc.target/i386/mpx/pr79753.c: New test.
---
 gcc/testsuite/gcc.target/i386/mpx/pr79753.c | 14 ++
 gcc/tree-chkp.c | 11 ++-
 2 files changed, 20 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/mpx/pr79753.c

diff --git a/gcc/testsuite/gcc.target/i386/mpx/pr79753.c b/gcc/testsuite/gcc.target/i386/mpx/pr79753.c
new file mode 100644
index 000..9b7bc52e1ed
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/mpx/pr79753.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-fcheck-pointer-bounds -mmpx -O2" } */
+
+int
+foo (void)
+{
+  return 0;
+}
+
+void
+bar (int **p)
+{
+  *p = (int *) (__UINTPTR_TYPE__) foo ();
+}
diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index acd57eac5ef..c057b977342 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -2218,6 +2218,7 @@ chkp_build_returned_bound (gcall *call)
   gimple *stmt;
   tree fndecl = gimple_call_fndecl (call);
   unsigned int retflags;
+  tree lhs = gimple_call_lhs (call);
 
   /* To avoid fixing alloca expands in targets we handle
  it separately.  */
@@ -2227,9 +2228,8 @@ chkp_build_returned_bound (gcall *call)
 	  || DECL_FUNCTION_CODE (fndecl) == BUILT_IN_ALLOCA_WITH_ALIGN))
 {
   tree size = gimple_call_arg (call, 0);
-  tree lb = gimple_call_lhs (call);
   gimple_stmt_iterator iter = gsi_for_stmt (call);
-  bounds = chkp_make_bounds (lb, size, , true);
+  bounds = chkp_make_bounds (lhs, size, , true);
 }
   /* We know bounds returned by set_bounds builtin call.  */
   else if (fndecl
@@ -2282,9 +2282,10 @@ chkp_build_returned_bound (gcall *call)
 
   bounds = chkp_find_bounds (gimple_call_arg (call, argno), );
 }
-  else if (chkp_call_returns_bounds_p (call))
+  else if (chkp_call_returns_bounds_p (call)
+	   && BOUNDED_P (lhs))
 {
-  gcc_assert (TREE_CODE (gimple_call_lhs (call)) == SSA_NAME);
+  gcc_assert (TREE_CODE (lhs) == SSA_NAME);
 
   /* In general case build checker builtin call to
 	 obtain returned bounds.  */
@@ -2311,7 +2312,7 @@ chkp_build_returned_bound (gcall *call)
   print_gimple_stmt (dump_file, call, 0, TDF_VOPS|TDF_MEMSYMS);
 }
 
-  bounds = chkp_maybe_copy_and_register_bounds (gimple_call_lhs (call), bounds);
+  bounds = chkp_maybe_copy_and_register_bounds (lhs, bounds);
 
   return bounds;
 }
-- 
2.11.1