[m-rev.] for review: --warn-suspicious-foreign-procs

[Julien Fischer]

On Mon, 7 Sep 2009, Peter Wang wrote:

> On 2009-09-05, Julien Fischer <juliensf at csse.unimelb.edu.au> wrote:
>>
>> Add a new warning, --warn-suspicious-foreign-procs.
>> This checks the bodies of foreign_proc pragmas for possible errors.
>> Currently, these errors are:
>>
>> (1) the presence of SUCCESS_INDICATOR (or in Java, succeeded),
>> in foreign procs. for predicates that cannot fail.
>>
>> (2) the lack of SUCCESS_INDICATOR (succeeded) in foreign procs.
>> for predicates that can fail.
>>
>> (3) the presence of "return" (or "ret" or "jmp" in IL).  This could
>> indicate the presence of a return statement in the foreign proc.
>>
>> Potentially, we could also check for the presence of the this
>> pointer in Java and C# foreign_procs, but that isn't implemented
>> at the moment.
>>
>> The warning is disabled by default, since it will produce spurious
>> results if the things it checks for occur in foreign language
>> comments.
>
> In the case of (3) this could occur quite often which might limit
> uptake of the option.  Which would be a shame as the success indicator
> checks should be safe and useful to enable by default.

We can see about doing that after we have had a chance to try it out
for a bit.

>> Index: compiler/make_hlds_warn.m
>> ===================================================================
>> RCS file: /home/mercury/mercury1/repository/mercury/compiler/make_hlds_warn.m,v
>> retrieving revision 1.32
>> diff -u -r1.32 make_hlds_warn.m
>> --- compiler/make_hlds_warn.m	2 Sep 2009 00:30:16 -0000	1.32
>> +++ compiler/make_hlds_warn.m	4 Sep 2009 14:31:22 -0000
>
>> @@ -608,9 +611,178 @@
>>      varset.search_name(VarSet, Var, Name),
>>      string.prefix(Name, "_").
>>
>> +:- pred pragma_foreign_proc_body_checks(foreign_language::in,
>> +    prog_context::in, simple_call_id::in, pred_id::in, proc_id::in,
>> +    module_info::in, list(string)::in,
>> +    list(error_spec)::in, list(error_spec)::out) is det.
>> +
>> +pragma_foreign_proc_body_checks(Lang, Context, PredOrFuncCallId, PredId, ProcId,
>> +        ModuleInfo, BodyPieces, !Specs) :-
>> +    check_fp_body_for_success_indicator(Lang, Context, PredOrFuncCallId,
>> +        PredId, ProcId, ModuleInfo, BodyPieces, !Specs),
>> +    check_fp_body_for_return(Lang, Context, PredOrFuncCallId, BodyPieces,
>> +        !Specs).
>
> Wouldn't these be better as a single pass? (not necessarily in this commit)

You mean merge the check_fp_body_for_* predicates?  I tried that
initially - the resulting code was rather more complicated than I would
have liked.

>> +
>> +    % Check to see if a foreign_proc body contains a return statment
>
> statement

Fixed.

>
>> Index: doc/user_guide.texi
>> ===================================================================
>> RCS file: /home/mercury/mercury1/repository/mercury/doc/user_guide.texi,v
>> retrieving revision 1.592
>> diff -u -r1.592 user_guide.texi
>> --- doc/user_guide.texi	3 Sep 2009 23:57:44 -0000	1.592
>> +++ doc/user_guide.texi	4 Sep 2009 15:01:07 -0000
>> @@ -6313,6 +6313,11 @@
>>  @findex --no-warn-unresolved-polymorphism
>>  Do not warn about unresolved polymorphism.
>>
>> + at sp 1
>> + at item --warn-suspicious-foreign-procs
>> + at findex --warn-suspicious-foreign-procs
>> +Warn about possible errors in the bodies of foreign procedures.
>> +
>
> This should describe the checks as currently implemented,
> or at least the limitations of the implementation.

Here is a revised entry for the users's guide:

Index: user_guide.texi
===================================================================
RCS file: /home/mercury/mercury1/repository/mercury/doc/user_guide.texi,v
retrieving revision 1.592
diff -u -r1.592 user_guide.texi
--- user_guide.texi	3 Sep 2009 23:57:44 -0000	1.592
+++ user_guide.texi	7 Sep 2009 08:36:02 -0000
@@ -6313,6 +6313,17 @@
  @findex --no-warn-unresolved-polymorphism
  Do not warn about unresolved polymorphism.

+ at sp 1
+ at item --warn-suspicious-foreign-procs
+ at findex --warn-suspicious-foreign-procs
+Warn about possible errors in the bodies of foreign procedures.
+When enabled, the compiler attempts to determine whether the success
+indicator for a foreign procedure is correctly set, and whether
+the foreign procedure body contains operations that are not allowed
+(for example, @code{return} statements in a C foreign procedure).
+Note that since the compiler's ability to parse foreign language code
+is limited some warnings reported by this option may be spurious and
+some actual errors may not be detected at all.
  @end table

  @node Verbosity options

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------