Re: compilation warnings in liboctave/CMatrix.cc

[Jaroslav Hajek]

On Thu, Jan 13, 2011 at 9:44 PM, John W. Eaton <address@hidden> wrote:
> On 13-Jan-2011, CdeMills wrote:
>
> | Any idea why this message is produced ?
>
> Part of the answer is that the macro used to call Fortran code inserts
> a call to setjmp for error handling, but I don't know precisely the
> rules that GCC uses to decide when the problem can occur.  When a
> variable is in a register?
>
> jwe
>

I'm not sure how exactly registers come into it, but the basic problem
is that any modification to a stack local variable in the caller
context may be restored by longjmp. So the problem really only is if
the parameters to F77_XFCN introduce a side effect, such as a
non-const method call. Apparently, this can happen even by some
optimization by GCC aka delayed update; I'd say GCC should be smart
enough to discard such an optimization in the critical section but
maybe it always aren't; at least it warns us about it.

A bigger part of the problem is that calling Array::fortran_vec()
inside F77_XFCN (which we do extremely often) is unsafe in theory,
because that function must be non-const by its very definition. In
practice, we often do that with unshared arrays, where we know that
not to occur; often, but not always. So yes, some calls can get us in
trouble. I now see that some of those in dbleLU belong to that area
:(.

What should we do? Good question. I don't think that simply avoiding
fortran_vec() is a good solution; that would be far too inconvenient.
IMHO, it would be inherently much safer if we simply didn't embed the
setjmp/longjmp handling code into the surrounding scope (which is what
F77_XFCN does now) but created a wrapper function for it. That is the
recommended way to do things, because you avoid these issues nicely
and cleanly. It would allow us to get rid of all those stupid ad hoc
volatile declarations and nicely and cleanly solve all these issues.

The challenge is how to do that with macros. We'd probably need to
provide slightly more boilerplate code in the function declarations,
because the wrapper would need to pass all of its args into the F77
function, and C macros aren't probably powerful enough to transform
the declaration list into such code.

So, my proposal is to create a pair of macros, F77_WRAP_FCN and
F77_WRAP_XFCN. Both of them will declare the corresponding F77_FCN in
a separate extern"C" block, and then provide a wrapper, called
f77_wrapper_<function_name>.
Then, we'll change declarations like:

F77_RET_T
F77_FUNC (dgetrf, DGETRF) (const octave_idx_type&, const
octave_idx_type&, double*,
                       const octave_idx_type&, octave_idx_type*,
octave_idx_type&);

to

F77_WRAP_FUNC (dgetrf, DGETRF, (const octave_idx_type& a1, const
octave_idx_type& a2, double* a3,
                                const octave_idx_type& a4,
octave_idx_type* a5, octave_idx_type& a6), (a1, a2, a3, a4, a5, a6));

which will expand to:

extern "C"
F77_RET_T
F77_FUNC (dgetrf, DGETRF) (const octave_idx_type& a1, const
octave_idx_type& a2, double* a3,
                                const octave_idx_type& a4,
octave_idx_type* a5, octave_idx_type& a6);

void
f77_wrapper_fcn (const octave_idx_type& a1, const octave_idx_type& a2,
double* a3,
                                const octave_idx_type& a4,
octave_idx_type* a5, octave_idx_type& a6) {
F77_FUNC (dgetrf, DGETRF) (a1, a2, a3, a4, a5, a6);
}

Similarly, F77_WRAP_XFCN will expand to a wrapper with the
setjmp/longjmp handler. The price is the additional boilerplate (a1,
a2, a3, a4, a5, a6) arglist in the wrapper definition, but I think
that is easily justifiable.
FWIW, all of this should be easily doable by a conversion script.
Instead of a1, a2, etc. I would use named arguments if they're
provided in the declarations, otherwise I would generate them along
with a TODO message.

Comments? I'm volunteering to do the new macros and the automatic
conversion part. I'll then leave it to others so hunt down and get rid
of redundant volatile declarations.
Unfortunately, for some reason Savannah doesn't want to recognize new
SSH keys for me, which slightly hinders my ability to contribute now
:( I have to look into it.

best regards