View Issue Details

IDProjectCategoryView StatusLast Update
0032913FPCCompilerpublic2019-07-18 05:55
ReporterDavid HawkAssigned ToFlorian 
PriorityhighSeveritymajorReproducibilityalways
Status feedbackResolutionopen 
PlatformwindowsOSwindowsOS Version10 64 bit
Product Version3.0.4Product Buildcurrent recommended download 
Target VersionFixed in Version 
Summary0032913: nested try blocks with exit gives Error: Internal error
DescriptionI have a procedure exit statement in try/finally block within the exception handler of a try/except block. Compiling it gives "Internal Error 200309201". See greatly simplified source in "Additional Information" below.

In my larger program I get an undefined symbol error .Lj9137. The code listing shows the following for the EXIT instruction:
# [3070] EXIT
    movq %rsp,%rcx
    leaq .Lj9137(%rip),%rdx
    call _FPC_local_unwind
    jmp .Lj9181

and .Lj9137 is nowhere to be found.

This code has worked in earlier versions of Lazarus (1.2?) and also works in Delphi (various versions).

Thanks in advance for looking into this.
Steps To ReproduceI have a fresh install of Lazarus 1.8 on Windows 10 64-bit. I created a new project with all defaults and used the source below as the .lpr file. Building the project produced the "internal error".

In my larger program I get an "undefined symbol error". If need be I can send you the entire program, but I think this simplified fragment should get you to the problem.
Additional Informationprogram undefined_symbol_bug;

uses SysUtils;

procedure p;
   begin
      try
      except
         on e: Exception do
            try
               EXIT
            finally
            end
      end
   end;

