void pointer

[Jeffrey Ratcliffe]

I'm writing some Perl bindings for SANE. Having got things working,
I'm trying to fix a couple of memory leaks.

valgrind is complaining:

==7353== Invalid read of size 1
==7353== at 0x40239E3: strlen (mc_replace_strmem.c:242)
==7353== by 0x80D0710: Perl_newSVpv (in /usr/bin/perl)
==7353== by 0x472152F: XS_SANE_open (SANE.xs:126)
==7353== by 0x80C22D2: Perl_pp_entersub (in /usr/bin/perl)
==7353== by 0x80C0CAA: Perl_runops_standard (in /usr/bin/perl)
==7353== by 0x806727A: perl_run (in /usr/bin/perl)
==7353== by 0x8063791: main (in /usr/bin/perl)
==7353== Address 0x49840b8 is 0 bytes after a block of size 8 alloc'd
==7353== at 0x4021BDE: calloc (vg_replace_malloc.c:397)
==7353== by 0x473CAF2: sane_dll_open (in /usr/lib/libsane.so.1.0.19)
==7353== by 0x473D193: sane_open (in /usr/lib/libsane.so.1.0.19)
==7353== by 0x47213D9: XS_SANE_open (SANE.xs:124)
==7353== by 0x80C22D2: Perl_pp_entersub (in /usr/bin/perl)
==7353== by 0x80C0CAA: Perl_runops_standard (in /usr/bin/perl)
==7353== by 0x806727A: perl_run (in /usr/bin/perl)
==7353== by 0x8063791: main (in /usr/bin/perl)

void
sane_open(class, name)
SANE_String_Const name
INIT:
SANE_Status status;
SANE_Handle h;
PPCODE:
status = sane_open(name, &h);
XPUSHs(sv_2mortal(newSViv(status)));
126-> XPUSHs(sv_2mortal(newSVpv(h, 0)));

SANE_Handle is a void pointer:

http://www.sane-project.org/html/doc011.html#s4.2.6: Access to a
scanner is provided through an opaque type called SANE_Handle. The C
declaration of this type is given below.

typedef void *SANE_Handle;

While this type is declared to be a void pointer, an application must
not attempt to interpret the value of a SANE_Handle. In particular,
SANE does not require that a value of this type is a legal pointer
value.

So in the typemap I have:

SANE_Handle T_OPAQUEPTR

I assume that newSVpv(h, 0) is causing me the trouble. I can't work
out what is correct.

Any help would be appreciated

Jeff

[Nicholas Clark]

Post by Jeffrey Ratcliffe
http://www.sane-project.org/html/doc011.html#s4.2.6: Access to a
scanner is provided through an opaque type called SANE_Handle. The C
declaration of this type is given below.
typedef void *SANE_Handle;
While this type is declared to be a void pointer, an application must
not attempt to interpret the value of a SANE_Handle. In particular,
SANE does not require that a value of this type is a legal pointer
value.
SANE_Handle T_OPAQUEPTR
I assume that newSVpv(h, 0) is causing me the trouble. I can't work
out what is correct.

newSVpv() is documented as:

Creates a new SV and copies a string into it. The reference count for the
SV is set to 1. If C<len> is zero, Perl will compute the length using
strlen(). For efficiency, consider using C<newSVpvn> instead.

SV* newSVpv(const char *const s, const STRLEN len)

So yes, it's doing a strlen() because you specify the length as 0. To do what
you wanted to do, you wanted newSVpvn():

Creates a new SV and copies a string into it. The reference count for the
SV is set to 1. Note that if C<len> is zero, Perl will create a zero length
string. You are responsible for ensuring that the source string is at least
C<len> bytes long. If the C<s> argument is NULL the new SV will be undefined.

SV* newSVpvn(const char *const s, const STRLEN len)



But I'm not even sure that that's sensible. Your typemap is

Post by Jeffrey Ratcliffe
void
sane_open(class, name)
SANE_String_Const name
SANE_Status status;
SANE_Handle h;
status = sane_open(name, &h);
XPUSHs(sv_2mortal(newSViv(status)));
126-> XPUSHs(sv_2mortal(newSVpv(h, 0)));

So what you're doing is a moderately expensive way of creating an empty string.
You're not storing the value of h itself.

Assuming that you wanted to store the value of h, I don't know what the best
thing to do is, and I have to stop typing now as food is served.

Nicholas Clark

[Jan Dubois]

