Re: [PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Jakub Jelinek]

On Wed, Mar 28, 2018 at 08:31:38AM +0200, Martin Liška wrote:
> Ok, so should we make the set of cum->bnds_in_bt based on 
> flag_check_pointer_bounds flag?
> 
> If so, I've got patch that I've tested on my x86_64-linux-gnu machin.
> 
> Martin

> >From 7b5978e61305c5098a084c2352fcbacb4c347158 Mon Sep 17 00:00:00 2001
> From: marxin 
> Date: Wed, 21 Mar 2018 10:51:32 +0100
> Subject: [PATCH] Do not call chkp_type_bounds_count if MPX is not enabled (PR
>  target/84988).
> 
> gcc/ChangeLog:
> 
> 2018-03-21  Martin Liska  
> 
>   PR target/84988
>   * config/i386/i386.c (ix86_function_arg_advance): Do not call
>   chkp_type_bounds_count if MPX is not enabled.

LGTM.

> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -8618,7 +8618,8 @@ ix86_function_arg_advance (cumulative_args_t cum_v, 
> machine_mode mode,
>if (cum->caller)
>   cfun->machine->outgoing_args_on_stack = true;
>  
> -  cum->bnds_in_bt = chkp_type_bounds_count (type);
> +  if (flag_check_pointer_bounds)
> + cum->bnds_in_bt = chkp_type_bounds_count (type);
>  }
>  }
>  
> -- 
> 2.16.2
> 


Jakub

Re: [PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Martin Liška]

On 03/21/2018 01:44 PM, Jakub Jelinek wrote:

On Wed, Mar 21, 2018 at 01:40:08PM +0100, Martin Liška wrote:

2018-03-21  Martin Liska  

PR target/84988
* config/i386/i386.c (ix86_function_arg_advance): Do not call
chkp_type_bounds_count if MPX is not enabled.
---
  gcc/config/i386/i386.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 5b1e962dedb..0693f8fc451 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -8618,7 +8618,8 @@ ix86_function_arg_advance (cumulative_args_t cum_v, 
machine_mode mode,
if (cum->caller)
cfun->machine->outgoing_args_on_stack = true;
  
-  cum->bnds_in_bt = chkp_type_bounds_count (type);

+  if (type && POINTER_BOUNDS_TYPE_P (type))
+   cum->bnds_in_bt = chkp_type_bounds_count (type);


This is weird.  POINTER_BOUNDS_TYPE_P (type)
is TREE_CODE (type) == POINTER_BOUNDS_TYPE,
and for POINTER_BOUNDS_TYPE chkp_type_bounds_count will just unconditionally
return 0.

Jakub



Ok, so should we make the set of cum->bnds_in_bt based on 
flag_check_pointer_bounds flag?

If so, I've got patch that I've tested on my x86_64-linux-gnu machin.

Martin
>From 7b5978e61305c5098a084c2352fcbacb4c347158 Mon Sep 17 00:00:00 2001
From: marxin 
Date: Wed, 21 Mar 2018 10:51:32 +0100
Subject: [PATCH] Do not call chkp_type_bounds_count if MPX is not enabled (PR
 target/84988).

gcc/ChangeLog:

2018-03-21  Martin Liska  

	PR target/84988
	* config/i386/i386.c (ix86_function_arg_advance): Do not call
	chkp_type_bounds_count if MPX is not enabled.
---
 gcc/config/i386/i386.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index b4f6aec1434..2b2896f7ac6 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -8618,7 +8618,8 @@ ix86_function_arg_advance (cumulative_args_t cum_v, machine_mode mode,
   if (cum->caller)
 	cfun->machine->outgoing_args_on_stack = true;
 
-  cum->bnds_in_bt = chkp_type_bounds_count (type);
+  if (flag_check_pointer_bounds)
+	cum->bnds_in_bt = chkp_type_bounds_count (type);
 }
 }
 
-- 
2.16.2

Re: [PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Jakub Jelinek]