begin
end.
Tagscompiler, exceptions, linker
Fixed in Revision
FPCOldBugId
FPCTarget3.2.0
Attached Files
  • undefined_symbol_bug.pp (220 bytes)
    program undefined_symbol_bug;
    
    procedure p;
       begin
          try
    	    WriteLn('Test');
          except
            try
    		  Exit;
    		finally
    	      WriteLn('Test Final');
    		end;
          end;
       end;
    
    begin
      p;
    end. 
  • tw32913a.pp (290 bytes)
    { %opt=-O2 }
    
    program tw32913a;
    
    {$mode objfpc}
    
    procedure NullProc;
      begin
      end;
    
    procedure p;
      begin
        try
          NullProc;
        except
          try
            Exit;
          finally
            NullProc;
          end;
        end;
      end;
    
    
    begin
      p();
      WriteLn('ok');
    end. 
    
    tw32913a.pp (290 bytes)
  • tw32913b.pp (357 bytes)
    { %opt=-O2 }
    
    program tw32913b;
    
    {$mode objfpc}
    
    uses
      SysUtils;
    
    procedure NullProc;
      begin
      end;
    
    procedure p;
      begin
        try
          NullProc;
    	  raise Exception.Create('Test exception');
        except
          try
            Exit;
          finally
            NullProc;
          end;
        end;
      end;
    
    
    begin
      p();
      WriteLn('ok');
    end. 
    
    tw32913b.pp (357 bytes)
  • tw32913c.pp (273 bytes)
    { %opt=-O2 }
    
    program tw32913c;
    
    {$mode objfpc}
    
    procedure NullProc;
      begin
      end;
    
    procedure p;
      begin
        try
        except
          try
            Exit;
          finally
            NullProc;
          end;
        end;
      end;
    
    
    begin
      p();
      WriteLn('ok');
    end. 
    
    tw32913c.pp (273 bytes)
  • i32913.patch (6,461 bytes)
    Index: compiler/nflw.pas
    ===================================================================
    --- compiler/nflw.pas	(revision 42348)
    +++ compiler/nflw.pas	(working copy)
    @@ -187,7 +187,6 @@
               constructor create(l,r,_t1 : tnode);virtual;reintroduce;
               function pass_typecheck:tnode;override;
               function pass_1 : tnode;override;
    -          function simplify(forinline: boolean): tnode; override;
              protected
               procedure adjust_estimated_stack_size; virtual;
            end;
    @@ -2373,10 +2372,46 @@
     
     
         function ttryexceptnode.pass_1 : tnode;
    +      var
    +        old_procinfo_flags: tprocinfoflags;
    +        oldstacksize: longint;
    +        oldpushedparasize: aint;
    +        olddeadnodes: boolean;
    +        nocode: boolean;
           begin
             result:=nil;
             expectloc:=LOC_VOID;
             firstpass(left);
    +
    +        { i32913 - if a try...finally block is nested inside an exception handler,
    +          the try part of the except block is empty, and Exit is called in the try
    +          part of the finally block, an internal error is raised. While removing
    +          the exception block if there is no code on the left node provides a
    +          massive performance boost, we still need to process "right" and "t1" for
    +          syntax errors, otherwise tests/tbf/tb0089.pp and tests/tb0090.pp will
    +          fail.  To prevent internal error 200309201 from being triggered later
    +          on, we need to make sure that the procinfo's flags remain correct but
    +          also that no code gets placed into the exception filter. [Kit] }
    +
    +        nocode := has_no_code(left);
    +        if nocode then
    +          begin
    +            { Make a note of the current procinfo's flags, along with the
    +              estimated parameter and stack sizes.  If the except block has a
    +              procedure call or a try..finally block, these values will be
    +              changed unnecessarily (or falsely in the case of the flags). }
    +            old_procinfo_flags:=current_procinfo.flags;
    +            oldstacksize:=current_procinfo.estimatedtempsize;
    +            oldpushedparasize:=current_procinfo.maxpushedparasize;
    +
    +            { Everything in the except block is dead code, but we still need to
    +              perform a first pass on the nodes there for syntactic reasons, so
    +              set a flag that prevents code generation that might otherwise
    +              trigger internal error 200309201. }
    +            olddeadnodes:=current_procinfo.current_nodes_dead;
    +            current_procinfo.current_nodes_dead:=True;
    +          end;
    +
             { on statements }
             if assigned(right) then
               firstpass(right);
    @@ -2384,19 +2419,29 @@
             if assigned(t1) then
               firstpass(t1);
     
    -        include(current_procinfo.flags,pi_do_call);
    -        include(current_procinfo.flags,pi_uses_exceptions);
    +        if nocode then
    +          begin
    +            { empty try -> can never raise exception -> do nothing }
    +            result:=cnothingnode.create;
     
    -        adjust_estimated_stack_size;
    -      end;
    +            { Restore the procinfo flags and the estimated parameter and stack
    +              sizes in case anything in the except block added an unused call
    +              or finally block etc. }
    +            current_procinfo.flags:=old_procinfo_flags;
    +            current_procinfo.estimatedtempsize:=oldstacksize;
    +            current_procinfo.maxpushedparasize:=oldpushedparasize;
     
    +            { No longer dealing with dead nodes associated with this empty try
    +              section, so revert it to the old values }
    +            current_procinfo.current_nodes_dead:=olddeadnodes;
    +          end
    +        else
    +          begin
    +            include(current_procinfo.flags,pi_do_call);
    +            include(current_procinfo.flags,pi_uses_exceptions);
     
    -    function ttryexceptnode.simplify(forinline: boolean): tnode;
    -      begin
    -        result:=nil;
    -        { empty try -> can never raise exception -> do nothing }
    -        if has_no_code(left) then
    -          result:=cnothingnode.create;
    +            adjust_estimated_stack_size;
    +          end;
           end;
     
     
    @@ -2457,20 +2502,34 @@
             if assigned(third) then
               firstpass(third);
     
    -        include(current_procinfo.flags,pi_do_call);
    +        { If this flag is set, then the entire try..finally block can never be
    +          reached, so don't set the flags or adjust the stack size }
    +        if not current_procinfo.current_nodes_dead then
    +          begin
    +            include(current_procinfo.flags,pi_do_call);
     
    -        { pi_uses_exceptions is an information for the optimizer and it
    -          is only interested in exceptions if they appear inside the body,
    -          so ignore implicit frames when setting the flag }
    -        if not(implicitframe) then
    -          include(current_procinfo.flags,pi_uses_exceptions);
    +            { pi_uses_exceptions is an information for the optimizer and it
    +              is only interested in exceptions if they appear inside the body,
    +              so ignore implicit frames when setting the flag }
    +            if not(implicitframe) then
    +              include(current_procinfo.flags,pi_uses_exceptions);
     
    -        adjust_estimated_stack_size;
    +            adjust_estimated_stack_size;
    +          end;
           end;
     
     
        function ttryfinallynode.simplify(forinline : boolean): tnode;
          begin
    +       { Required so code is still parsed but a finally block isn't generated
    +         if it's in an except block that is being stripped (i32913) - doing so
    +         may trigger internal error 200309201. [Kit] }
    +       if current_procinfo.current_nodes_dead then
    +         begin
    +           result:=cnothingnode.create;
    +           Exit;
    +         end;
    +
            result:=nil;
            { if the try contains no code, we can kill
              the try and except and return only the
    Index: compiler/procinfo.pas
    ===================================================================
    --- compiler/procinfo.pas	(revision 42348)
    +++ compiler/procinfo.pas	(working copy)
    @@ -136,6 +136,9 @@
                 Requires different entry code for some targets. }
               ConstructorCallingConstructor: boolean;
     
    +          { set if the nodes in the current block have been determined to be unreachable }
    +          current_nodes_dead: boolean;
    +
               constructor create(aparent:tprocinfo);virtual;
               destructor destroy;override;
     
    
    i32913.patch (6,461 bytes)

Relationships

parent of 0035857 new [Patch] Node semantic pass and supporting code 
related to 0035841 feedbackFlorian Split from 0032913 - Nested try blocks with Exit cause assembler error 
Not all the children of this issue are yet resolved or closed.

Activities

Thaddy de Koning

2017-12-31 12:23

reporter   ~0105183

Last edited: 2017-12-31 12:24

View 2 revisions

The code is wrong:
{$mode delphi}{$H+}
program undefined_symbol_bug;

uses SysUtils;

function p:longint;
begin
 Result := 0;
 // allocate resource here
 try
   try
   // process
   except
     on e: Exception do
     Exit(ErrorCode{that you must define!});
   end;
 finally
   //release resource
 end;
end;

begin
end.

This belongs on the forum. Although it should not give an internal error.

J. Gareth Moreton

2017-12-31 14:13

developer   ~0105189

I would consider putting a try..finally block in the exception handler to be questionable at best, but not erroneous unless the compiler explicitly forbids it.

Internal errors are effectively assertions and are automatically bugs if triggered, no matter what code is passed into it.

David Hawk

2017-12-31 15:14

reporter   ~0105192

To: Thaddy de Koning

Thanks for your comment.

I believe the code I submitted is correct. Exit only takes a parameter if exiting from a function (as you changed it to). My example called exit from a procedure.

See: https://www.freepascal.org/docs-html/rtl/system/exit.html

Again, this worked on a previous version of Lazarus and works in all versions of Delphi I have tried.

In any case the compiler should not generate an internal error.

Thaddy de Koning

2017-12-31 17:02

reporter   ~0105200

That's true. An internal error always warrants a bug report. Sorry if I misunderstood. I merely focused on the bogus try/finally.

Note that this also does not raise an internal error:
{$mode delphi}{$H+}
program undefined_symbol_bug;
uses SysUtils;
procedure p;
begin
 // allocate resource here
 try
   try
   // process
   except
     on e: Exception do
     Exit
   end;
 finally
   //release resource
 end;
end;

begin
end.

Marco van de Voort

2017-12-31 17:40

manager   ~0105201

Keep in mind that an IE is always a bug, even if the code is not sane.

David Hawk

2018-01-01 15:20

reporter   ~0105217

The example was the most concise code I could come up with that triggered the internal error in the compiler. It wasn't intended as a textbook example of the use of nested try blocks. Pascal generally does allow nested constructs and this is a part of the language's power. I assure you all that my code (from which I extracted this) is insanely elegant...

David Hawk

2018-05-18 17:33

reporter   ~0108413

Is there any way to raise the priority of this issue? It has been almost six months now and is a show stopper for my project (at least with regards for allowing it to compile with Lazarus - Delphi handles it just fine, of course). The ill-advised comment that my "code is wrong" seems to have derailed any effort at fixing it (it was classified as "minor" and no one has been assigned to it yet). I would expect other people will run into this bug too.

J. Gareth Moreton

2018-05-18 18:24

developer   ~0108414

Last edited: 2018-05-18 18:26

View 3 revisions

Escalated priority and severity. (Priority: "normal" and Severity: "minor" are the default settings)

While not the best answer, do you have a means to work around the issue? Can you switch the order so the try..finally block surrounds the try..except block instead? I'm guessing the finally section is for deallocating some memory, which is not a good idea to allocate in an exceptional situation anyway in case, for example, the exception is EOutOfMemory.

Nevertheless, this is a compiler bug just on account that it raised an internal error.

David Hawk

2018-05-18 19:00

reporter   ~0108415

Thanks for the escalation - much appreciated. I was afraid this bug might have been forgotten!

My workaround has been to continue development using Delphi and hope that I haven't introduced any FPC incompatibilities in the last six months. My build system used to fully test both Delphi and FPC functionality, but the FPC test has been disabled since I first encountered this bug.

My code is moderately complex in this area and I don't want to re-arrange it simply to work around a compiler bug. I fear I would probably introduce new bugs into good working code.

Thanks again. Let me know if there is any more information you need or if you want me to test any proposed fixes.

J. Gareth Moreton

2018-05-19 02:45

developer   ~0108423

Since this has been around for a while and is a blocking issue for some programmers, I'll take it upon myself to investigate this problem and see if I can fix the bug.

J. Gareth Moreton

2018-05-20 18:53

developer   ~0108451

Last edited: 2018-05-20 18:54

View 2 revisions

What I've found so far, using the following code example:

program undefined_symbol_bug;

uses SysUtils;

procedure p;
   begin
      try
        {a}
      except
        on e: Exception do
          try
            {b}
          finally
            {c}
          end;
      end;
   end;

begin
  p;
end.

{a}, {b} and {c} are replaced with simple WriteLn or Exit commands, or left blank.

- If {a} is empty and {b} is not empty, then Internal Error 200309201 is raised. Value of {c} doesn't matter, and the error doesn't happen if the 'try...finally' block is removed and replaced with other code. Error still occurs if the "on e: Exception do" line is removed.
- If {a} is not empty, then the program normally compiles whether or not {b} or {c} are empty, except in one circumstance...
- If {b} contains "Exit", then the assembly produced is missing a label and fails to link.

Do-wan Kim

2018-05-25 01:59

reporter   ~0108521

Last edited: 2018-05-25 03:29

View 4 revisions

I found easy way for avoiding internal error.
But I don't check generated code.

Index: compiler/nflw.pas
===================================================================
--- compiler/nflw.pas (revision 39112)
+++ compiler/nflw.pas (working copy)
@@ -2279,7 +2279,7 @@
       begin
         result:=nil;
         { empty try -> can never raise exception -> do nothing }
- if has_no_code(left) then
+ if has_no_code(left) and has_no_code(right) and has_no_code(t1) then
           result:=cnothingnode.create;
       end;

(edit)
Generated code seems to be ok.

{a} empty and {b} exit case

.section .text.n_p$test_trynested_$$_p,"x"
    .balign 16,0x90
.globl P$TEST_TRYNESTED_$$_P
P$TEST_TRYNESTED_$$_P:
# Temps allocated between ebp-4 and ebp+0
# [9] begin
    pushl %ebp
    movl %esp,%ebp
    leal -4(%esp),%esp
    pushl %ebx
    pushl %esi
    pushl %edi
# [10] try
    xorl %eax,%eax
    pushl $.La1
    pushl %ebp
    pushl $__FPC_on_handler
    pushl %fs:(%eax)
    movl %esp,%fs:(%eax)
    xorl %edx,%edx
    popl %eax
    addl $12,%esp
    movl %eax,%fs:(%edx)
    jmp .Lj10
.Lj12:
    movl %eax,-4(%ebp)
# [14] try
    xorl %eax,%eax
    pushl $P$TEST_TRYNESTED$_$P_$$_fin$0
    pushl %ebp
    pushl $__FPC_finally_handler
    pushl %fs:(%eax)
    movl %esp,%fs:(%eax)
# [15] exit;
    jmp .Lj15
    xorl %eax,%eax
    popl %edx
    addl $12,%esp
    movl %edx,%fs:(%eax)
    movl %ebp,%eax
    call P$TEST_TRYNESTED$_$P_$$_fin$0
    jmp .Lj14
.Lj15:
    call _FPC_leave
    jmp .Lj8
.Lj14: // after except
    call FPC_DONEEXCEPTION
    jmp .Lj10
.Lj8: // after finally
    call FPC_DONEEXCEPTION
    jmp .Lj3
.Lj10:
.Lj3:
# [19] end;
    popl %edi
    popl %esi
    popl %ebx
    movl %ebp,%esp
    popl %ebp
    ret

J. Gareth Moreton

2018-05-28 10:21

developer   ~0108556

Internal error is no longer present, but it still fails to link if {a} is not empty. .Lj7 gets removed or is not inserted at all.

Do-wan Kim

2018-05-29 04:06

reporter   ~0108566

Last edited: 2018-05-29 04:08

View 2 revisions

I think main problem is try except no code check.
If no code on try-except, it make to cnothingnode.
Try-finally section always make code for it and it make code on except-end section but parent try-except node is nothing and missing label for it.

if try-except code is nothing, it also removes statement in except-end section for code generation. it is dead code and don't be reachable.

J. Gareth Moreton

2018-05-29 04:57

developer   ~0108567

If try-except code is nothing ({a} is empty), your fix works like a charm. However, if {a} is not empty - that is, there is code in the try-except block - then the label is missing and the module fails to link.

Nevertheless, good work so far. Hopefully we can fix this problem in its entirety.

Do-wan Kim

2018-05-30 01:33

reporter   ~0108580

Ah, I don't know about it.
Thank you for explaining :)

