Project

Profile

Help

Bug #5913

closed

Tail recursion overwrites value of variable

Added by Martin Holmes about 1 year ago. Updated 5 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Internals
Sprint/Milestone:
-
Start date:
2023-03-08
Due date:
% Done:

80%

Estimated time:
Legacy ID:
Applies to branch:
10, 11, 12, trunk
Fix Committed on Branch:
10, 11, 12, trunk
Fixed in Maintenance Release:
Platforms:
.NET, Java

Description

Arising out of a question on Slack on March 7:

Attached is a zip with test.xsl and test.xspec. The XSLT contains two functions, one of which calls the other, and the second is recursive. The XSpec tests the functions with various inputs. The process runs fine when an xsl:message is present either in the first or second function, even if the xsl:message does not read or output the content of any variables, but if all xsl:messages are commented out, the process fails with this error:

XTTE0570 An empty sequence is not allowed as the value of variable $nextLetter

which is a result of accessing a position in the variable $candidateSequence; it seems that the $prevPos variable ends up being assigned a value of 90 at some point, which is a value it should never get. This may be the result of some confusion between the position in the letter sequence ('A' through 'Z', length 26) and the codepoint of the last letter 'Z', which would be 90.


Files

saxon_10_issue.zip (3.17 KB) saxon_10_issue.zip test.xsl with two functions; test.xspec with some tests for those functions. Martin Holmes, 2023-03-08 18:30
Actions #1

Updated by Martin Honnen about 1 year ago

XSpec 2.2.4 fails for me with both Saxon 10.9 and 11.5 (used HE) if the comment is not in the code, here is the stack of 11.5:

Type error at char 0 in expression in xsl:variable/@select on line 102 column 107 of test.xsl:
  XTTE0570  An empty sequence is not allowed as the value of variable $nextLetter
at function hcmc:getNextBreakpoint on line 68 column 65 of test.xsl:
     invoked by unknown caller (class net.sf.saxon.value.MemoClosure)
at function hcmc:getLocationAlphabeticBreakPoints on line 36 column 80 of test.xsl:
     invoked by function call at file:/C:/Users/marti/OneDrive/Documents/xslt/blog-xslt-3-by-example/message-prevents-error/xspec/test-compiled.xsl#1632
at template scenario1-scenario1 on line 1571 column 83 of test-compiled.xsl:
     invoked by xsl:call-template at file:/C:/Users/marti/OneDrive/Documents/xslt/blog-xslt-3-by-example/message-prevents-error/xspec/test-compiled.xsl#1549
at template scenario1 on line 61 column 83 of test-compiled.xsl:
     invoked by xsl:for-each at file:/C:/Users/marti/OneDrive/Documents/xslt/blog-xslt-3-by-example/message-prevents-error/xspec/test-compiled.xsl#1546
at template scenario1 on line 61 column 83 of test-compiled.xsl:
     invoked by xsl:call-template at file:/C:/Users/marti/OneDrive/Documents/xslt/blog-xslt-3-by-example/message-prevents-error/xspec/test-compiled.xsl#50
at template main on line 31 column 41 of test-compiled.xsl:
     invoked by xsl:for-each at file:/C:/Users/marti/OneDrive/Documents/xslt/blog-xslt-3-by-example/message-prevents-error/xspec/test-compiled.xsl#47
at template main on line 31 column 41 of test-compiled.xsl:
An empty sequence is not allowed as the value of variable $nextLetter
Actions #2

Updated by Martin Honnen about 1 year ago

Saxon 12 HE gives

Testing with SAXON HE 12.0
Scenarios for testing hcmc:getLocationAlphabeticBreakPoints
..Threshold in the middle of a letter
Type error in expression in xsl:variable/@select on line 102 column 107 of test.xsl:
  XTTE0570  An empty sequence is not allowed as the value of variable $nextLetter