Post by Jeffrey Ratcliffe
void
sane_open(class, name)
SANE_String_Const name
SANE_Status status;
SANE_Handle h;
status = sane_open(name, &h);
XPUSHs(sv_2mortal(newSViv(status)));
126-> XPUSHs(sv_2mortal(newSVpv(h, 0)));

You probably want to use

XPUSHs(sv_2mortal(newSViv(PTR2IV(h))));

[...]

Post by Jeffrey Ratcliffe
SANE_Handle T_OPAQUEPTR

I think you want just T_PTR instead. Or you can explicitly
restore the pointer from the IV as e.g.:

h = INT2PTR(SANE_Handle, ST(0));

The T_OPAQUEPTR type would pack your pointer into a string, which is
less efficient than just converting it to an IV and back.

It is somewhat confusing that you are using the typemap for your INPUT
parameter but then try to encode the OUTPUT parameter yourself. You
should consistently do one or the other and not mix things (just for the
sake of maintaining the code later).

Cheers,
-Jan

[Torsten Schoenfeld]

Post by Jan Dubois

Post by Jeffrey Ratcliffe
SANE_Handle T_OPAQUEPTR

If you want the handle to be represented by a blessed scalar reference, you can
use T_PTROBJ. Or, if the package name chosen by T_OPAQUEPTR doesn't suit you,
define your own typemap and use sv_setref_pv yourself. See the implementation
of T_PTROBJ in $INC/ExtUtils/typemap for an example.

Post by Jan Dubois
It is somewhat confusing that you are using the typemap for your INPUT
parameter but then try to encode the OUTPUT parameter yourself. You
should consistently do one or the other and not mix things (just for the
sake of maintaining the code later).

I don't see a way to avoid this if you use PPCODE sections to return more than
one value.

-Torsten

[Jan Dubois]

Post by Torsten Schoenfeld

Post by Jan Dubois
It is somewhat confusing that you are using the typemap for your INPUT
parameter but then try to encode the OUTPUT parameter yourself. You
should consistently do one or the other and not mix things (just for the
sake of maintaining the code later).

I don't see a way to avoid this if you use PPCODE sections to return more than
one value.

Yes, so use CODE sections with multiple OUTPUT variables. You can still
explicitly adjust the stackpointer an XSRETURN_EMPTY in an error branch
if you don't want to return your output variables on error. The OUTPUT
handling is only executed when you fall off the end of your XS code.

Cheers,
-Jan

[Torsten Schoenfeld]

[Any reason you removed the CC to the perl-xs list?]

Oops, no, that was accidental. Adding it back, and quoting your reply below for
the benefit of the public.

Post by Jan Dubois

Post by Torsten Schoenfeld
I don't see a way to avoid this if you use PPCODE sections to return more than
one value.

Yes, so use CODE sections with multiple OUTPUT variables. You can still
explicitly adjust the stackpointer an XSRETURN_EMPTY in an error branch
if you don't want to return your output variables on error. The OUTPUT
handling is only executed when you fall off the end of your XS code.

Interesting. I didn't know that OUTPUT could handle multiple return values.

Actually, I think I was wrong: you cannot have multiple return values with the
OUTPUT section, you can only designate additional values using parameters that
have been passed in with this.
But at least in newer versions of xsubpp you can use the OUTLIST modifier
on arguments to add the to the output list (check perlxs.pod in newer Perl
versions).

Yeah, but I've seen strange bugs with OUTLIST. I don't remember the details,
but I think the bugs only occurred when ExtUtils::ParseXS' xsubpp was in use.
The symptom was a spurious "Modification of a read-only value attempted" warning.

What if there is a variable number of return values? Imagine, for example, a
void
foo (int *n_return_values, double **return_values)
int n_return_values = 0, i;
double *return_values = NULL;
foo (&n_return_values, &return_values);
EXTEND (SP, n_return_values);
for (i = 0; i < n_return_values; i++) {
PUSHs (sv_2mortal (newSVnv (return_values[i])));
}
Is there a way to let typemaps do the work in this case?

I don't see a way to get this working using typemaps, sorry.

Over in the gtk2-perl camp, we're hoping to mostly automate the creation of
bindings with gobject-introspection anyway, so no bid deal. :-)

-Torsten

[Jeffrey Ratcliffe]

Post by Jan Dubois
You probably want to use
XPUSHs(sv_2mortal(newSViv(PTR2IV(h))));
[...]

SANE_Handle T_OPAQUEPTR

I think you want just T_PTR instead. Or you can explicitly

Excellent, thank you. That seems to have done the trick.

Regards

Jeff