[m-rev.] for review: fix mantis bug 572

[Julien Fischer]

On Tue, 20 Feb 2024, Zoltan Somogyi wrote:

> Fix a bug in liveness of if-then-elses that can't succeed.
> 
> This fixes Mantis bug #572.
> 
> compiler/liveness.m:
>     As above. The details are described in tests/valid/bug572.m.
> 
> compiler/hlds_out_goal.m:
>     Add a comment to direct readers to what they may be looking for.
> 
> tests/valid/bug572.m:
>     Add the test case for Mantis #572.
> 
> tests/valid/Mmakefile:
>     Enable the new test case.
> 
> diff --git a/compiler/hlds_out_goal.m b/compiler/hlds_out_goal.m
> index 3fe3668d3..52b5a2201 100644
> --- a/compiler/hlds_out_goal.m
> +++ b/compiler/hlds_out_goal.m
> @@ -47,6 +47,9 @@
>      % Print a goal in a way that is suitable for debugging the compiler
>      % (but necessarily for anything else).
>      %
> +    % If what you are after is a short, less-than-one-line description
> +    % of a goal, then look in hlds_des.m.

s/hlds_des/hlds_desc/

> +    %
>  :- pred dump_goal(io.text_output_stream::in, module_info::in,
>      var_name_source::in, tvarset::in, inst_varset::in, hlds_goal::in,
>      io::di, io::uo) is det.
> @@ -54,6 +57,9 @@
>      % Print a goal in a way that is suitable for debugging the compiler
>      % (but necessarily for anything else), followed by a newline.
>      %
> +    % If what you are after is a short, less-than-one-line description
> +    % of a goal, then look in hlds_des.m.

Again.

> +    %
>  :- pred dump_goal_nl(io.text_output_stream::in, module_info::in,
>      var_name_source::in, tvarset::in, inst_varset::in, hlds_goal::in,
>      io::di, io::uo) is det.

...

> diff --git a/compiler/liveness.m b/compiler/liveness.m
> index f6eab3564..dae075a08 100644
> --- a/compiler/liveness.m
> +++ b/compiler/liveness.m

...

> @@ -781,49 +814,69 @@ detect_deadness_in_goal(LiveInfo, !.Liveness, Goal0, Goal, !Deadness) :-

...

>          else
>              set_of_var.union(DeadnessCond, DeadnessElse, Deadness),
>              set_of_var.intersect(Deadness, CompletedNonLocals,
>                  CompletedNonLocalDeadness),
> -            InstmapReachable = no,
> +            % If the end of the if-then-else as a whole is not reachable,
> +            % then neither branch's end is reachable.
> +            BranchEndReachable = this_branch_end_is_not_reachable,
> +            % ZZZ

What's the ZZZ for?

>              add_branch_pre_deaths(DeadnessCond, Deadness0,
> -                CompletedNonLocalDeadness, InstmapReachable, Cond1, Cond),
> +                CompletedNonLocalDeadness, BranchEndReachable, _, Cond1, Cond),
>              add_branch_pre_deaths(DeadnessElse, Deadness0,
> -                CompletedNonLocalDeadness, InstmapReachable, Else1, Else)
> +                CompletedNonLocalDeadness, BranchEndReachable, _, Else1, Else)
>          ),
>          !:Deadness = Deadness,
>          GoalExpr = if_then_else(Vars, Cond, Then, Else),

...

> diff --git a/tests/valid/bug572.m b/tests/valid/bug572.m
> index e69de29bb..a316424c1 100644
> --- a/tests/valid/bug572.m
> +++ b/tests/valid/bug572.m
> @@ -0,0 +1,189 @@

...

> +% In this case, the sanity check that aborted the compiler was in the last
> +% pass, but the problem was caused by code in the second, which included
> +% TypeClassInfo_for_argument_21 in the post-death set of the goal that
> +% took out of it the type info variable for T, thus killing it before
> +% the end of the else part. It did this because this (compiler-generated)
> +% goal was the last reference it saw, and *due to all branches through
> +% the switch on TArgs throwing exceptions*, it knew that the end of the
> +% procedure body was not reachable. This reasoning was correct, but it
> +% disregarded the presence of the sanity checks in the last pass.
> +%
> +% I (zs) three possible approaches for fixing this bug.

*see* three possible approaches

> +% Approach one would simply disable all sanity checks that insist on
> +% agreement on the set of live vars at the ends of different branches
> +% in a branched control structure. This would include not just the sanity
> +% checks in the fourth pass in the liveness module, but all similar checks
> +% in all of the compiler's analysis and code generation modules.
> +% This would be discard a defense against bugs in quite a large part
> +% of the compiler, and hence would be a pretty bad idea.
> +%
> +% Approach two would be to make TypeClassInfo_for_argument_21 live at
> +% the end of the else part by adding it to the else part goal's post-birth set.
> +% The post-birth set is specifically designed for this use case; it is
> +% only ever used to tell the code generator "execution will never reach
> +% the end of this goal, but consider this variable to be born at the very end
> +% of it when comparing the set of live variables at the end of this branch
> +% with the livenesses at the ends of the other branches in this structure".
> +% This would almost certainly work, but it would mean that this variable
> +% first dies in the else part, and then gets born again. Besides this being
> +% inbelegant, the seeming resurrection of a previously-dead variable

s/inbelegant/inelegant/

> +% has also historically posed problems for the optional delay_death pass.
> +%
> +% The fix I (zs) chose uses approach three, which is to make
> +% TypeClassInfo_for_argument_21 live at the end of the else part
> +% by preventing it being killed in the first place. It was being killed
> +% by code that handled the arms of the branches control structures

s/branches/branched/

> +% (switches, disjunctions, or if-then-else) whose handling of arms that
> +% cannot succeed was set up to handle situations in which some *other* arm
> +% of the branched control structure *could* succeed. Its design seems to have
> +% considered the possibility that *none* of the branches could succeed, but
> +% the implementation did not :-( The fix is to make the implementation match
> +% the old design, which is: when we find that the current branch being
> +% considered cannot succeed but none of other branches can succeed either,
> +% then don't kill the variables that we used to kill.

That looks fine otherwise.

Julien.