[m-rev.] for review: fix half of Mantis bug #548

[Peter Wang]

On Thu, 10 Feb 2022 10:22:32 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> For review by anyone.
> 
> I can see four ways to fix the second problem in Mantis 548.
> Here they are, but this discussion makes sense only if you have read
> the comments in the new test case descrbing that problem.
> 
> 1. Change how we represent HLDS vars in the MLDS. At the moment,
> that representation, lvn_prog_var, consists of the HLDS variable's name
> and its number. This means that if we generate code twice for the same
> HLDS code, we will get the same MLDS name for every HLDS variable
> in that HLDS code. We could fix this by including in lvn_prog_var
> a unique integer, allocated from a counter when the MLDS variable
> is constructed, either in addition to, or in place of, the HLDS variable number.
> This should be simple to implement, but would make HLDS dumps
> harder to read, either by adding clutter (if we keep the HLDS variable number)
> or by removing a link to the HLDS (if we replace the HLDS variable number).
> 
> 2. We could keep locals vars as they are, but add the unique integer
> to the field name that we turn MLDS variables into. This would involve
> adding an integer, again allocated from a counter, to fvn_env_field_from_local_var.
> This may be somewhat harder to implement, because it breaks the current
> direct correspondence between the local var and field name versions
> of a HLDS variable.
> 
> 3. The name collision for field vars occurs because two different switch arms,
> which are independent pieces of code, use the same environment structure.
> We could change things so that independent pieces of code get separate
> environment structures. This change would be so complex that I definitely
> won't be doing it :-(
> 
> 4. As a compromise, when we construct the environment structure,
> we could replace whatever code appends the lists of local vars from
> different places to construct the list of fields to put into the environment
> structure with code that unions sets of local vars instead, so that
> even if two independent pieces of code specify the same local variable
> (as in the case of the code that tickled this bug), we won't get duplicate
> field names. This approach would probably not be too hard to implement,
> but I definitely wouldn't be able to write a convincing correctness
> argument in its favor :-(
> 
> Opinions?

1. Seems overkill to me.

2. Sounds okay to me (barring difficulty of implementation).

4. It sounds like a variation of (2) which avoids adding distinguishing
suffixes unless a name collision would arise, which would be a slightly
better result than (2), but may not be worth the extra complexity.
But you seem to suggest that (4) would be easier than (2).


> diff --git a/tests/hard_coded/Mercury.options b/tests/hard_coded/Mercury.options
> index de994ee36..5f39fdf27 100644
> --- a/tests/hard_coded/Mercury.options
> +++ b/tests/hard_coded/Mercury.options
> @@ -15,6 +15,7 @@ MCFLAGS-bug300 =		--no-const-struct --optimize-constructor-last-call
>  MCFLAGS-bug314 =		--trace deep
>  MCFLAGS-bug392 =		-O0 --deforestation
>  MCFLAGS-bug455_mod_a 		= --intermodule-optimization
> +MCFLAGS-bug458 =        --inlining

The module name is wrong. Perhaps --inlining is not necessary given the
pragma inline?

> +test_pred(T, List, A, N) :-
> +    (
> +        ( T = f1(_, _)
> +        ; T = f2(_, _)
> +        ; T = f3(_, _)
> +        ; T = f4(_, _)
> +        ; T = f5(_, _)
> +        ; T = f6(_, _)
> +        ; T = f7(_, _)
> +        ; T = f8(_, _)
> +        ),
> +        N = 11
> +    ;
> +        ( T = f0(_, _)
> +        ; T = f9(_, _)
> +        ),
> +        % The two bug we are testing for occurred when ml_tag_switch.m

two bugs

> +        % duplicated the code of this switch arm for two different
> +        % primary tag values, one unshared (as is f0) and one shared
> +        % (as is f9).
> +        %
> +        % One bug was that auxiliary MLDS functions inside divisible_by_some
> +        % were generated once and then used twice, resulting in duplicate
> +        % function definition.

definitions

> +        %
> +        % The other bug involved the fact that the local variables of this arm
> +        % are put into an environment structure by the implementation of the
> +        % commit implicit in the "some [I]" quantification inlined here.
> +        % When this arm is duplicated, we get duplicate field names in this
> +        % environment structure *even if we do not duplicate the MLDS code
> +        % of this case, but generate it fresh for each ptag*, because
> +        %
> +        % - the name of each MLDS variable representing a HLDS variable
> +        %   is constructed solely from the HLDS name and number of the HLDS
> +        %   variable, and
> +        %
> +        % - the name of each field in the environment structure that represents
> +        %   an MLDS variable is constructed solely from the name of that MLDS
> +        %   variable.
> +        %
> +        % At neither stage did we add any kind of suffix to ensure the
> +        % uniqueness of the name. (For the problem with duplicate aux function
> +        % definitions, generating the code of this arm fresh for each ptag
> +        % does fix the problem, because for aux MLDS functions, we *do*
> +        % add a unique numerical suffix to their name.)
> +        ( if divisible_by_some(List, A) then
> +            N = A
> +        else
> +            N = 42
> +        )
> +    ).

The rest looks fine.

Peter