[m-rev.] for review: fix bug #196: binary compatiblity checks in C grade don't work

[Paul Bone]

On Sun, Jul 17, 2016 at 01:49:49PM +1000, Julien Fischer wrote:
> 
> Hi Zoltan,
> 
> On Thu, 14 Jul 2016, Zoltan Somogyi wrote:
> >On Fri, 15 Jul 2016 01:16:25 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
> >>Is anyone intending to review this one?  In the absence of any comments
> >>I'll commit it later today.
> >
> >I thought I already reviewed the idea, and said that I would prefer
> >to fix the problem by exporting a function that refers to the value
> >of MR_GRADE_VAR. I think this would be more robust in the face
> >of compiler optimizations, though it does have the slight downside
> >of adding one symbol per module to the executable's symbol table.
> 
> The following diff implements the above.
> 
> Julien.
> 
> ---------------
> 
> Fix bug #196: binary compatibility checks in C grades do not work.
> 
> These have been broken since 2011 because GCC and clang (at least) began
> optimising away the MR_grade variable we emit at the end of each generated .c
> file.  The fix is to replace the MR_grade variable with an exported function
> that references MR_GRADE_VAR in each generated .c file.
> 
> compiler/llds_out_file.m:
> compiler/mlds_to_c.m:
> util/mkinit.c:
>      In each generated .c file, emit an exported function that references
>      MR_GRADE_VAR.

I just upgraded to ROTD 2016-07-18 and noticed this new warning, it wasn't
there before:

Mercury/asm_fast.gc.debug.stseg/x86_64-pc-linux-gnu/Mercury/cs/js_write_grammar_init.c:3380:13: warning: no previous prototype for ‘mercury_init_grade_check’ [-Wmissing-prototypes]

I'm using gcc 5.4.0 20160609 on x86_64.  The warning seems to be grade
independant.  It also benign, things otherwise work as expected.


-- 
Paul Bone


