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

Hmm.... thanks for pressing this, demerphq. You appear to be right about not needing so many mortals. I think the trick is to avoid incrementing any refcounts (as I was doing after hv_store()), and only mortalize the RV I am pushing on the stack. This is basically what you said, but the code comes out a bit different from what you said:

    AV *arr = newAV();
    av_push( arr, newSVpv("test", 0));
    XPUSHs(sv_2mortal(newRV_noinc((SV *)arr)));

    HV *hash = newHV();
    hv_store( hash, "key", strlen("key"), newSVpv("test", 0), 0);
    XPUSHs(sv_2mortal(newRV_noinc((SV *)hash)));

[download]

So I'm just creating all "intermediate" variables non-mortals, not incrementing any refcounts after av_push or hv_store(), using newRV_noinc(), and mortalizing just the RV that's pushed on the stack.

Does that seem right? Sure looks cleaner...

--
edan

  • Comment on Re^6: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory
  • Download Code

Replies are listed 'Best First'.
Re^7: perlembed: mortalize an AV, get "Attempt to free unreferenced scalar" - don't mortalize, leaks memory
by demerphq (Chancellor) on Jul 11, 2006 at 11:43 UTC

    Hmm.... thanks for pressing this

    No problem. I got it wrong first off, which I felt bad about, and ultimately your quest to educate yourself ended up educating me, which IMO is a victory for us both, so thanks right back atchya :-)

    Anyway, the posted code looks lot closer to what I would expect. The only glaring issue is basically a nit, in that you need to handle the case of the hv_store() failing. Which is why its normally done like 

    HV *hash = newHV();
SV *sv= newSVpv("test", 0);
if (!hv_store( hash, "key", strlen("key"), sv , 0)) {
  /* need to explicitly free the sv if its 
   * not successfully stored in the hash
   */
  SvREFCNT_dec(sv); 
}

    [download]

    As otherwise if the hv_store fails you will leak....

    I guess that this is just sample code so I should overlook the the overuse of strlen but in that sample I'd supply sizes explicitly. Also, be careful, strlen() is unsafe in the core (especially as regards SV's) as strings are allowed to contain embedded nulls.

    ---
    $world=~s/war/peace/g

[reply]
[d/l]
      Are you sure about that? Seems to me from perlapi that the caller is responsible for both incrementing and decrementing the stored SV. But since I'm not incrementing, why should I decrement upon failure? I took the example of a bare hv_store() with neither increment nor decrement from DBI.xs, as you suggested...

      --
      edan

[reply]

        You dont need to increment it as its at refcount 1 when its created, and isn't shared amongst multiple structures. But you do need to decrement if hv_store() fails as otherwise it won't be freed. And if DBI isnt doing that then I suspect its being a little naughty. :-)

        AFAIK its really unlikely that hv_store fails so its probably ok. I just like to cover all my bases, especially as when im working on this stuff im usually patching core and somebody like hv or dave_the_m or Rafael or somebody like that will call me on it if I dont. :-)

        ---
        $world=~s/war/peace/g

[reply]

In Section Seekers of Perl Wisdom