| Message ID | 09ca632c-23c6-4731-ab99-4fefc5e6de4e@suse.cz |
|---|---|
| State | New |
| Headers | show |
2017-03-10 16:15 GMT+03:00 Martin Liška <mliska@suse.cz>: > 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
On 03/14/2017 11:27 PM, Ilya Enkovich wrote: > 2017-03-10 16:15 GMT+03:00 Martin Liška <mliska@suse.cz>: >> 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 >
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 <mliska@suse.cz>: >>> 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
2017-03-15 13:15 GMT+03:00 Martin Liška <mliska@suse.cz>: > On 03/14/2017 11:27 PM, Ilya Enkovich wrote: >> >> 2017-03-10 16:15 GMT+03:00 Martin Liška <mliska@suse.cz>: >>> >>> 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 >> >
2017-03-15 20:09 GMT+03:00 Jeff Law <law@redhat.com>: > 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 <mliska@suse.cz>: >>>> >>>> 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 >
From f4ad5003a42ca95187305a9b201656c7da2aaa01 Mon Sep 17 00:00:00 2001 From: marxin <mliska@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 <mliska@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 <mliska@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 00000000000..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, &iter, true); + bounds = chkp_make_bounds (lhs, size, &iter, 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), &iter); } - 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