J. Gareth Moreton

2018-05-30 02:20

developer  

undefined_symbol_bug.pp (220 bytes)
program undefined_symbol_bug;

procedure p;
   begin
      try
	    WriteLn('Test');
      except
        try
		  Exit;
		finally
	      WriteLn('Test Final');
		end;
      end;
   end;

begin
  p;
end. 

J. Gareth Moreton

2018-05-30 02:21

developer   ~0108581

Last edited: 2018-05-30 02:25

View 2 revisions

Find attached the source file that I've been using as a test case. As it stands, this happens if you try to compile it:

Free Pascal Compiler version 3.1.1 [2018/05/30] for x86_64
Copyright (c) 1993-2018 by Florian Klaempfl and others
Note: Switching assembler to default source writing assembler
Target OS: Win64 for x64
Compiling C:\Users\NLO-012\Documents\Programming\fpc\compiler\undefined_symbol_bug.pp
Assembling undefined_symbol_bug
Linking C:\Users\NLO-012\Documents\Programming\fpc\compiler\undefined_symbol_bug.exe
undefined_symbol_bug.pp(11,28) Error: Undefined symbol: .Lj7
undefined_symbol_bug.pp(11,28) Fatal: There were 1 errors compiling module, stopping

(The reported file position seems to be unreliable, as that points to the end of the string in the second "WriteLn"... or it might be a clue as to which jump is faulty)

