[m-rev.] for review: fix the second problem in Mantis bug #548

[Julien Fischer]

On Fri, 11 Feb 2022, Zoltan Somogyi wrote:

> Don't let ml_tag_switch.m generate duplicate fields.
> 
> This fixes the second problem identified by Mantis bug #548.
> 
> compiler/ml_tag_switch.m:
>     Detect the circumstances in which this problem would arise.
>     In such cases, simply fail, and let ml_switch_gen.m fall back
>     to implementing the switch as an if-then-else chain.
> 
> compiler/ml_switch_gen.m:
>     Implement that fallback.
> 
> compiler/switch_util.m:
>     The new code in ml_tag_switch.m needs to thread a fourth piece of state
>     through the predicate it passes to group_cases_by_ptag, so change
>     its argument list to accommodate such predicates. And since some other
>     modules pass the same predicates to group_cases_by_ptag and
>     string_binary_cases, make the same change in the argument list
>     of that predicate as well.
>
>     Delete one stray comment, and note that another comment seems misplaced.
> 
> compiler/ml_string_switch.m:
> compiler/string_switch.m:
> compiler/switch_case.m:
> compiler/tag_switch.m:
>     Conform to the changes in switch_util.m.
> 
> tests/hard_coded/bug548.exp:
> tests/hard_coded/Mmakefile:
>     Enable the previously-added test case for Mantis #548, after
>     add an .exp file for it.

> diff --git a/compiler/ml_tag_switch.m b/compiler/ml_tag_switch.m
> index 78af03869..85a3f98f1 100644
> --- a/compiler/ml_tag_switch.m
> +++ b/compiler/ml_tag_switch.m

...

> @@ -125,54 +142,217 @@ ml_generate_tag_switch(TaggedCases, Var, CodeModel, CanFail,

...

> +        acc_dup_properties_of_stmt(Stmt,
> +            does_not_contain_label, HasLabel,
> +            will_not_gen_aux_pred, HasAuxPred,
> +            has_no_local_vars, HasLocalVars),
> +        ( if
> +            HasAuxPred = will_gen_aux_pred,
> +            HasLocalVars = has_local_vars
> +        then
> +            % Unlike labels and aux functions (see below), the names
> +            % of MLDS variables do not include a sequence number.
> +            % Therefore whether we return Stmt to be used duplicated as is,
> +            % or letting it be generated several times from scratch,
> +            % the resulting MLDS code will contain duplicate definitions
> +            % of the local variables indicated by has_local_vars,
> +            % and when ml_elim_nested moves all local variables *from all
> +            % the duplicated copies of the block that these came from*
> +            % to a *single* environment structure, the affected fields
> +            % of the environment structure will be doubly defined,
> +            % and the target language compiler will rightly report an error.
> +            % The simplest way to avoid this is use this setting of
> +            % !:MayUseTagSwitch to tell ml_generate_tag_switch_if_possible
> +            % to fail, letting ml_switch_gen.m fall back to an if-then-else
> +            % chain for thr switch. Slow but working target language code

s/thr/the/

> +            % beats "fast" but non-compilable target language code :-(
> +            % Given how long the problem we are guarding against here
> +            % has lurked in this code without being detected, the speed
> +            % of the code we generate in such rare cases are extremely
> +            % unlikely to matter in practice.
> +            !:MayUseTagSwitch = may_not_use_tag_switch,
> +            % Since we are forcing ml_generate_tag_switch_if_possible
> +            % to fail, the MaybeCode and Info values we return won't be used.
> +            MaybeCode = generate(EntryPackedArgsMap, Goal),
> +            Info = Info1
> +        else
> +            ( if
> +                ( HasLabel = does_contain_label
> +                ; HasAuxPred = will_gen_aux_pred
> +                )
> +            then
> +                % Stmt contains either the definition of either a label,
> +                % or an auxiliary function. We therefore cannot include
> +                % two copies Stmt in the code we generate for the switch,

    *of* Stmt

> +                % because that would leave the label or the aux function
> +                % multiply defined, but if we create MLDS code for Goal
> +                % in every branch, things will be fine, because each
> +                % time we generate code for Goal, any labels and aux functions
> +                % will have different sequence number included in their names.

    *a* different

or:

     different sequence numbers

That looks fine otherwise.

Julien.