Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix C and C++ bindings #125

Merged
merged 4 commits into from
Jun 9, 2018
Merged

Fix C and C++ bindings #125

merged 4 commits into from
Jun 9, 2018

Conversation

10110111
Copy link
Contributor

@10110111 10110111 commented Jun 7, 2018

These changes should be more correct than my previous attempt. I've fixed complex-related icba*.f90 files, and regenerated the C prototypes from all the icba*.f90 files. C++ header now simply includes the C one to avoid duplication of maintenance effort.

The C prototypes were generated by a quick-and-dirty perl script, which won't work in the fully general case, but should work for the well-behaved iso_c_binding code (namely, it relies on the order of parameters described inside the *_c functions being the same as in their prototypes). Here's the script, in case someone wants to regenerate the prototypes later (NOTE: for PARPACK you'll have to filter the output with something like sed 's@\<int comm,@MPI_Fint comm,@'):

#!/usr/bin/perl

use strict;
use warnings;
use Switch;

if(@ARGV!=0)
{
    print("Usage: pipe icba*.f90 files to stdin, and I'll print C prototypes for the functions therein\n");
    exit(0);
}

my $mode="funcSearch";
my $needComma=0;

while(<>)
{
    my $line=$_;
    switch($mode)
    {
        case "funcSearch"
        {
            if($line =~ /^[ \t]*subroutine[ \t]+([a-z_0-9]+)[ \t]*\(/)
            {
                my $funcName=$1;
                print("void $funcName(");
                while($line =~ /.*\&$/)
                {
                    $line .= <>;
                }
                $mode="argPrinting";
            }
        }
        case "argPrinting"
        {
            next if($line =~ /^[ \t]*use[ \t]*::[ \t]*iso_c_binding[ \t]*$/ || $line =~ /^[ \t]*implicit[ \t]+none[ \t]*$/);
            if($line =~ /^[ \t]*call[ \t]/)
            {
                print(");\n");
                $needComma=0;
                $mode="funcSearch";
                next;
            }

            print(", ") if($needComma);

            if($line =~ /[ \t]*(logical\(kind=c_bool\)|character\(kind=c_char\)|integer\(kind=c_int\)|(?:real|complex)\(kind=c_(?:float|double)(?:_complex)?\)),[ \t]*(?:(dimension\([^)]+\)|value),)?[ \t]*intent\((in|out|inout)\)[ \t]*::[ \t]*([a-z0-9_]+)[ \t]*$/)
            {
                my $type=$1;
                my $dim=$2 || "";
                my $intent=$3;
                my $parmName=$4;
                switch($type)
                {
                    case "logical(kind=c_bool)"
                    {
                        if($intent eq "in" && $dim eq "value")
                        {
                            print("bool")
                        }
                        else
                        {
                            print("int")
                        }
                    }
                    case "integer(kind=c_int)"              { print("int") }
                    case "character(kind=c_char)"           { print("char") }
                    case "real(kind=c_float)"               { print("float") }
                    case "complex(kind=c_float_complex)"    { print("float _Complex") }
                    case "real(kind=c_double)"              { print("double") }
                    case "complex(kind=c_double_complex)"   { print("double _Complex") }
                    else { die("Unrecognized type: $type") }
                }
                switch($dim)
                {
                    case "value"
                    {
                        # don't print anything
                    }
                    case /^dimension\(.+\)$/
                    {
                        print(" const") if($intent eq "in");
                        print("*");
                    }
                    case ""
                    {
                        print("*") if($intent eq "inout" || $intent eq "out")
                    }
                    else
                    {
                        die("Unrecognized parameter passing mode: $dim")
                    }
                }
                print(" $parmName");
                $needComma=1;
            }
            else
            {
                die("Failed to parse line: $line");
            }
        }
    }
}

This fixes #123.

10110111 added 2 commits June 7, 2018 13:17
Main problems found in the original version are:
I. C vs iso_c_binding:
    1. Mismatch between float complex and float parameters in C prototypes and
    icba*.f90 files
    2. Mismatch between passing of some arguments by pointer vs by value
II. iso_c_binding vs Fortran:
    1. Mismatch between real and complex parameters
    2. Wrong intent for 'workev'

These problems lead to UB, which, in particular, resulted in test
crashes on i686 targets. This patch fixes them.
This no longer includes a tweaked copy of arpack.h. Maintaining both
sets of prototypes separately is too much manual labor.
@coveralls
Copy link

coveralls commented Jun 7, 2018

Pull Request Test Coverage Report for Build 295

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 70.297%

Totals Coverage Status
Change from base Build 290: 0.0%
Covered Lines: 5022
Relevant Lines: 7144

💛 - Coveralls

double _Complex* workd, double _Complex* workl, int lworkl,
double _Complex* rwork, int& info);
}
#include "arpack.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Durr my bad - sorry! I should have done this from the start. Thanks!

@fghoussen
Copy link
Collaborator

fghoussen commented Jun 7, 2018

Sorry for the late reply.

Changes in icbazn.f90 and icbacn.f90 looks correct (my mystake, at the time, I had to do all from scratch). The same problem has been probably cut-pasted in icbpzn.f90 and icbpcn.f90.

Ideally, tests should use/test all cases/signatures... But there are a lot of them (I did a few).

@10110111
Copy link
Contributor Author

10110111 commented Jun 7, 2018

There are no tests for PARPACK C bindings, are there? Frankly, I've never dealt with PARPACK, so I'm not even sure how I can make a test for its C bindings. But indeed, the icbp*.f90 files do have that same real(kind=c_double_complex) problem, as well as mentioning of complex for tol, which is always real.

@10110111
Copy link
Contributor Author

10110111 commented Jun 7, 2018

OK, I've done the mechanical fixes for PARPACK C and C++ bindings too, but they are completely untested (or all tests pass if there exist any I don't know about).

@fghoussen
Copy link
Collaborator

fghoussen commented Jun 9, 2018

@10110111 : thanks ! Your patch looks OK to me : in icb[ap]*.f90, the bug is tol is a double/float, not a double/float complex. This can trigger a byte shift and crash (real(kind=c_float_complex) <=> complex(kind=c_float_complex) <=> OK). icb_parpack_c and icb_parpack_cpp are tests which use p[zc][ae]upd if I remember well.

@sylvestre sylvestre merged commit 449dc71 into opencollab:master Jun 9, 2018
@sylvestre
Copy link
Contributor

Do you think it fixes all issues?
thanks

@10110111
Copy link
Contributor Author

10110111 commented Jun 9, 2018

Well, all those I found. There might be another mismatches between icb*.f90 and *.f files, but finding them would require one to manually look through each iso_c_binding declaration comparing with the declarations in actual Fortran routines. I'm not prepared for such an amount of manual labor (and writing a parser of Fortran sources to automate this doesn't look like a faster task to do).

@fghoussen
Copy link
Collaborator

Do you think it fixes all issues?

I hope so !... Is it ok on i386 ?

There might be another mismatches... but finding them would require one to manually look...

100% agree: arpack users will open issues if they find problems (easy to fix a posteriori and add test - crazy job to check a priori). Ideally, tests should test all signatures (= heavy job for a walking-dead-fortran thing like arpack...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

icb_arpack_cpp: testsuite fails on i386
5 participants