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