at function hcmc:getNextBreakpoint on line 68 column 65 of test.xsl:
An empty sequence is not allowed as the value of variable $nextLetter

All tests pass with the comment being present in 10.9, 11.5, 12.0.

Actions #3

Updated by Norm Tovey-Walsh about 1 year ago

It can be reduced to this test which can be run with -it -xsl:...

<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
                xmlns:hcmc="http://hcmc.uvic.ca/ns"
                xmlns:xh="http://www.w3.org/1999/xhtml"
                xmlns:xs="http://www.w3.org/2001/XMLSchema"
                exclude-result-prefixes="#all"
                version="3.0">

  <xsl:template name="xsl:initial-template">
    <xsl:variable name="testPlaceDiv1" as="element(xh:div)">
      <div xmlns="http://www.w3.org/1999/xhtml">
        <ul class="family">
          <li><span class="name">Abe, Yoshikazu</span></li>
        </ul>
      </div>
    </xsl:variable>

    <xsl:sequence select="hcmc:getLocationAlphabeticBreakPoints($testPlaceDiv1, 10)"/>
  </xsl:template>

  <xsl:function name="hcmc:getLocationAlphabeticBreakPoints" as="xs:string+">
    <xsl:param name="placeDiv" as="element(xh:div)"/>
    <xsl:param name="threshold" as="xs:integer"/>
    <xsl:variable name="candidateSequence" as="xs:string+" select="(for $i in string-to-codepoints('A')[1] to string-to-codepoints('Z')[1] return codepoints-to-string($i))"/> 

    <xsl:sequence 
        select="hcmc:getNextBreakpoint($placeDiv, $threshold, 0, $candidateSequence, 0, ())"/>
  </xsl:function>

  <xsl:function name="hcmc:getNextBreakpoint" as="xs:string+">
    <xsl:param name="placeDiv" as="element(xh:div)"/>
    <xsl:param name="threshold" as="xs:integer"/>
    <xsl:param name="prevPos" as="xs:integer"/>
    <xsl:param name="candidateSequence" as="xs:string+"/>
    <xsl:param name="currTotal" as="xs:integer"/>
    <xsl:param name="sequenceSoFar" as="xs:string*"/>

    <xsl:choose>
      <xsl:when test="$prevPos ge (count($candidateSequence) - 1)">
        <xsl:choose>
          <xsl:when test="($currTotal + count($placeDiv//xh:li[xh:span[@class='name'][starts-with(., $candidateSequence[last()])]])) gt $threshold">
            <xsl:sequence select="($sequenceSoFar, $candidateSequence[last()])"/>
          </xsl:when>
          <xsl:otherwise>            
            <xsl:sequence select="$sequenceSoFar"/>
          </xsl:otherwise>
        </xsl:choose>
      </xsl:when>
      <xsl:when test="empty($sequenceSoFar) and $prevPos lt 1">
        <xsl:sequence select="hcmc:getNextBreakpoint($placeDiv, $threshold, 1, $candidateSequence, count($placeDiv//xh:li[xh:span[@class='name'][starts-with(., $candidateSequence[1])]]), $candidateSequence[1])"/>
      </xsl:when>
      <xsl:otherwise>
        <xsl:variable name="nextLetter" as="xs:string" select="$candidateSequence[$prevPos + 1]"/>
        <xsl:variable name="totalForNextLetter" as="xs:integer" select="count($placeDiv//xh:li[xh:span[@class='name'][starts-with(., $nextLetter)]])"/>
        <xsl:variable name="newTotal" as="xs:integer" select="$currTotal + $totalForNextLetter"/>
        <xsl:choose>
          <xsl:when test="($newTotal gt $threshold)">
            <xsl:sequence select="hcmc:getNextBreakpoint($placeDiv, $threshold, $prevPos + 1, $candidateSequence, $totalForNextLetter, (($sequenceSoFar, if ($totalForNextLetter gt 0) then $candidateSequence[($prevPos + 1)] else ())))"/>
          </xsl:when>
          <xsl:otherwise>
            <!-- Otherwise, we just move up one. -->
            <xsl:sequence select="hcmc:getNextBreakpoint($placeDiv, $threshold, ($prevPos + 1), $candidateSequence, $newTotal, $sequenceSoFar)"/>
          </xsl:otherwise>
        </xsl:choose>
      </xsl:otherwise>
    </xsl:choose>
  </xsl:function>