On Wed, Mar 21, 2018 at 01:40:08PM +0100, Martin Liška wrote:
> 2018-03-21  Martin Liska  
> 
>   PR target/84988
>   * config/i386/i386.c (ix86_function_arg_advance): Do not call
>   chkp_type_bounds_count if MPX is not enabled.
> ---
>  gcc/config/i386/i386.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 5b1e962dedb..0693f8fc451 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -8618,7 +8618,8 @@ ix86_function_arg_advance (cumulative_args_t cum_v, 
> machine_mode mode,
>if (cum->caller)
>   cfun->machine->outgoing_args_on_stack = true;
>  
> -  cum->bnds_in_bt = chkp_type_bounds_count (type);
> +  if (type && POINTER_BOUNDS_TYPE_P (type))
> + cum->bnds_in_bt = chkp_type_bounds_count (type);

This is weird.  POINTER_BOUNDS_TYPE_P (type)
is TREE_CODE (type) == POINTER_BOUNDS_TYPE,
and for POINTER_BOUNDS_TYPE chkp_type_bounds_count will just unconditionally
return 0.

Jakub

Re: [PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Martin Liška]

On 03/21/2018 10:39 AM, Martin Liška wrote:

On 03/21/2018 10:36 AM, Richard Biener wrote:

On Tue, Mar 20, 2018 at 10:44 PM, Richard Sandiford
 wrote:

Jeff Law  writes:

On 03/20/2018 01:36 PM, Martin Liška wrote:

Hi.

This is a work-around to not iterate all members of array that can be huge.
As MPX will be removed in GCC 9.x, I hope it's acceptable. I don't want
to come
up with a new param for it.

Survives tests on x86_64-linux-gnu.

Martin

gcc/ChangeLog:

2018-03-20  Martin Liska  

 PR target/84988
 * tree-chkp.c (CHKP_ARRAY_MAX_CHECK_STEPS): Define a new macro.
 (chkp_find_bound_slots_1): Limit number of iterations.

Or just CLOSE/WONTFIX :-)

I've got no objections here -- we want to  minimize the effort put into
CHKP given its going to be deprecated.


The problem is that this affects normal configs, not just ones with
MPX enabled.


Indeed.  It get's called via

#0  chkp_find_bound_slots_1 (type=0x769ee9d8, have_bound=0x2ed3868, offs=0)
 at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1708
#1  0x01379a13 in chkp_find_bound_slots (type=0x769ee9d8,
 res=0x2ed3868)
 at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1754
#2  0x01377054 in chkp_type_bounds_count (type=0x769ee9d8)
 at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1009
#3  0x016c664f in ix86_function_arg_advance (cum_v=...,
 mode=E_BLKmode, type=0x769ee9d8, named=true)
 at /space/rguenther/src/svn/early-lto-debug/gcc/config/i386/i386.c:8621
