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

[Julien Fischer]

Hi Zoltan,

On Fri, 24 Jul 2020, Zoltan Somogyi wrote:

> The attached diff has bootchecked in a deep profiling grade both with and without
> -fno-common, and I am just now testing it in other LLDS grades, but my laptop
> has gcc 7.5. Does anyone have gcc 10? If so, could you please try it out,
> either before or after review (by anyone) or commit?

I will try it out with GCC 10 over the weekend.  (I don't recall if I
have a VM lying about that has GCC 10; if not I will create one.)

> Fix the declarations of builtin proc layouts.
> 
> This should fix Mantis bug #514.
> 
> runtime/mercury_builtin_types_proc_layouts.h:
>     Fix the first problem: refer to four type constructors defined
>     in private_builtin.m by their correct arities, so that the proc layout
>     structures we generate for their unify and compare predicates
>     get generated with the correct names and contents.
>
>     The arities were probably correct when the code was written;
>     we changed them from 1 to 0 at some point, and simply forgot
>     to update all the affected places.
> 
> runtime/mercury_stack_layout.h:
>     Fix the second problem: add "extern" to the declarations of those
>     proc layout structures. Without the "extern", compilers defaulting
>     the -fcommon will place these structures into common storage,
>     which can be used as *definitions*. With the "extern", compilers
>     should do the right thing whether or not they default to -fcommon.

We should probably look at defaulting to -fno-common for GCC < 10 (and
clang too if required).  The GCC manual says there may be a performance
impact with -fcommon.

> runtime/mercury_unify_compare_body.h:
>     Conform to the change in mercury_builtin_types_proc_layouts.h:
>     refer to the affected four types using the correct arities.

There should be an entry for this fix in the NEWS file.
(You can leave that since this change will need to go onto the 20.06 branch anyway,
I'll add one when I add a NEWS section for 20.06.1.)

> diff --git a/runtime/mercury_builtin_types_proc_layouts.h b/runtime/mercury_builtin_types_proc_layouts.h
> index a868fc6f9..c893e7066 100644
> --- a/runtime/mercury_builtin_types_proc_layouts.h
> +++ b/runtime/mercury_builtin_types_proc_layouts.h
> @@ -64,10 +64,10 @@ MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(builtin, trailptr, 0);
>  MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(builtin, ticket, 0);
>  MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, heap_pointer, 0);
>  MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, ref, 1);
> -MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, type_ctor_info, 1);
> -MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, type_info, 1);
> -MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, base_typeclass_info, 1);
> -MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, typeclass_info, 1);
> +MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, type_ctor_info, 0);
> +MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, type_info, 0);
> +MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, base_typeclass_info, 0);
> +MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(private_builtin, typeclass_info, 0);
>  MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(type_desc, type_ctor_desc, 0);
>  MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(type_desc, pseudo_type_desc, 0);
>  MR_DECLARE_UCI_PROC_STATIC_LAYOUTS(type_desc, type_desc, 0);

Add a comment at the head of runtime/mercury_builtin_types.h pointing out that
all these places need to be updated if the arity of a builtin type changes.

That looks fine otherwise.

Julien.