David Hawk

2018-06-14 16:29

reporter   ~0108908

(Sorry - we are in the process of moving so I haven't checked in on this for a couple of weeks)

For what it is worth, in my main program {a}, {b} and {c} are definitely non-empty, and the undefined label is referenced by the EXIT statement.

Sounds like you are making progress on this. I want to thank both of you for looking into it.

Dave

J. Gareth Moreton

2018-07-14 03:33

developer   ~0109440

I just found something interesting. In the error message file, there's this listing:

cg_e_control_flow_outside_finally=06040_E_Control flow statements are not allowed in a finally block
% It isn't allowed to use the control flow statements \var{break},
% \var{continue} and \var{exit}
% inside a finally statement. The following example shows the problem:
% \begin{verbatim}
% ...
% try
% p;
% finally
% ...
% exit; // This exit ISN'T allowed
% end;
% ...
%
% \end{verbatim}
% If the procedure \var{p} raises an exception the finally block is
% executed. If the execution reaches the exit, it's unclear what to do:
% exit the procedure or search for another exception handler.

It seems that Free Pascal is not supposed to support the presence of "Exit" inside try..finally blocks. Internally, try..finally blocks are treated as nested procedures so the finally part is correctly executed if an exception occurs, and as the hint text suggests, the behaviour of Exit is unclear.

Then again, you state that Delphi supports the use of Exit in the finally section, and it behaves correctly. I might have to ask Florian and the other developers about this one.

Thaddy de Koning

2018-07-14 07:45

reporter   ~0109442

Correct. Exit is currently not allowed in finally blocks. Hence my first example.
Doesn't explain internal error, though.

David Hawk

2018-07-26 20:20

reporter   ~0109693

Interesting, but this might be a tangential issue. Remember the original problem code was:
      try
      except
         on e: Exception do
            try
               EXIT
            finally
            end
      end

(IMHO, the compiler should handle exit in a finally block as well, but that is a separate issue)

J. Gareth Moreton

2019-07-12 09:23

developer   ~0117209

I thought I'd give an update. I've managed to fix the original issue at long last - it turns out the relevant exit label was never created - but I've got a heap of regression failures so I'm trying to see what's going on with those and if they are actual failures or just a problem with my test configuration.

Marc

2019-07-12 09:53

reporter   ~0117211

Thank you for devoting your time to this.

J. Gareth Moreton

2019-07-12 14:54

developer   ~0117220

Last edited: 2019-07-12 14:57

View 2 revisions

This was harder than I would have given it credit for, not just because it was actually two separate problems. If a try section of a try..except block was empty, it would convert it to a "nothing node" because it's just wasted effort to set up an exception handler, but if that handler had a finally block in it, it already wrote code to an implicit nested procedure, and because it wasn't expecting it, would raise an internal error later.

The initial solution was to simply not parse the nodes in the except section if the try section was empty, but this caused tests tbf/tb0089.pp and tbf/tb0090.pp to fail because they contained a goto inside the exception handler to a label outside, thus betraying the fact that not parsing the nodes effectively means not doing proper syntax checking on them. Long story short, the nodes had to be parsed, but there had to be a way to not generate any code.

In the end, I had to add a new feature to the compiler that marked if the node parser was dealing with 'dead code'. At the moment, all it does is cause finally blocks to be treated differently, but could be used as a possible optimisation technique for future development.

I just hope I haven't broken any rules regarding how "pass_1" and "simplify" should be used.

For the issue of the exit label not being generated, the variable that keeps track of flow control will now cascade the "exit" flag when within a try..except or try..finally block. Everything seems to behave as it should so far.

J. Gareth Moreton

2019-07-12 15:15

developer   ~0117221

P.S. The patch also contains an improved version of "undefined_symbol_bug.pp" as an actual test, although looking at it now, the test in the patch should probably be split into 3 (so it's just one procedure per test that tests different combinations of try..except and try..finally).

Sven Barth

2019-07-12 19:08

manager   ~0117225

I think we should split this issue. David's issue is with Exit not working (as he wrote in his real code {a}, {b} and {c} are all non-empty even if that's only true for {b} in his example code). So we should use *this* issue for the Exit problem and open a new issue for the empty try block which thankfully doesn't happen that often "in the wild" anyway (otherwise we'd have more bug reports already).

