[fpc-devel] Deeper problem with Internal Error 200309201

[J. Gareth Moreton]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freepascal.org/pipermail/fpc-devel/attachments/20190713/2ce256c8/attachment-0001.html>