</xsl:stylesheet>

The test conditions are remarkably sensitive at this point. Removing apparently unnecessary types (such as xs:string from $nextLetter) or even replacing the construction of the candidate sequence with a literal sequence avoids the error.

Actions #4

Updated by Martin Honnen about 1 year ago

When I run Norm's reduced test case with Saxon 12.0 HE and tail call optimization disabled (i.e. -opt:-t) it does not give the error. The same happens with HE 11.5 and HE 10.9.

Actions #5

Updated by Michael Kay about 1 year ago

We've worked through this together and have established what's going wrong; it remains to decide how to fix it.

We're basically getting an interference between two optimisations:

  1. We're doing a lazy evaluation of the expression for $i in 65 to 90 return codepoints-to-string($i) (the fact that we could be smarter and evaluate this as a constant expression is immaterial to this bug)

  2. The call on line 27 is a (different-function) tail call, so the stack frame of the caller is reused and overwritten (this gives benefits when a group of functions call each other in mutual recursion).

Normally, if an expression contains local variable references, then when we defer it for lazy evaluation (in a MemoClosure) we save a copy of anything in the context that it depends on. But this expression is identified as having no context dependencies, so we avoid the cost of making this copy. However, although it doesn't depend on the context, it does make use of the local stack-frame for evaluating and temporarily keeping the value of $i. After evaluating the expression, the stack frame slot holding $i will have the value 90, and because the stack frame has been shared with the tail-called function, this has the effect of clobbering the value of $prevPos.

Actions #6

Updated by Michael Kay about 1 year ago

A sufficient solution is to change Closure.saveContext() so it creates a new (all-null) stack frame in the saved context if the expression contains a subexpression that declares local range variables (specifically, an Assignation or a FLWORExpression).

However, this is a rather expensive search to do at run-time. The answer to this seems to be to allocate an extra dependency flag, StaticProperty.DEPENDS_ON_OWN_RANGE_VARIABLES which is maintained in the static properties word of every expression.

Actions #7

Updated by Michael Kay about 1 year ago

  • Subject changed from Saxon 10: error when running without xsl:message to Tail recursion overwrites value of variable
  • Category set to Internals
  • Status changed from New to In Progress
  • Assignee set to Michael Kay
  • Applies to branch 11, 12, trunk added
  • Fix Committed on Branch 11, 12, trunk added
  • Platforms .NET, Java added
Actions #8

Updated by Michael Kay about 1 year ago

I've added this as test case function-2109 - but unfortunately it doesn't fail when run from the test suite driver, it only fails when run from the command line. As we've seen, it's very sensitive to the input conditions.

Actions #9

Updated by Michael Kay about 1 year ago

  • Status changed from In Progress to Resolved
  • Fix Committed on Branch 10 added
Actions #10

Updated by O'Neil Delpratt about 1 year ago

  • Status changed from Resolved to Closed
  • % Done changed from 0 to 100
  • Fixed in Maintenance Release 12.1 added

Bug fix applied in the Saxon 12.1 maintenance release.

Actions #11

Updated by Debbie Lockett 5 months ago

  • Status changed from Closed to Resolved
  • % Done changed from 100 to 80
  • Fixed in Maintenance Release 11.6 added

The bug fix was also applied in the Saxon 11.6 maintenance release. Bug leaving bug "resolved" rather than "closed" as the fix has not gone out in a Saxon 10 maintenance release.

Please register to edit this issue

Also available in: Atom PDF