Also it's correct that both FPC and Delphi do not allow control flow statements (break, continue, exit) inside the finally-clause of a try-finally-block. The idea of a finally-clause is to *always* be executed (with the exception of calling Halt, but Halt is kinda a sledgehammer in that regard...) and thus allowing control flow statements in there is kinda counter productive.

J. Gareth Moreton

2019-07-12 19:20

developer   ~0117226

You might be right about splitting the issue - it's easy enough to split because the fixes are located in different files in the patch.

There are two different bugs, but which are triggered in very similar ways. The thing is, the Exit error also occurred in David's issue, but was hidden behind the internal error.

J. Gareth Moreton

2019-07-12 19:40

developer   ~0117227

Split off the fixes to the Exit bug into 0035841. Also split the test into three.

tw32913c.pp tests the empty try section.

tw32913a.pp (290 bytes)
{ %opt=-O2 }

program tw32913a;

{$mode objfpc}

procedure NullProc;
  begin
  end;

procedure p;
  begin
    try
      NullProc;
    except
      try
        Exit;
      finally
        NullProc;
      end;
    end;
  end;


begin
  p();
  WriteLn('ok');
end. 
tw32913a.pp (290 bytes)
tw32913b.pp (357 bytes)
{ %opt=-O2 }

program tw32913b;

{$mode objfpc}

uses
  SysUtils;

procedure NullProc;
  begin
  end;

procedure p;
  begin
    try
      NullProc;
	  raise Exception.Create('Test exception');
    except
      try
        Exit;
      finally
        NullProc;
      end;
    end;
  end;


begin
  p();
  WriteLn('ok');
end. 
tw32913b.pp (357 bytes)
tw32913c.pp (273 bytes)
{ %opt=-O2 }

program tw32913c;

{$mode objfpc}

procedure NullProc;
  begin
  end;

procedure p;
  begin
    try
    except
      try
        Exit;
      finally
        NullProc;
      end;
    end;
  end;


begin
  p();
  WriteLn('ok');
end. 
tw32913c.pp (273 bytes)

J. Gareth Moreton

2019-07-12 19:49

developer   ~0117228

Note the tests will likely not pass unless the patch over at 0035841 is also applied.

Florian

2019-07-12 21:32

administrator   ~0117232

I think the patch cannot applied this way:
- in pass_1, we are not parsing anymore, so parsingdeadcode is named wrong
- in general, such state variables like parsingdeadcode are a bad design and should be avoided
- storing current_procinfo partially (random fields, why those, why not others?) is not a good idea

J. Gareth Moreton

2019-07-13 00:03

developer   ~0117233

