[PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

This fixes a bug discovered when we were evaluating the current state of
-fstack-check.  It ought to be able to go forward independent of the
rest of the -fstack-check work.

The aarch64 specific code does not correctly handle large frames and
will generate RTL with unrecognizable insns for such cases.  This is
clearly visible if -fstack-check is enabled by default or if it were to
be added to the torture flags in the testsuite.

I've tested this by bootstrapping and regression testing an aarch64
compiler with -fstack-check on by default and hacks to force all
allocations/probing of more than PROBE_INTERVAL bytes to go through this
path.  It fixes a slew of testsuite failures (~80 for C and a few for
Fortran and C++).


One example is c-torture/compile/20031023-1.c which has a local frame of
0x10000000000 bytes.

OK for the trunk?

Jeff





[-- Attachment #2: P --]
[-- Type: text/plain, Size: 1047 bytes --]

	* config/aarch64/aarch64.c (aarch64_emit_probe_stack_range): Handle
	frame sizes that do not satisfy aarch64_uimm12_shift.

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 04417dc..4f89457 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -2722,11 +2722,19 @@ aarch64_emit_probe_stack_range (HOST_WIDE_INT first, HOST_WIDE_INT size)
 		     plus_constant (Pmode, stack_pointer_rtx, -first));
 
       /* LAST_ADDR = SP + FIRST + ROUNDED_SIZE.  */
-      emit_set_insn (reg2,
-		     plus_constant (Pmode, stack_pointer_rtx,
-				    -(first + rounded_size)));
-
-
+      HOST_WIDE_INT adjustment = - (first + rounded_size);
+      if (! aarch64_uimm12_shift (adjustment))
+	{
+	  aarch64_internal_mov_immediate (reg2, GEN_INT (adjustment),
+					  true, Pmode);
+	  emit_set_insn (reg2, gen_rtx_PLUS (Pmode, stack_pointer_rtx, reg2));
+	}
+      else
+	{
+	  emit_set_insn (reg2,
+			 plus_constant (Pmode, stack_pointer_rtx, adjustment));
+	}
+	  	
       /* Step 3: the loop
 
 	 do

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Jakub Jelinek]

On Thu, Jun 22, 2017 at 09:01:03AM -0600, Jeff Law wrote:
> This fixes a bug discovered when we were evaluating the current state of
> -fstack-check.  It ought to be able to go forward independent of the
> rest of the -fstack-check work.
> 
> The aarch64 specific code does not correctly handle large frames and
> will generate RTL with unrecognizable insns for such cases.  This is
> clearly visible if -fstack-check is enabled by default or if it were to
> be added to the torture flags in the testsuite.
> 
> I've tested this by bootstrapping and regression testing an aarch64
> compiler with -fstack-check on by default and hacks to force all
> allocations/probing of more than PROBE_INTERVAL bytes to go through this
> path.  It fixes a slew of testsuite failures (~80 for C and a few for
> Fortran and C++).
> 
> 
> One example is c-torture/compile/20031023-1.c which has a local frame of
> 0x10000000000 bytes.
> 
> OK for the trunk?

So shouldn't we add at least one testcase that #includes that testcase
(or one written from scratch) and is compiled with -fstack-check ?

	Jakub

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Richard Earnshaw (lists)]

On 22/06/17 16:01, Jeff Law wrote:
> This fixes a bug discovered when we were evaluating the current state of
> -fstack-check.  It ought to be able to go forward independent of the
> rest of the -fstack-check work.
> 
> The aarch64 specific code does not correctly handle large frames and
> will generate RTL with unrecognizable insns for such cases.  This is
> clearly visible if -fstack-check is enabled by default or if it were to
> be added to the torture flags in the testsuite.
> 
> I've tested this by bootstrapping and regression testing an aarch64
> compiler with -fstack-check on by default and hacks to force all
> allocations/probing of more than PROBE_INTERVAL bytes to go through this
> path.  It fixes a slew of testsuite failures (~80 for C and a few for
> Fortran and C++).
> 
> 
> One example is c-torture/compile/20031023-1.c which has a local frame of
> 0x10000000000 bytes.
> 
> OK for the trunk?
> 

OK.  But as Jakub says, a test would be nice.

R.

> Jeff
> 
> 
> 
> 
> 
> P
> 
> 
> 	* config/aarch64/aarch64.c (aarch64_emit_probe_stack_range): Handle
> 	frame sizes that do not satisfy aarch64_uimm12_shift.
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 04417dc..4f89457 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -2722,11 +2722,19 @@ aarch64_emit_probe_stack_range (HOST_WIDE_INT first, HOST_WIDE_INT size)
>  		     plus_constant (Pmode, stack_pointer_rtx, -first));
>  
>        /* LAST_ADDR = SP + FIRST + ROUNDED_SIZE.  */
> -      emit_set_insn (reg2,
> -		     plus_constant (Pmode, stack_pointer_rtx,
> -				    -(first + rounded_size)));
> -
> -
> +      HOST_WIDE_INT adjustment = - (first + rounded_size);
> +      if (! aarch64_uimm12_shift (adjustment))
> +	{
> +	  aarch64_internal_mov_immediate (reg2, GEN_INT (adjustment),
> +					  true, Pmode);
> +	  emit_set_insn (reg2, gen_rtx_PLUS (Pmode, stack_pointer_rtx, reg2));
> +	}
> +      else
> +	{
> +	  emit_set_insn (reg2,
> +			 plus_constant (Pmode, stack_pointer_rtx, adjustment));
> +	}
> +	  	
>        /* Step 3: the loop
>  
>  	 do
> 

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Jeff Law]

On 06/22/2017 09:29 AM, Richard Earnshaw (lists) wrote:
> On 22/06/17 16:01, Jeff Law wrote:
>> This fixes a bug discovered when we were evaluating the current state of
>> -fstack-check.  It ought to be able to go forward independent of the
>> rest of the -fstack-check work.
>>
>> The aarch64 specific code does not correctly handle large frames and
>> will generate RTL with unrecognizable insns for such cases.  This is
>> clearly visible if -fstack-check is enabled by default or if it were to
>> be added to the torture flags in the testsuite.
>>
>> I've tested this by bootstrapping and regression testing an aarch64
>> compiler with -fstack-check on by default and hacks to force all
>> allocations/probing of more than PROBE_INTERVAL bytes to go through this
>> path.  It fixes a slew of testsuite failures (~80 for C and a few for
>> Fortran and C++).
>>
>>
>> One example is c-torture/compile/20031023-1.c which has a local frame of
>> 0x10000000000 bytes.
>>
>> OK for the trunk?
>>
> 
> OK.  But as Jakub says, a test would be nice.
Sure.  I'll do something with 20031023-1.c to ensure it or an equivalent
is compiled with -fstack-check.  That isn't totally unexpected.   I
would have also been receptive to adding -fstack-check to the torture flags.

I'll cobble together the test side momentarily.

jeff

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Mike Stump]

On Jun 22, 2017, at 8:32 AM, Jeff Law <law@redhat.com> wrote:
> 
> Sure.  I'll do something with 20031023-1.c to ensure it or an equivalent
> is compiled with -fstack-check.  That isn't totally unexpected.   I
> would have also been receptive to adding -fstack-check to the torture flags.

Ouch.  Though stack checking might be important, the feature is very, very narrow, and once tested, if unlike to ever break or interact badly with other work.  I'd rather people default it to on, run the entire suite, fix all bugs (with test cases added for all the bugs), then turn it back off.  Additional torture passes are expensive; we use them for things that do regress, that are important, that have thousands of moving parts to keep them working.  O2, -g are good examples for things that by their nature, likely will always be best served by torture options.  Now, if you want to focus on security for 1-3 months, add it, fix all the bugs, then turn it off; it would be a great way to get all the bugs filed, if you want.

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Jakub Jelinek]

On Thu, Jun 22, 2017 at 10:02:16AM -0700, Mike Stump wrote:
> On Jun 22, 2017, at 8:32 AM, Jeff Law <law@redhat.com> wrote:
> > 
> > Sure.  I'll do something with 20031023-1.c to ensure it or an equivalent
> > is compiled with -fstack-check.  That isn't totally unexpected.   I
> > would have also been receptive to adding -fstack-check to the torture flags.
> 
> Ouch.  Though stack checking might be important, the feature is very, very
> narrow, and once tested, if unlike to ever break or interact badly with
> other work.  I'd rather people default it to on, run the entire suite, fix
> all bugs (with test cases added for all the bugs), then turn it back off. 
> Additional torture passes are expensive; we use them for things that do
> regress, that are important, that have thousands of moving parts to keep
> them working.  O2, -g are good examples for things that by their nature,
> likely will always be best served by torture options.  Now, if you want to
> focus on security for 1-3 months, add it, fix all the bugs, then turn it
> off; it would be a great way to get all the bugs filed, if you want.

Yeah, I think it should be enough to have one or a couple of -fstack-check
covering testcases in the testsuite, with 10-20 different static frame sizes
(a couple of leaf functions and then functions that call something) and then
for each also variant with alloca or VLA, make it executable and test you
can store stuff in the buffers and read it back in some other function,
compile this with -fno-omit-frame-pointer, -fomit-frame-pointer and default.
No need to torture everything with it.

	Jakub

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Andrew Pinski]

On Thu, Jun 22, 2017 at 10:02 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Jun 22, 2017, at 8:32 AM, Jeff Law <law@redhat.com> wrote:
>>
>> Sure.  I'll do something with 20031023-1.c to ensure it or an equivalent
>> is compiled with -fstack-check.  That isn't totally unexpected.   I
>> would have also been receptive to adding -fstack-check to the torture flags.
>
> Ouch.  Though stack checking might be important, the feature is very, very narrow, and once tested, if unlike to ever break or interact badly with other work.  I'd rather people default it to on, run the entire suite, fix all bugs (with test cases added for all the bugs), then turn it back off.  Additional torture passes are expensive; we use them for things that do regress, that are important, that have thousands of moving parts to keep them working.  O2, -g are good examples for things that by their nature, likely will always be best served by torture options.  Now, if you want to focus on security for 1-3 months, add it, fix all the bugs, then turn it off; it would be a great way to get all the bugs filed, if you want.


One other way of doing this testing is having your own testers test
both with and without -fstack-check like some folks do for -fPIC.

Thanks,
Andrew

>

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Jeff Law]

On 06/22/2017 11:07 AM, Jakub Jelinek wrote:
> On Thu, Jun 22, 2017 at 10:02:16AM -0700, Mike Stump wrote:
>> On Jun 22, 2017, at 8:32 AM, Jeff Law <law@redhat.com> wrote:
>>>
>>> Sure.  I'll do something with 20031023-1.c to ensure it or an equivalent
>>> is compiled with -fstack-check.  That isn't totally unexpected.   I
>>> would have also been receptive to adding -fstack-check to the torture flags.
>>
>> Ouch.  Though stack checking might be important, the feature is very, very
>> narrow, and once tested, if unlike to ever break or interact badly with
>> other work.  I'd rather people default it to on, run the entire suite, fix
>> all bugs (with test cases added for all the bugs), then turn it back off. 
>> Additional torture passes are expensive; we use them for things that do
>> regress, that are important, that have thousands of moving parts to keep
>> them working.  O2, -g are good examples for things that by their nature,
>> likely will always be best served by torture options.  Now, if you want to
>> focus on security for 1-3 months, add it, fix all the bugs, then turn it
>> off; it would be a great way to get all the bugs filed, if you want.
> 
> Yeah, I think it should be enough to have one or a couple of -fstack-check
> covering testcases in the testsuite, with 10-20 different static frame sizes
> (a couple of leaf functions and then functions that call something) and then
> for each also variant with alloca or VLA, make it executable and test you
> can store stuff in the buffers and read it back in some other function,
> compile this with -fno-omit-frame-pointer, -fomit-frame-pointer and default.
> No need to torture everything with it.
I expect we'll want to do a bit more than that -- in particular if we
look at how prologues as generated on aarch64, we'll want to test each
of the paths  through there which allocate stack space and verify that
the proper probes are emitted or omitted.

The thought of scanning the assembly code or RTL is too painful to
contemplate.  THus I've been pondering having the prologue expanders
emit notes into the dump file about what they did and why WRT probing.

Or maybe we should use the unit testing framewhere here.  Hmmm....


jeff

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Florian Weimer]

* Mike Stump:

> On Jun 22, 2017, at 8:32 AM, Jeff Law <law@redhat.com> wrote:
>> 
>> Sure.  I'll do something with 20031023-1.c to ensure it or an equivalent
>> is compiled with -fstack-check.  That isn't totally unexpected.   I
>> would have also been receptive to adding -fstack-check to the torture flags.
>
> Ouch.  Though stack checking might be important, the feature is very,
> very narrow, and once tested, if unlike to ever break or interact
> badly with other work.  I'd rather people default it to on, run the
> entire suite, fix all bugs (with test cases added for all the bugs),
> then turn it back off.

Are there many tests which give different results with and without
stack checking?

I expect that eventually, there will be a configure flag which
controls whether stack checking is enabled by default (similar to
PIE-by-default), and we could thus rely on tester variance to cover
the test suite with both flag states.

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Richard Biener]

On June 22, 2017 7:17:16 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>On 06/22/2017 11:07 AM, Jakub Jelinek wrote:
>> On Thu, Jun 22, 2017 at 10:02:16AM -0700, Mike Stump wrote:
>>> On Jun 22, 2017, at 8:32 AM, Jeff Law <law@redhat.com> wrote:
>>>>
>>>> Sure.  I'll do something with 20031023-1.c to ensure it or an
>equivalent
>>>> is compiled with -fstack-check.  That isn't totally unexpected.   I
>>>> would have also been receptive to adding -fstack-check to the
>torture flags.
>>>
>>> Ouch.  Though stack checking might be important, the feature is
>very, very
>>> narrow, and once tested, if unlike to ever break or interact badly
>with
>>> other work.  I'd rather people default it to on, run the entire
>suite, fix
>>> all bugs (with test cases added for all the bugs), then turn it back
>off. 
>>> Additional torture passes are expensive; we use them for things that
>do
>>> regress, that are important, that have thousands of moving parts to
>keep
>>> them working.  O2, -g are good examples for things that by their
>nature,
>>> likely will always be best served by torture options.  Now, if you
>want to
>>> focus on security for 1-3 months, add it, fix all the bugs, then
>turn it
>>> off; it would be a great way to get all the bugs filed, if you want.
>> 
>> Yeah, I think it should be enough to have one or a couple of
>-fstack-check
>> covering testcases in the testsuite, with 10-20 different static
>frame sizes
>> (a couple of leaf functions and then functions that call something)
>and then
>> for each also variant with alloca or VLA, make it executable and test
>you
>> can store stuff in the buffers and read it back in some other
>function,
>> compile this with -fno-omit-frame-pointer, -fomit-frame-pointer and
>default.
>> No need to torture everything with it.
>I expect we'll want to do a bit more than that -- in particular if we
>look at how prologues as generated on aarch64, we'll want to test each
>of the paths  through there which allocate stack space and verify that
>the proper probes are emitted or omitted.
>
>The thought of scanning the assembly code or RTL is too painful to
>contemplate.  THus I've been pondering having the prologue expanders
>emit notes into the dump file about what they did and why WRT probing.
>
>Or maybe we should use the unit testing framewhere here.  Hmmm....

I've been thinking that for Linux the kernel should have a way (/proc/pid controlled) to only extend the stack mapping for faults to the page exactly below the current end of the mapping.  So we'd get a segfault for any missed probe.

Just for QA purposes of course, I expect too many false positives in a full system.

Richard.

>
>jeff

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Jakub Jelinek]

On Thu, Jun 22, 2017 at 08:42:54PM +0200, Richard Biener wrote:
> >The thought of scanning the assembly code or RTL is too painful to
> >contemplate.  THus I've been pondering having the prologue expanders
> >emit notes into the dump file about what they did and why WRT probing.
> >
> >Or maybe we should use the unit testing framewhere here.  Hmmm....
> 
> I've been thinking that for Linux the kernel should have a way (/proc/pid controlled) to only extend the stack mapping for faults to the page exactly below the current end of the mapping.  So we'd get a segfault for any missed probe.
> 
> Just for QA purposes of course, I expect too many false positives in a full system.

Or valgrind could have such a tool or option.

	Jakub

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Jeff Law]

On 06/22/2017 12:23 PM, Florian Weimer wrote:
> * Mike Stump:
> 
>> On Jun 22, 2017, at 8:32 AM, Jeff Law <law@redhat.com> wrote:
>>>
>>> Sure.  I'll do something with 20031023-1.c to ensure it or an equivalent
>>> is compiled with -fstack-check.  That isn't totally unexpected.   I
>>> would have also been receptive to adding -fstack-check to the torture flags.
>>
>> Ouch.  Though stack checking might be important, the feature is very,
>> very narrow, and once tested, if unlike to ever break or interact
>> badly with other work.  I'd rather people default it to on, run the
>> entire suite, fix all bugs (with test cases added for all the bugs),
>> then turn it back off.
> 
> Are there many tests which give different results with and without
> stack checking?
It's not terrible.  Some are simply dump scanning regexps that need
updating and some guality stuff -- at least on x86, ppc & aarch.

> 
> I expect that eventually, there will be a configure flag which
> controls whether stack checking is enabled by default (similar to
> PIE-by-default), and we could thus rely on tester variance to cover
> the test suite with both flag states.
That'd be my expectation as well.

But I still think we're going to want to explicitly test the various
cases where we want to see probes vs when we do not.  That kind of
testing won't be covered unless we explicitly do so and the least
painful way to cover may be via dump messages or the unit testing framework.


jeff

Re: [PATCH][RFA] Fix -fstack-check with really big frames on aarch64

[Florian Weimer]

* Jeff Law:

> But I still think we're going to want to explicitly test the various
> cases where we want to see probes vs when we do not.  That kind of
> testing won't be covered unless we explicitly do so and the least
> painful way to cover may be via dump messages or the unit testing
> framework.

Agreed, it provides coverage different from compiling unrelated tests
with stack checking enabled.