diff mbox

libgo patch committed: Update to 1.7rc3

Message ID CAOyqgcVbeVydOX2D3zQd5gtArRxOGuMHYcrWmF8Lr_rKi_fs1Q@mail.gmail.com
State New
Headers show

Commit Message

Ian Lance Taylor Aug. 8, 2016, 10:55 p.m. UTC 
On Fri, Aug 5, 2016 at 1:24 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Aug 4, 2016 at 6:48 PM, Ian Lance Taylor <iant@golang.org> wrote:
>> On Thu, Aug 4, 2016 at 1:11 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> On Thu, Aug 4, 2016 at 12:53 AM, Ian Lance Taylor <iant@golang.org> wrote:
>>>> On Thu, Jul 28, 2016 at 4:24 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>>>>
>>>>> A new testsuite failure is introduced:
>>>>>
>>>>> FAIL: text/template
>>>>>
>>>>> on both, x86_64-linux-gnu and alpha-linux-gnu.
>>>>>
>>>>> The testcase corrupts stack with a too deep recursion.
>>>>>
>>>>> There is a part in libgo/go/text/template/exec.go that should handle
>>>>> this situaiton:
>>>>>
>>>>> // maxExecDepth specifies the maximum stack depth of templates within
>>>>> // templates. This limit is only practically reached by accidentally
>>>>> // recursive template invocations. This limit allows us to return
>>>>> // an error instead of triggering a stack overflow.
>>>>> const maxExecDepth = 100000
>>>>>
>>>>> but the limit is either set too high, or the error handling code is
>>>>> inefficient on both, split-stack (x86_64) and non-split-stack (alpha)
>>>>> targets. Lowering this value to 10000 "fixes" the testcase on both
>>>>> targets.
>>>>
>>>> I can not recreate this problem on x86 or x86_64.
>>>>
>>>> Does this patch work around the problem on Alpha?
>>>
>>> Yes, the patch "fixes" the problem on alpha, but I still see the
>>> failure on x86_64, even with the unlimited stack.
>>
>> OK, I was able to recreate this by using GNU ld rather than gold.  I
>> have committed the appended patch to reduce the number of recursive
>> template invocations, since you said that 10000 did let the test pass
>> for you, and it works for me using GNU ld.  This number is still high
>> enough to not cut off any reasonable template execution.
>>
>> For this patch bootstrapped and ran Go testsuite on
>> x86_64-pc-linux-gnu, using both GNU ld and gold.  Committed to
>> mainline.
>
> The fixed test now passes reliably on alpha, *but* fails sporadically
> on x86_64-linux-gnu with split-stack (GNU ld).

Good point, I see this too.  I cut the value further, to 1000, and now
the test consistently passes for me.  That still seems adequate for
real word use.

Bootstrapped and ran Go testsuite on x86_64-pc-linux-gnu.  Committed
to mainline.

Ian
diff mbox

Patch

Index: gcc/go/gofrontend/MERGE
===================================================================
--- gcc/go/gofrontend/MERGE	(revision 239258)
+++ gcc/go/gofrontend/MERGE	(working copy)
@@ -1,4 +1,4 @@ 
-672db63f342c99bdc7ed46f040038440f429e600
+3b9c57a35370f26e6cf5839084e367e75e45ec97
 
 The first line of this file holds the git revision number of the last
 merge done from the gofrontend repository.
Index: libgo/go/text/template/exec.go
===================================================================
--- libgo/go/text/template/exec.go	(revision 239141)
+++ libgo/go/text/template/exec.go	(working copy)
@@ -19,9 +19,9 @@  import (
 // templates. This limit is only practically reached by accidentally
 // recursive template invocations. This limit allows us to return
 // an error instead of triggering a stack overflow.
-// For gccgo we make this 10000 rather than 100000 to avoid stack overflow
+// For gccgo we make this 1000 rather than 100000 to avoid stack overflow
 // on non-split-stack systems.
-const maxExecDepth = 10000
+const maxExecDepth = 1000
 
 // state represents the state of an execution. It's not part of the
 // template so that multiple executions of the same template