What would you suggest instead, Florian? I've learnt the hard way that I have to call pass_1 on all the nodes to ensure all the syntax checks are done, but allowing code to be generated in the above situation causes the internal error to be triggered.

How should a state be stored in this case? It can't really go in tprocinfo.flags because of its transient nature.

J. Gareth Moreton

2019-07-13 05:03

developer   ~0117237

Last edited: 2019-07-13 05:19

View 4 revisions

I've refactored the patch a bit, but honestly, I can't see any other way to fix this issue. These are the other things I have tried in the case of a try..except node with an empty try (left) section:

- Not removing the except section at all. Causes a performance penalty in the compiled binary due to the extra stack unwinding required for the unnecessary exception handler.
- Not running firstpass on the right and t1 nodes. Correct code generation, but causes test regressions because the code in the except block isn't fully syntax-checked.

The original internal error was caused by the ttryexceptnode.simplify method replacing everything with a nothing node, but there being code in the po_exceptfilter procedure that the compiler, as a result, doesn't expect. The solution is to catch the situation and not set the 'code' field, but the state needs to be communicated between two separate objects (the try..except node realises its try section is empty and flags that, then the try..finally node, which knows nothing about the try..except node, needs to pick that up somehow - P.S. this "parsingdeadcode" flag has been renamed to "current_nodes_dead").

I've hopefully explained via comments why I'm only doing a partial state restore on current_procinfo - the try...finally block sets a number of flags that no longer apply if the entire node is to be stripped out, while the estimated stack size and parameter size is increased by the calls to the finally block that will be stripped out, so they need reverting.

It could be argued that instead of reverting these fields, the nodes should not call "adjust_estimated_stack_size" and related methods when the 'current_nodes_dead' flag is set, but these might be overridden on some platform implementations and would require a deeper overhaul.

If this approach is not acceptable, then I humbly call upon you or another administrator for assistance, because I can't yet see another viable solution that doesn't cause something else to fail. I suppose my conundrum is this... we have a state that we need to communicate between different nodes that can't directly access each other's data - what's a good way design-wise to implement this?

ADDENDUM: If there's concern about the 'current_nodes_dead' flag introducing bugs, one option might be to check its state at the end of "do_firstpass" and trigger an internal error if it's still True by that point. The other option is to merge the flag with the 'flowcontrol' variable, but this is declared in pass_2 while this particular information is dealt with in pass 1. I also don't particularly like how 'flowcontrol' is set-up because this is even worse than 'current_nodes_dead'... it's a global variable not associated with any object or class.



i32913.patch (6,461 bytes)
Index: compiler/nflw.pas
===================================================================
--- compiler/nflw.pas	(revision 42348)
+++ compiler/nflw.pas	(working copy)
@@ -187,7 +187,6 @@
           constructor create(l,r,_t1 : tnode);virtual;reintroduce;
           function pass_typecheck:tnode;override;
           function pass_1 : tnode;override;
-          function simplify(forinline: boolean): tnode; override;
          protected
           procedure adjust_estimated_stack_size; virtual;
        end;
@@ -2373,10 +2372,46 @@
 
 
     function ttryexceptnode.pass_1 : tnode;
+      var
+        old_procinfo_flags: tprocinfoflags;
+        oldstacksize: longint;
+        oldpushedparasize: aint;
+        olddeadnodes: boolean;
+        nocode: boolean;
       begin
         result:=nil;
         expectloc:=LOC_VOID;
         firstpass(left);
+
+        { i32913 - if a try...finally block is nested inside an exception handler,
+          the try part of the except block is empty, and Exit is called in the try
+          part of the finally block, an internal error is raised. While removing
+          the exception block if there is no code on the left node provides a
+          massive performance boost, we still need to process "right" and "t1" for
+          syntax errors, otherwise tests/tbf/tb0089.pp and tests/tb0090.pp will
+          fail.  To prevent internal error 200309201 from being triggered later
+          on, we need to make sure that the procinfo's flags remain correct but
+          also that no code gets placed into the exception filter. [Kit] }
+
+        nocode := has_no_code(left);
+        if nocode then
+          begin
+            { Make a note of the current procinfo's flags, along with the
+              estimated parameter and stack sizes.  If the except block has a
+              procedure call or a try..finally block, these values will be
+              changed unnecessarily (or falsely in the case of the flags). }
+            old_procinfo_flags:=current_procinfo.flags;
+            oldstacksize:=current_procinfo.estimatedtempsize;
+            oldpushedparasize:=current_procinfo.maxpushedparasize;
+
+            { Everything in the except block is dead code, but we still need to
+              perform a first pass on the nodes there for syntactic reasons, so
+              set a flag that prevents code generation that might otherwise
+              trigger internal error 200309201. }
+            olddeadnodes:=current_procinfo.current_nodes_dead;
+            current_procinfo.current_nodes_dead:=True;
+          end;
+
         { on statements }
         if assigned(right) then
           firstpass(right);
@@ -2384,19 +2419,29 @@
         if assigned(t1) then
           firstpass(t1);
 
-        include(current_procinfo.flags,pi_do_call);
-        include(current_procinfo.flags,pi_uses_exceptions);
+        if nocode then
+          begin
+            { empty try -> can never raise exception -> do nothing }
+            result:=cnothingnode.create;
 
