<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi everyone,
<div class="moz-forward-container">
<p>So my patch to fix <a moz-do-not-send="true"
href="https://bugs.freepascal.org/view.php?id=32913">#32913</a>
was unfortunately rejected because it relied on a slightly hacky
feature in the form of a new state variable that, currently, is
only used to patch this particular issue, and as Florian has
hinted in the issue notes, a deeper fix needs to be
investigated.</p>
<p>What I can put together so far is that if you have a construct
like this...</p>
<p><tt>try</tt><tt><br>
</tt><tt>except<br>
try</tt><tt><br>
</tt><tt> [b]</tt><tt><br>
finally<br>
[c]<br>
end;<br>
</tt><tt>end;</tt></p>
<p>...and [b] is not empty ([c] doesn't matter), then the compiler
will trigger Internal Error 200309201. What happened is that
during the simplify stage of ttryexceptnode, if the try block is
empty (represented by the 'left' node), the compiler would
replace the entire construct with a 'nothing' node. Logically a
sound choice, because it's impossible for an exception to be
raised in this instance. The problem comes that the except
section contains a try..finally block which sets up a stack
unwind block. It's a little hard to follow at this point, but
what it culminates in is that there's a non-null exception
filter (try..finally and try..except get bundled together for
the stack unwind) that the compiler is not expecting later on,
which causes a sanity check to fail and trigger the internal
error.</p>
<p>One approach is to simply not transform the except section into
a nothing node, but this adds significant overhead to the
compiled binaries and incur a performance penalty by having an
unnecessary stack unwind (such try..except blocks may occur in
the case of inlined functions yielding no code, for example).<br>
</p>
<p>My first attempt at fixing this was to move the 'simplify' code
into 'pass_1' for ttryexceptnode, and if it detected an empty
try section, to simply not run "pass_1" on anything in the
except section, so no code is ever put into the exception filter
- I argued this logic to myself because, as already explained,
this exception handler would never be executed. This worked,
but caused a regression in tests/tbf/tb0089.pp and
tests/tbf/tb0090.pp - these tests had an empty try section in
their try..except blocks, because all they had to test was to
attempt to use 'goto' from the except section to jump to a label
outside of the block. This particular syntactic check was only
performed in "pass_1", so by not running it for those nodes, the
syntax check was skipped and the tests erroneously compiled
successfully.</p>
<p>Since this approach resulted in a regression, it wasn't
acceptable and so I had to find some other means to prevent code
entering the exception filter without completely overhauling the
node parser. This led to what is how the "current_nodes_dead"
flag, set if the try section is empty and designed with future
expansion in mind (if you know nodes are unreachable, you can
skip doing any kind of code generation or even delete them
entirely once you do the syntactic checking). After writing
the necessary code in the ttryfinallynode class, I got the issue
fixed at long last.</p>
<p>It goes without saying that I was upset that the patch was
rejected, especially as this particular issue has been on my
books for literally a year now, and I had no other viable
solution to offer since everything else I had tried resulted in
an error elsewhere. However, now that Florian's actually
responded and hinted at a willingness to look for a bigger
solution, I'm able to look at it more objectively. Having a
flag that's only used to fix one feature is a little dangerous,
even with a proposal for future use, because chances are it will
be forgotten about and people will just introduce other flags
when and where they need.</p>
<p>I would like some discussion on this with the administrative
team because this does require some careful design if we're
going to do this properly. Building on Florian's suggestion, I
would like to propose splitting "pass_1" into "pass_syntax" and
a pass that transforms the nodes (I would name it
"pass_transform", but this doesn't sound 'important' enough,
given it is effectively compiling the nodes). "pass_syntax"
would be a public method declared in tnode as the following
"function pass_syntax: Boolean; virtual;" - if it returns True,
then everything was fine; if not, it returns False and this can
be used to set the error flag. By being defined this way, this
is no ability for the node to transform itself, but it can set
private fields based on the syntax it finds (by setting private
fields, the "first pass" won't have to repeat some of the work
needed to determine how to transform the nodes).<br>
</p>
<p>There are two ways this can then be implemented... one way is
to have the syntax pass be completely separate from the now
badly-named "first pass" and just do syntax checking on all the
nodes. If any syntax errors are found, then the error flag can
be set and "firstpass" will skip these nodes already. After
this syntax pass is done, then the compiler can move onto the
"first pass". The drawback to this is that it will likely
increase compilation time quite noticably, since the compiler
will be running an additional traversal through the entire node
tree.<br>
</p>
<p>The second way is to have the syntax pass as the first step of
"firstpass", so an extra traversal is not required. However,
this will be harder to develop and maintain because care has to
be taken that all nodes are syntax-checked but not transformed
if they would result in dead or unexpected code (as with
internal error 200309201). Given this would require a flag for
a 'dead node', we may end up in a similar situation as with the
"hacky" patch. However, one thought that occurred to me is that
we can make a new flag for tnodeflag named something like
'nf_dead', since this doesn't add any new fields, and if this
flag is set, then "pass_1" and "simplify" are not called, and it
cascades the flag to the child nodes. This would be the 32nd
flag for tnodeflag, so it reaches the upper limit for a small
set - something to keep in mind if it gets expanded later.<br>
</p>
<p>On the surface, the first implementation suggestion feels like
the cleaner option, but I don't know how much of a penalty it
will add to the compiler. Personally I would like to attempt a
design for the second implementation to keep the number of
passes down to a minimum and the compiler reasonably fast (also
my motivation behind the x86_64 peephole optimizer overhaul).
But that's just my opinion.</p>
<p>On an additional note, I do wonder if it's possible to merge
"pass_1" and "simplify", since they are treated pretty much
identically in the "firstpass" routine - if either of them
return something other than nil, it is considered to be a node
transformation, running this block of code (there are two
copies, one for each call):</p>
<p><tt>p.free;</tt><tt><br>
</tt><tt>p := hp;</tt><tt><br>
</tt><tt>firstpass(p);</tt><tt><br>
</tt></p>
<p>(hp contains the return value of whatever method was just
called) This may be impractical because "simplify" is called
elsewhere for some nodes.<br>
</p>
<p>At least that's what I've thought about so far. Where do we go
from here? I sense for this task, I should write up a PDF
design spec like I did with the optimizer overhaul, but this is
something that needs a fair bit of discussion. And
implementation will be a fairly mammoth task because it would
require introducing the new method to every single node type.<br>
</p>
<p>Thank you for your time, and I hope we can find the best
solution to this issue.<br>
</p>
<p>Gareth aka. Kit<br>
</p>
</div>
<div id="DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2"><br />
<table style="border-top: 1px solid #D3D4DE;">
<tr>
<td style="width: 55px; padding-top: 13px;"><a href="https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=emailclient" target="_blank"><img src="https://ipmcdn.avast.com/images/icons/icon-envelope-tick-round-orange-animated-no-repeat-v1.gif" alt="" width="46" height="29" style="width: 46px; height: 29px;" /></a></td>
<td style="width: 470px; padding-top: 12px; color: #41424e; font-size: 13px; font-family: Arial, Helvetica, sans-serif; line-height: 18px;">Virus-free. <a href="https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=emailclient" target="_blank" style="color: #4453ea;">www.avast.com</a>
</td>
</tr>
</table><a href="#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2" width="1" height="1"> </a></div></body>
</html>