Re^2: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory

[edan]

Thanks for the reply demerphq. But I think you've confused me more.

I thought my mortalized SVs were only supposed to be blown away when the following code gets executed:

    FREETMPS;
    LEAVE;

So how could the values be freed too soon?

Also, if what you say is true, then I'd think it would hold true for storing values in a hash, too. But I experimented, and it seems like there, I have to mortalize the SVs I am storing in the hash, or there's a leak:

    HV *hash = (HV *)sv_2mortal((SV *) newHV());
    SV *type = newSVpv("string", 0);
    //SV *type = sv_2mortal(newSVpv("string", 0));
    SvREFCNT_inc( type );
    if ( NULL == hv_store( hash, "type", strlen("type"), type, 0) ) {
        SvREFCNT_dec( type );
        printf("hv_store failed\n"); exit(1);
    }
    XPUSHs(sv_2mortal(newRV((SV *)hash)));

That leaks, but if I mortalize the SV as in the comment, everything is great, and there are no warnings. So what gives? What's the difference between the av_push and hv_store examples?

Thanks

[demerphq]

Update: Looks like this isnt right... Sigh

I thought my mortalized SVs were only supposed to be blown away when the following code gets executed:

Yes, the logic i explained is basically what happens when FREETMPS/LEAVE occurs. Dont take my description too literally, im far from being authority on the internals, just trying to add some insight based on the little understanding i do have.

So how could the values be freed too soon?

Im assuming the reason the vars could be blown away too soon is that when you mortalize a var it goes on a temps stack, which is then processed by the FREETMPS/LEAVE macros. If you mortalize a member element of a composite structure which is itself mortalized then presumably there will be two attempts to refcount-dec/free the same thing.

That leaks, but if I mortalize the SV as in the comment, everything is great, and there are no warnings. So what gives? What's the difference between the av_push and hv_store examples?

Im basically guessing, but id think the problem is that when you do newSVpv() the created SVPV has a refcount of 1. You then increment the refcount, bringing it to 2, (note that in the array example you do not do the additional SvREFCOUNT_inc()) then you hv_push into a mortalized hash, which is then referenced by a mortal SVRV. So the mortal SVPV gets its refcount decremented once by its own mortalization, and then decremented again when the hash is freed. If you dont mortalize the SVPV then its refcount is 1 and it leaks. However it seems to me that the proper thing to do here is to do something like:

    HV *hash = newHV();
    SV *type = newSVpv("string", 0);
    if ( NULL == hv_store( hash, "type", 
          strlen("type"), type, 0) ) 
    {
        SvREFCNT_dec( type ); /* free the SV */
        printf("hv_store failed\n"); 
        exit(1); /*me wonders about this...*/
    }
    XPUSHs(sv_2mortal(newRV((SV *)hash)));

The idea being that the only thing that needs to be mortal is the newRV(), which when its freed will cascade through the things it references, refcount decrementing them to 0 and thus causing them to be freed.

Anyway, its very possible ive gotten something horribly wrong, but this is more or less as i understand it.

[edan]

Tried that. Leaks like a madman. Nope, I think the HV and the SVpv need to be mortal. I'll stick with my current incantation, since empirically it's correct, even though I still don't know why, and am still plagued by the thought that maybe it only seems right, but isn't.

Any more help is most welcome.

[tye]

Possible "mistakes":

1. Ref count set too high: Leaks memory
2. Ref count set too low: Throws up, hopefully not too cryptically
3. Ref count set exactly 1 "too high" and mortalized when it doesn't need to be: Nothing goes wrong

So you've probably got a case of (3) now. You usually don't have to make mortal something that you aren't putting directly onto the Perl stack, but sometimes it is convient to do so.

For example, you could use newRV_noinc() since you are making a reference to an AV that you just created (and thus that already has a reference count of 1). Then you wouldn't need to make the AV mortal. Or, you could decide that you like the slightly less efficient method of immediately making it mortal such that it will get freed even if something goes wrong with (or before) your call to newRV().

I'd try to avoid calling SvREFCNT_inc() / SvREFCNT_dec(), there's usually a clearer way (that is less likely to be done incorrectly). You should call croak() not printf() and exit().

Why not use call_pv() instead of call_sv() and making yet another thing that you have to make mortal?

So your original problem was two mistakes. First, av_push() doesn't increment the refcount so creating a new SV and av_push()ing it onto one array is fine (refcount starts out as and stays 1). But av_push()ing it onto two arrays would be bad (refcount still 1 when needs to be 2). Mortalizing the SV means the refcount drops to 0 later so it tries to get free()d twice.

Second, you use newRV() [which is newRV_inc()] so the AV's ref count was too high. So you had one ref-count too high and a different ref count too low. So your two choices were leaking an AV such that the AV didn't try to free up the SV a second time or properly destroy the AV such that it noticed that the SV had too low of a ref count.

[demerphq]

Tried that. Leaks like a madman.

Well, thanks for letting me know. I guess i need to book up on this stuff a little better :-)

I think at this point if some internals guru hasnt popped up to explain things (i havent checked as of writing this) that you should ask on p5p.

Sorry I wasnt more helpful. :-(

[demerphq]

Hmm, i did a little research into this, and i think i know what the problem was with my code. newRV is an alias for newRV_inc(), which means the item being referenced has an implicit SvREFCNT_inc() called on it, thus in the snippet i showed *hash ends up with a refcount of 2 instead of a refcount of 1. Changing it to newRV_noinc() should fix it.

    HV *hash = newHV();
    SV *type = newSVpv("string", 0);
    if ( NULL == hv_store( hash, "type", 
          strlen("type"), type, 0) ) 
    {
        SvREFCNT_dec( type ); /* free the SV */
        printf("hv_store failed\n"); 
        exit(1); /*me wonders about this...*/
    }
    XPUSHs(sv_2mortal(newRV_noinc((SV *)hash)));

I really have to get myself set up to check this, but I thought id mention it. BTW, if you havent already read it you should review "Reference Counts and Mortality" in perlguts. Which on a quick reading basically agrees with what i said before, with the minor caveat about newRV_noinc(), which it specifically documents as being a common mistake.

For example, imagine you want to return a reference from an XSUB function. Inside the XSUB routine, you create an SV which initially has a reference count of one. Then you call newRV_inc, passing it the just-created SV. This returns the reference as a new SV, but the reference count of the SV you passed to newRV_inc has been incremented to two. Now you return the reference from the XSUB routine and forget about the SV. But Perl hasn't! Whenever the returned reference is destroyed, the reference count of the original SV is decreased to one and nothing happens. The SV will hang around without any way to access it until Perl itself terminates. This is a memory leak.

Which basically explains why my original snippet "leaks like mad". It leaks like mad because the root of the structure that you want to disappear on its own never goes to 0, so the whole lot stays in memory. Change the newRV() to newRV_noinc() and it should be freed as and when expected and no leaks.