8616    {
8617  /* Track if there are outgoing arguments on stack.  */
8618  if (cum->caller)
8619    cfun->machine->outgoing_args_on_stack = true;
8620
8621  cum->bnds_in_bt = chkp_type_bounds_count (type);
8622    }
8623    }
8624
8625    /* Define where to put the arguments to a function.

but I think we know POINTER_BOUNDS_TYPE_P etc. never return
true if -fcheck-pointer-* or -mmpx is not enabled, right?  So we can
guard the above call appropriately and save some compile-time
for all of us?


Good observation, let me enhance the patch for the PR.

Martin



Richard.


Thanks,
Richard




Hi.

I've got this, that survives bootstrap tests on x86_64-linux-gnu.

May I install it?
Thanks,
Martin
>From 15f95ac5117d9e2bab96c725716e5cb2832d3589 Mon Sep 17 00:00:00 2001
From: marxin 
Date: Wed, 21 Mar 2018 10:51:32 +0100
Subject: [PATCH] Do not call chkp_type_bounds_count if MPX is not enabled (PR
 target/84988).

gcc/ChangeLog:

2018-03-21  Martin Liska  

	PR target/84988
	* config/i386/i386.c (ix86_function_arg_advance): Do not call
	chkp_type_bounds_count if MPX is not enabled.
---
 gcc/config/i386/i386.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 5b1e962dedb..0693f8fc451 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -8618,7 +8618,8 @@ ix86_function_arg_advance (cumulative_args_t cum_v, machine_mode mode,
   if (cum->caller)
 	cfun->machine->outgoing_args_on_stack = true;
 
-  cum->bnds_in_bt = chkp_type_bounds_count (type);
+  if (type && POINTER_BOUNDS_TYPE_P (type))
+	cum->bnds_in_bt = chkp_type_bounds_count (type);
 }
 }
 
-- 
2.16.2

Re: [PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Martin Liška]

On 03/21/2018 10:36 AM, Richard Biener wrote:

On Tue, Mar 20, 2018 at 10:44 PM, Richard Sandiford
 wrote:

Jeff Law  writes:

On 03/20/2018 01:36 PM, Martin Liška wrote:

Hi.

This is a work-around to not iterate all members of array that can be huge.
As MPX will be removed in GCC 9.x, I hope it's acceptable. I don't want
to come
up with a new param for it.

Survives tests on x86_64-linux-gnu.

Martin

gcc/ChangeLog:

2018-03-20  Martin Liska  

 PR target/84988
 * tree-chkp.c (CHKP_ARRAY_MAX_CHECK_STEPS): Define a new macro.
 (chkp_find_bound_slots_1): Limit number of iterations.

Or just CLOSE/WONTFIX :-)

I've got no objections here -- we want to  minimize the effort put into
CHKP given its going to be deprecated.


The problem is that this affects normal configs, not just ones with
MPX enabled.


Indeed.  It get's called via

#0  chkp_find_bound_slots_1 (type=0x769ee9d8, have_bound=0x2ed3868, offs=0)
 at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1708
#1  0x01379a13 in chkp_find_bound_slots (type=0x769ee9d8,
 res=0x2ed3868)
 at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1754
#2  0x01377054 in chkp_type_bounds_count (type=0x769ee9d8)
 at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1009
#3  0x016c664f in ix86_function_arg_advance (cum_v=...,
 mode=E_BLKmode, type=0x769ee9d8, named=true)
 at /space/rguenther/src/svn/early-lto-debug/gcc/config/i386/i386.c:8621
8616{
8617  /* Track if there are outgoing arguments on stack.  */
8618  if (cum->caller)
8619cfun->machine->outgoing_args_on_stack = true;
8620
8621  cum->bnds_in_bt = chkp_type_bounds_count (type);
8622}
8623}
8624
8625/* Define where to put the arguments to a function.

but I think we know POINTER_BOUNDS_TYPE_P etc. never return
true if -fcheck-pointer-* or -mmpx is not enabled, right?  So we can
guard the above call appropriately and save some compile-time
for all of us?


Good observation, let me enhance the patch for the PR.

Martin



Richard.


Thanks,
Richard

Re: [PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Richard Biener]

On Tue, Mar 20, 2018 at 10:44 PM, Richard Sandiford
 wrote:
> Jeff Law  writes:
>> On 03/20/2018 01:36 PM, Martin Liška wrote:
>>> Hi.
>>>
>>> This is a work-around to not iterate all members of array that can be huge.
>>> As MPX will be removed in GCC 9.x, I hope it's acceptable. I don't want
>>> to come
>>> up with a new param for it.
>>>
>>> Survives tests on x86_64-linux-gnu.
>>>
>>> Martin
>>>
>>> gcc/ChangeLog:
>>>
>>> 2018-03-20  Martin Liska  
>>>
>>> PR target/84988
>>> * tree-chkp.c (CHKP_ARRAY_MAX_CHECK_STEPS): Define a new macro.
>>> (chkp_find_bound_slots_1): Limit number of iterations.
>> Or just CLOSE/WONTFIX :-)
>>
>> I've got no objections here -- we want to  minimize the effort put into
>> CHKP given its going to be deprecated.
>
> The problem is that this affects normal configs, not just ones with
> MPX enabled.

Indeed.  It get's called via

#0  chkp_find_bound_slots_1 (type=0x769ee9d8, have_bound=0x2ed3868, offs=0)
at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1708
#1  0x01379a13 in chkp_find_bound_slots (type=0x769ee9d8,
res=0x2ed3868)
at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1754
#2  0x01377054 in chkp_type_bounds_count (type=0x769ee9d8)
at /space/rguenther/src/svn/early-lto-debug/gcc/tree-chkp.c:1009
#3  0x016c664f in ix86_function_arg_advance (cum_v=...,
mode=E_BLKmode, type=0x769ee9d8, named=true)
at /space/rguenther/src/svn/early-lto-debug/gcc/config/i386/i386.c:8621
8616{
8617  /* Track if there are outgoing arguments on stack.  */
8618  if (cum->caller)
8619cfun->machine->outgoing_args_on_stack = true;
8620
8621  cum->bnds_in_bt = chkp_type_bounds_count (type);
8622}
8623}
8624
8625/* Define where to put the arguments to a function.

but I think we know POINTER_BOUNDS_TYPE_P etc. never return
true if -fcheck-pointer-* or -mmpx is not enabled, right?  So we can
guard the above call appropriately and save some compile-time
for all of us?

Richard.

> Thanks,
> Richard

Re: [PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Richard Sandiford]

Jeff Law  writes:
> On 03/20/2018 01:36 PM, Martin Liška wrote:
>> Hi.
>> 
>> This is a work-around to not iterate all members of array that can be huge.
>> As MPX will be removed in GCC 9.x, I hope it's acceptable. I don't want
>> to come
>> up with a new param for it.
>> 
>> Survives tests on x86_64-linux-gnu.
>> 
>> Martin
>> 
>> gcc/ChangeLog:
>> 
>> 2018-03-20  Martin Liska  
>> 
>> PR target/84988
>> * tree-chkp.c (CHKP_ARRAY_MAX_CHECK_STEPS): Define a new macro.
>> (chkp_find_bound_slots_1): Limit number of iterations.
> Or just CLOSE/WONTFIX :-)
>
> I've got no objections here -- we want to  minimize the effort put into
> CHKP given its going to be deprecated.

The problem is that this affects normal configs, not just ones with
MPX enabled.

Thanks,
Richard

Re: [PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Jeff Law]

On 03/20/2018 01:36 PM, Martin Liška wrote:
> Hi.
> 
> This is a work-around to not iterate all members of array that can be huge.
> As MPX will be removed in GCC 9.x, I hope it's acceptable. I don't want
> to come
> up with a new param for it.
> 
> Survives tests on x86_64-linux-gnu.
> 
> Martin
> 
> gcc/ChangeLog:
> 
> 2018-03-20  Martin Liska  
> 
> PR target/84988
> * tree-chkp.c (CHKP_ARRAY_MAX_CHECK_STEPS): Define a new macro.
> (chkp_find_bound_slots_1): Limit number of iterations.
Or just CLOSE/WONTFIX :-)

I've got no objections here -- we want to  minimize the effort put into
CHKP given its going to be deprecated.

jeff

[PATCH] Fix compile-time hog in MPX boundary checking (PR target/84988).

[Martin Liška]

Hi.

This is a work-around to not iterate all members of array that can be huge.
As MPX will be removed in GCC 9.x, I hope it's acceptable. I don't want to come
up with a new param for it.

Survives tests on x86_64-linux-gnu.

Martin

gcc/ChangeLog:

2018-03-20  Martin Liska  

PR target/84988
* tree-chkp.c (CHKP_ARRAY_MAX_CHECK_STEPS): Define a new macro.
(chkp_find_bound_slots_1): Limit number of iterations.
---
 gcc/tree-chkp.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)


diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c
index 40497ce94e7..d10e6c40423 100644
--- a/gcc/tree-chkp.c
+++ b/gcc/tree-chkp.c
@@ -1688,6 +1688,10 @@ chkp_find_bounds_for_elem (tree elem, tree *all_bounds,
 }
 }
 
+/* Maximum number of elements to check in an array.  */
+
+#define CHKP_ARRAY_MAX_CHECK_STEPS4096
+
 /* Fill HAVE_BOUND output bitmap with information about
bounds requred for object of type TYPE.
 
@@ -1733,7 +1737,9 @@ chkp_find_bound_slots_1 (const_tree type, bitmap have_bound,
 	  || integer_minus_onep (maxval))
 	return;
 
-  for (cur = 0; cur <= TREE_INT_CST_LOW (maxval); cur++)
+  for (cur = 0;
+	  cur <= MIN (CHKP_ARRAY_MAX_CHECK_STEPS, TREE_INT_CST_LOW (maxval));
+	  cur++)
 	chkp_find_bound_slots_1 (etype, have_bound, offs + cur * esize);
 }
 }