> 
> diff --git a/compiler/llds_out_file.m b/compiler/llds_out_file.m
> index f437ce4..e02410e 100644
> --- a/compiler/llds_out_file.m
> +++ b/compiler/llds_out_file.m
> @@ -482,6 +482,10 @@ output_c_module_init_list(Info, ModuleName, AnnotatedModules, RttiDatas,
>          io.write_string("required_final(void);\n", !IO)
>      ),
> 
> +    io.write_string("const char *", !IO),
> +    output_init_name(ModuleName, !IO),
> +    io.write_string("grade_check(void);\n", !IO),
> +
>      io.write_string("\n", !IO),
> 
>      io.write_string("void ", !IO),
> @@ -609,14 +613,19 @@ output_c_module_init_list(Info, ModuleName, AnnotatedModules, RttiDatas,
>          io.write_string("required_final(void)\n", !IO),
>          io.write_string("{\n", !IO),
>          output_required_init_or_final_calls(FinalPredNames, !IO),
> -        io.write_string("}\n", !IO)
> +        io.write_string("}\n", !IO),
> +        io.nl(!IO)
>      ),
> 
>      io.write_string(
> -        "/* ensure everything is compiled with the same grade */\n",
> +        "// Ensure everything is compiled with the same grade.\n",
>          !IO),
> -    io.write_string(
> -        "static const void *const MR_grade = &MR_GRADE_VAR;\n", !IO).
> +    io.write_string("const char *", !IO),
> +    output_init_name(ModuleName, !IO),
> +    io.write_string("grade_check(void)\n", !IO),
> +    io.write_string("{\n", !IO),
> +    io.write_string("    return &MR_GRADE_VAR;\n", !IO),
> +    io.write_string("}\n", !IO).
> 
>  :- pred module_defines_label_with_layout(llds_out_info::in,
>      annotated_c_module::in) is semidet.
> diff --git a/compiler/mlds_to_c.m b/compiler/mlds_to_c.m
> index eeb570c..9a9608d 100644
> --- a/compiler/mlds_to_c.m
> +++ b/compiler/mlds_to_c.m
> @@ -455,7 +455,7 @@ mlds_output_src_file(Opts, Indent, MLDS, !IO) :-
>      mlds_output_init_fn_defns(Opts, MLDS_ModuleName, FuncDefns,
>          TypeCtorInfoDefns, AllocSites, InitPreds, FinalPreds, !IO),
>      io.nl(!IO),
> -    mlds_output_grade_var(!IO),
> +    mlds_output_grade_check_fn_defn(MLDS_ModuleName, !IO),
>      io.nl(!IO),
>      mlds_output_src_end(Indent, ModuleName, !IO).
> 
> @@ -672,15 +672,16 @@ mlds_output_auto_gen_comment(Opts, ModuleName, !IO) :-
>      % file gets compiled with. This ensures that we don't try to link objects
>      % files compiled in different grades.
>      %
> -:- pred mlds_output_grade_var(io::di, io::uo) is det.
> +:- pred mlds_output_grade_check_fn_defn(mlds_module_name::in, io::di, io::uo) is det.
> 
> -mlds_output_grade_var(!IO) :-
> +mlds_output_grade_check_fn_defn(ModuleName, !IO) :-
>      io.write_string(
> -        "/* ensure everything is compiled with the same grade */\n",
> +        "// Ensure everything is compiled with the same grade.\n",
>          !IO),
> -    io.write_string(
> -        "static const void *const MR_grade = &MR_GRADE_VAR;\n",
> -        !IO).
> +    output_grade_check_fn_name(ModuleName, !IO),
> +    io.write_string("\n{\n", !IO),
> +    io.write_string("    return &MR_GRADE_VAR;\n", !IO),
> +    io.write_string("}\n", !IO).
> 
>      % Get the foreign code for C.
>      %
> @@ -729,7 +730,9 @@ mlds_output_init_fn_decls(ModuleName, InitPreds, FinalPreds, !IO) :-
>          FinalPreds = [_ | _],
>          output_required_fn_name(ModuleName, "required_final", !IO),
>          io.write_string(";\n", !IO)
> -    ).
> +    ),
> +    output_grade_check_fn_name(ModuleName, !IO),
> +    io.write_string(";\n", !IO).
> 
>  :- pred mlds_output_init_fn_defns(mlds_to_c_opts::in, mlds_module_name::in,
>      list(mlds_defn)::in, list(mlds_defn)::in,
> @@ -847,6 +850,20 @@ output_required_fn_name(ModuleName, Suffix, !IO) :-
>      io.write_string(Suffix, !IO),
>      io.write_string("(void)", !IO).
> 
> +:- pred output_grade_check_fn_name(mlds_module_name::in,
> +    io::di, io::uo) is det.
> +
> +output_grade_check_fn_name(ModuleName, !IO) :-
> +    ModuleNameString0 = sym_name_mangle(
> +        mlds_module_name_to_sym_name(ModuleName)),
> +    ( if string.prefix(ModuleNameString0, "mercury__") then
> +        ModuleNameString = ModuleNameString0
> +    else
> +        ModuleNameString = "mercury__" ++ ModuleNameString0
> +    ),
> +    io.format("const char *%s__grade_check(void)",
> +        [s(ModuleNameString)], !IO).
> +
>      % Generate calls to MR_init_entry() for the specified functions.
>      %
>  :- pred mlds_output_calls_to_init_entry(mlds_module_name::in,
> diff --git a/util/mkinit.c b/util/mkinit.c
> index 0e15a60..cbaa5b6 100644
> --- a/util/mkinit.c
> +++ b/util/mkinit.c
> @@ -619,9 +619,12 @@ static const char mercury_main_func[] =
>      "\n"
>      ;
> 
> -static const char mercury_grade_var[] =
> -    "/* ensure that everything gets compiled in the same grade */\n"
> -    "static const void *const MR_grade = &MR_GRADE_VAR;\n"
> +static const char mercury_grade_check_func[] =
> +    "// Ensure that everything gets compiled in the same grade.\n"
> +    "const char *mercury_init_grade_check(void)\n"
> +    "{\n"
> +    "    return &MR_GRADE_VAR;\n"
> +    "}\n"
>      "\n"
>      ;
> 
> @@ -1201,7 +1204,7 @@ output_main(void)
>          fputs(mercury_main_func, stdout);
>      }
> 
> -    fputs(mercury_grade_var, stdout);
> +    fputs(mercury_grade_check_func, stdout);
> 
>      if (output_main_func) {
>          fputs(main_func, stdout);
> _______________________________________________
> reviews mailing list
> reviews at lists.mercurylang.org
> https://lists.mercurylang.org/listinfo/reviews

-- 
Paul Bone