-        adjust_estimated_stack_size;
-      end;
+            { Restore the procinfo flags and the estimated parameter and stack
+              sizes in case anything in the except block added an unused call
+              or finally block etc. }
+            current_procinfo.flags:=old_procinfo_flags;
+            current_procinfo.estimatedtempsize:=oldstacksize;
+            current_procinfo.maxpushedparasize:=oldpushedparasize;
 
+            { No longer dealing with dead nodes associated with this empty try
+              section, so revert it to the old values }
+            current_procinfo.current_nodes_dead:=olddeadnodes;
+          end
+        else
+          begin
+            include(current_procinfo.flags,pi_do_call);
+            include(current_procinfo.flags,pi_uses_exceptions);
 
-    function ttryexceptnode.simplify(forinline: boolean): tnode;
-      begin
-        result:=nil;
-        { empty try -> can never raise exception -> do nothing }
-        if has_no_code(left) then
-          result:=cnothingnode.create;
+            adjust_estimated_stack_size;
+          end;
       end;
 
 
@@ -2457,20 +2502,34 @@
         if assigned(third) then
           firstpass(third);
 
-        include(current_procinfo.flags,pi_do_call);
+        { If this flag is set, then the entire try..finally block can never be
+          reached, so don't set the flags or adjust the stack size }
+        if not current_procinfo.current_nodes_dead then
+          begin
+            include(current_procinfo.flags,pi_do_call);
 
-        { pi_uses_exceptions is an information for the optimizer and it
-          is only interested in exceptions if they appear inside the body,
-          so ignore implicit frames when setting the flag }
-        if not(implicitframe) then
-          include(current_procinfo.flags,pi_uses_exceptions);
+            { pi_uses_exceptions is an information for the optimizer and it
+              is only interested in exceptions if they appear inside the body,
+              so ignore implicit frames when setting the flag }
+            if not(implicitframe) then
+              include(current_procinfo.flags,pi_uses_exceptions);
 
-        adjust_estimated_stack_size;
+            adjust_estimated_stack_size;
+          end;
       end;
 
 
    function ttryfinallynode.simplify(forinline : boolean): tnode;
      begin
+       { Required so code is still parsed but a finally block isn't generated
+         if it's in an except block that is being stripped (i32913) - doing so
+         may trigger internal error 200309201. [Kit] }
+       if current_procinfo.current_nodes_dead then
+         begin
+           result:=cnothingnode.create;
+           Exit;
+         end;
+
        result:=nil;
        { if the try contains no code, we can kill
          the try and except and return only the
Index: compiler/procinfo.pas
===================================================================
--- compiler/procinfo.pas	(revision 42348)
+++ compiler/procinfo.pas	(working copy)
@@ -136,6 +136,9 @@
             Requires different entry code for some targets. }
           ConstructorCallingConstructor: boolean;
 
+          { set if the nodes in the current block have been determined to be unreachable }
+          current_nodes_dead: boolean;
+
           constructor create(aparent:tprocinfo);virtual;
           destructor destroy;override;
 
i32913.patch (6,461 bytes)

Florian

2019-07-13 11:29

administrator   ~0117241

If it is only fixable by some hacky (sorry) patch, then a "bigger" solution is needed. For example moving semantic error checking into a separate pass. After all, the source of the "evil" is that pass_1 does semantic checking (the goto target) and also (almost) irreversible node transformations.

J. Gareth Moreton

2019-07-13 12:03

developer   ~0117244

Normally I would agree, but I remember that the work I did for the "as"/"is" feature over at 0033603 tends to combine the syntactic checking with the node transformations because very different nodes can be generated depending on what the compiler finds.

I didn't intend for the patch to be hacky, although I can see the issue since, currently, the "current_nodes_dead" is only used to fix this particular issue and any optimisation potential is nothing more than future speculation.

If it can be implemented in a well-designed way, I would like a way to flag if the current nodes are effectively unreachable (e.g. in this particular arrangement, or anything following an "Exit" node in the same block) because that means one doesn't have to run "pass_1" on them at all, just the syntactic checking (which is all that is required for tbf/tb0089.pp and tbf/tb0090.pp to pass). I am thinking ahead a bit in terms of making it fast as well as correct, but one thing that can help there is to introduce new private fields for the nodes that the syntax pass can fill in and can be picked up by the pass_1 if it's executed.

Since this will require a lot of thought and a potential overhaul though, I think this should be discussed more openly and deeply.

In the meantime, the other half of the fix over at 0035841 should be fine and fix the more common issue.

J. Gareth Moreton

2019-07-13 12:17

developer   ~0117245

P.S. Thank you Florian for taking the time to respond and start the ball rolling with finding a solution, because up until your last post, I was at a complete loss... I thought it was either this "hacky" patch, leave in the internal error, or allow a regression, none of which are very appealing.

Issue History

Date Modified Username Field Change
2017-12-31 03:45 David Hawk New Issue
2017-12-31 12:23 Thaddy de Koning Note Added: 0105183
2017-12-31 12:24 Thaddy de Koning Note Edited: 0105183 View Revisions
2017-12-31 14:13 J. Gareth Moreton Note Added: 0105189
2017-12-31 15:14 David Hawk Note Added: 0105192
2017-12-31 17:02 Thaddy de Koning Note Added: 0105200
2017-12-31 17:40 Marco van de Voort Note Added: 0105201
2018-01-01 15:20 David Hawk Note Added: 0105217
2018-05-18 17:33 David Hawk Note Added: 0108413
2018-05-18 18:24 J. Gareth Moreton Note Added: 0108414
2018-05-18 18:24 J. Gareth Moreton Priority normal => high
2018-05-18 18:24 J. Gareth Moreton Severity minor => major
2018-05-18 18:25 J. Gareth Moreton Note Edited: 0108414 View Revisions
2018-05-18 18:26 J. Gareth Moreton Note Edited: 0108414 View Revisions
2018-05-18 19:00 David Hawk Note Added: 0108415
2018-05-19 02:44 J. Gareth Moreton Assigned To => J. Gareth Moreton
2018-05-19 02:44 J. Gareth Moreton Status new => assigned
2018-05-19 02:45 J. Gareth Moreton Note Added: 0108423
2018-05-20 18:53 J. Gareth Moreton Note Added: 0108451
2018-05-20 18:54 J. Gareth Moreton Note Edited: 0108451 View Revisions
2018-05-25 01:59 Do-wan Kim Note Added: 0108521
2018-05-25 02:44 Do-wan Kim Note Edited: 0108521 View Revisions
2018-05-25 02:52 Do-wan Kim Note Edited: 0108521 View Revisions
2018-05-25 03:29 Do-wan Kim Note Edited: 0108521 View Revisions
2018-05-28 10:13 J. Gareth Moreton File Added: i32913.patch
2018-05-28 10:15 J. Gareth Moreton Assigned To J. Gareth Moreton => Marco van de Voort
2018-05-28 10:19 J. Gareth Moreton Assigned To Marco van de Voort => J. Gareth Moreton
2018-05-28 10:21 J. Gareth Moreton Note Added: 0108556
2018-05-28 10:22 J. Gareth Moreton File Deleted: i32913.patch
2018-05-29 04:06 Do-wan Kim Note Added: 0108566
2018-05-29 04:08 Do-wan Kim Note Edited: 0108566 View Revisions
2018-05-29 04:57 J. Gareth Moreton Note Added: 0108567
2018-05-30 01:33 Do-wan Kim Note Added: 0108580
2018-05-30 02:19 J. Gareth Moreton Tag Attached: compiler
2018-05-30 02:19 J. Gareth Moreton Tag Attached: exceptions
2018-05-30 02:19 J. Gareth Moreton Tag Attached: linker
2018-05-30 02:20 J. Gareth Moreton File Added: undefined_symbol_bug.pp
2018-05-30 02:21 J. Gareth Moreton Note Added: 0108581
2018-05-30 02:25 J. Gareth Moreton Note Edited: 0108581 View Revisions
2018-06-14 16:29 David Hawk Note Added: 0108908
2018-07-14 03:33 J. Gareth Moreton Note Added: 0109440
2018-07-14 07:45 Thaddy de Koning Note Added: 0109442
2018-07-26 20:20 David Hawk Note Added: 0109693
2019-07-12 09:23 J. Gareth Moreton Note Added: 0117209
2019-07-12 09:53 Marc Note Added: 0117211
2019-07-12 14:54 J. Gareth Moreton File Added: i32913.patch
2019-07-12 14:54 J. Gareth Moreton Note Added: 0117220
2019-07-12 14:54 J. Gareth Moreton Assigned To J. Gareth Moreton => Florian
2019-07-12 14:54 J. Gareth Moreton Status assigned => feedback
2019-07-12 14:54 J. Gareth Moreton FPCTarget => 3.2.0
2019-07-12 14:57 J. Gareth Moreton Note Edited: 0117220 View Revisions
2019-07-12 15:15 J. Gareth Moreton Note Added: 0117221
2019-07-12 19:08 Sven Barth Note Added: 0117225
2019-07-12 19:20 J. Gareth Moreton Note Added: 0117226
2019-07-12 19:34 J. Gareth Moreton Relationship added related to 0035841
2019-07-12 19:38 J. Gareth Moreton File Deleted: i32913.patch
2019-07-12 19:40 J. Gareth Moreton File Added: i32913.patch
2019-07-12 19:40 J. Gareth Moreton File Added: tw32913a.pp
2019-07-12 19:40 J. Gareth Moreton File Added: tw32913b.pp
2019-07-12 19:40 J. Gareth Moreton File Added: tw32913c.pp
2019-07-12 19:40 J. Gareth Moreton Note Added: 0117227
2019-07-12 19:49 J. Gareth Moreton Note Added: 0117228
2019-07-12 21:32 Florian Note Added: 0117232
2019-07-13 00:03 J. Gareth Moreton Note Added: 0117233
2019-07-13 04:46 J. Gareth Moreton File Deleted: i32913.patch
2019-07-13 05:03 J. Gareth Moreton File Added: i32913.patch
2019-07-13 05:03 J. Gareth Moreton Note Added: 0117237
2019-07-13 05:14 J. Gareth Moreton Note Edited: 0117237 View Revisions
2019-07-13 05:15 J. Gareth Moreton Note Edited: 0117237 View Revisions
2019-07-13 05:19 J. Gareth Moreton Note Edited: 0117237 View Revisions
2019-07-13 11:29 Florian Note Added: 0117241
2019-07-13 12:03 J. Gareth Moreton Note Added: 0117244
2019-07-13 12:17 J. Gareth Moreton Note Added: 0117245
2019-07-18 05:55 J. Gareth Moreton Relationship added parent of 0035857