Hi everyone,
So my patch to fix #32913
<https://bugs.freepascal.org/view.php?id=32913> 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.
What I can put together so far is that if you have a construct like this...
try
except
try
[b]
finally
[c]
end;
end;
...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.
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).
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.
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.
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.
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).
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.
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.
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.
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.free;
p := hp;
firstpass(p);
(hp contains the return value of whatever method was just called) This
may be impractical because "simplify" is called elsewhere for some nodes.
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.
Thank you for your time, and I hope we can find the best solution to
this issue.
Gareth aka. Kit
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
_______________________________________________
fpc-devel maillist - fpc-devel@lists.freepascal.org
https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
