[fpc-devel] Deeper problem with Internal Error 200309201

[J. Gareth Moreton]

So a progress update.

I ended up running into some difficulty with the original idea of having 
a full semantic pass that is run prior to the first pass. For some 
reason, "pass_semantic" wasn't always executed for every node, and I 
still haven't figured out why (although it seems to be related to call 
parameter nodes), so instead I had to have it call "pass_semantic" as 
the first pass, which goes through the tree that branches from that 
particular node and calls "pass_semantic" recursively for each node it 
finds and flags it when complete.

As a result it's become a bit of a hybrid of the first and second ideas 
I had, although it still feels fairly clean in that changes have been 
minimal and easy to follow.  However, a new "semantics done" flag was 
required, largely for performance reasons (it reduces the complexity of 
the semantics pass from O(n²) to about O(n), otherwise a large portion 
of the tree would be re-evaluated each time firstpass is called, since 
the semantics pass MUST evaluate the children before they have a chance 
to get pruned or transformed by "pass_1").  To facilitate this, I 
introduced a new field in the TNode ancestor class named "compileflags", 
which contains flags that are only useful during the compilation process 
and are not stored in a PPU file because their values are deterministic 
upon a successful compilation.  These compile flags are "cf_error" (was 
originally the "nf_error" flag - should always be 0 in a PPU file), 
"cf_pass1_done" (was originally the "nf_pass1_done" flag - should always 
be 1 in a PPU file) and the new "cf_semantics_done" flag.  The two gaps 
in the original tnodeflag enumeration now contain "nf_reserved1" and 
"nf_reserved2" so it doesn't change the bit ordering in existing PPU 
files.  Given the enumeration already had 31 values out of a safe 
maximum of 32, this has freed up two extra bits for future expansion 
without having to change the PPU format in any way.

Ideally I want to figure out why "pass_semantic" isn't getting called in 
some situations, as that would remove the need to check the flag at the 
start of every call to "firstpass".  Besides, the intention and ambition 
is to have all the semantic checks moved to "pass_semantic" (currently 
only the ones originally in 'tgotonode.pass_1' were somewhat critical), 
so "pass_1" is free to prune child nodes if it so desires.  This is how 
"ttryexceptnode.pass_1" now resolves issue 32913... if the try block is 
completely empty, it frees and removes the "right" and "t1" nodes, which 
equate to the "on" and "else" parts of the except section, so they 
cannot be used to generate code that the compiler is not expecting, but 
also without overlooking semantic checks like an illegal jump to a label 
outside of the except block.  I figure that other node types may get 
refactored later to take advantage of this.

Nevertheless, my code currently compiles successfully and fixes issue 
32913 without causing tbf/tb0089.pp or tbf/tb0090.pp to fail.  I'll be 
writing up a PDF file before submitting the patches though because I 
feel this is something that needs review before being accepted.  I 
apologise if this is a lot to take in, but once you see the document and 
my patches, I believe they will look satisfactory and clean.

Gareth aka. Kit


On 14/07/2019 21:55, J. Gareth Moreton wrote:
> Looks like we have a deal then!  Break away code from "pass_1" into 
> "pass_semantic".
>
> Thanks for your support Florian - I'll start making plans.
>
> Gareth aka. Kit
>
> On 14/07/2019 19:45, Florian Klämpfl wrote:
>> Am 14.07.2019 um 04:25 schrieb J. Gareth Moreton:
>>> So having a long think over the past day, I'm starting to turn 
>>> against the second idea I had because it would require
>>> complex state variable tracking and is just asking for a new bug to 
>>> be introduced, not to mention that the additional
>>> overhead will probably offset any potential speed gains, so the 
>>> cleaner, simple solution sounds far more appealing now.
>>> Also, when it comes to dead nodes (an except section that gets 
>>> removed or any code following "Exit" in the same block),
>>> there's nothing to stop the compiler from deleting the nodes 
>>> completely, since semantics have already been checked (and
>>> maybe chucking in an extra "unreachable code" warning if needs be), 
>>> thus pruning the tree and making the process faster
>>> for pass_generate_code.
>>>
>>> I do wonder why 'simplify' is a distinct method though, because a 
>>> lot of the time one can simply put its code at the end
>>> of 'pass_1' and there wouldn't be any difference in functionality 
>>> (plus what counts as 'pass_1' and what counts as
>>> 'simplify' is often left to the programmer's discretion) while nodes 
>>> that have more complex simplification, like
>>> taddnode, could use their own private methods.  It might be 
>>> something for me to experiment with on the side.
>> The idea is to keep passes distinct. It makes maintainace easier. 
>> Besides this, Simplify is made to be run multiply
>> times, pass_1 not.
>>
>>> So the node parsing overhaul, so far...
>>>
>>> - pass_semantic
>> Yes.
>>
>>> - pass_transform (what was "pass_1")
>> I wouldn't change this in one patch. Step by step ...
>>
>>> - simplify (if not merged into the above)
>>> - pass_generate_code (I won't touch this for now)
>>>
>>> On another note, I do think pass_2 (pass_generate_code) could use 
>>> some refactoring.  I don't like how "flowcontrol" is a
>>> global variable.  Though it's unlikely to happen, such a state 
>>> variable not being tied to a management object (e.g.
>>> current_procinfo) prevents the compiler from being multi-threaded, 
>>> at least for that pass.
>>>
>>> Gareth aka. Kit
>>>
>>> P.S. Anyone got a better name for "pass_transform"?
>> pass_1 was for years, I wouldn't change it for the time being.
>> _______________________________________________
>> fpc-devel maillist  -  fpc-devel at lists.freepascal.org
>> https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
>>
>>
>
> ---
> This email has been checked for viruses by Avast antivirus software.
> https://www.avast.com/antivirus
>
> _______________________________________________
> fpc-devel maillist  -  fpc-devel at lists.freepascal.org
> https://lists.freepascal.org/cgi-bin/mailman/listinfo/fpc-devel
>
>