Skip to content

Need help on calling Head from C

8 messages · Sandip Nandi, Hervé Pagès, Radford Neal

Original
Radford Neal
# 
The code is actually not safe.  Both "install" and "SalarLogical/Integer"
potentially allocate memory, so at least one needs to be protected before
callling lang3.  (Passing one such argument would be OK, since lang3 
protects its arguments, but it doesn't get a chance to do that while they
are still being evaluated.)

Now, I'm not sure this is the source of the actual problem, since both
"data.frame" and "head" presumably already exist, so the install won't
actually allocate memory.  But this is not a safe coding method in 
general.

   Radford Neal
Radford Neal
# 
Yes, either the result of Scalar Integer(1) or the result of install
should be protected (before calling lang3 - don't try to do it inside
the argument list of lang3).
Hervé Pagès
# 
Hi,
On 06/26/2014 01:36 PM, Radford Neal wrote:
How true but who can blame him? The Writing R Extensions manual
contains the same mistake:

   SEXP mkans(double x)
   {
     SEXP ans;
     ans = PROTECT(allocVector(REALSXP, 1));
     REAL(ans)[0] = x;
     UNPROTECT(1);
     return ans;
   }

   double feval(double x, SEXP f, SEXP rho)
   {
     defineVar(install("x"), mkans(x), rho);
     return REAL(eval(f, rho))[0];
   }

Reported here more than 6 years ago and never fixed:

   https://stat.ethz.ch/pipermail/r-devel/2008-January/048040.html

Cheers,
H.

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      Radford Neal
    

    

      
      #
    

  


  

          

      
      
      
    

        
A further comment on this...  Currently, symbols are never garbage
collected, so you might think the above is OK, since the result
of install("x") can't be lost wen mkans(x) is called.  However, I
think it's not a good idea to rely on symbols never being collected.
In any case, though, even if you are relying on that, the code isn't
safe because C doesn't specify the order of evaluation of argments,
so mkans(x) might be called before install("x").

One should also note that the PROTECT within mkans is unnecessary,
and must surely be confusing to anyone who thought (correctly) 
that they had understood what PROTECT is for.

   Radford Neal

  
  


  


      

    

    

      
      

        


  

        

      Hervé Pagès
    

    

      
      #
    

  


  

      
On 06/27/2014 02:23 AM, Radford Neal wrote:

            

      
      
      
    

        
I understand what PROTECT is for and I don't find the PROTECT in mkans
confusing.

Maybe it's not necessary now. But it's so much simpler/safer to just
systematically protect any freshly allocated SEXP. One day
someone might need to modify mkans(), say, add a call to warning() or
Rprintf() after the call to allocVector(), and will most likely forget
to add the PROTECT/UNPROTECT that will then become necessary. So at
least there should be a comment next to the allocVector() call in
mkans() to explain why in this particular case the returned value
doesn't need to be protected (but writing the comment probably takes
as much time as doing the PROTECT/UNPROTECT anyway).

Even now, why should I rely on the fact that the REAL() macro doesn't
allocate memory and will never do so?

So with systematic protection:

      SEXP mkans(double x)
      {
          SEXP ans;
          PROTECT(ans = allocVector(REALSXP, 1));
          REAL(ans)[0] = x;
          UNPROTECT(1);
          return ans;
      }

      double feval(double x, SEXP f, SEXP rho)
      {
          SEXP symbol, value;
          PROTECT(symbol = install("x"));
          PROTECT(value = mkans(x));
          defineVar(symbol, value, rho);
          UNPROTECT(2);
          return(REAL(eval(f, rho))[0]);
      }

I doubt you'll ever be able to detect any difference in terms of speed
or memory usage because the cost of PROTECT/UNPROTECT is virtually 0.
Furthermore, one could imagine a static code checker would be able to
verify this and warn me about missing PROTECTs.

BTW I appreciate the work you're doing on pqR and hopefully more of the
stuff you've done will get merged into R at some point.

Cheers,
H.

            

      
      
      
    

        

  
    

      
      
    

  


  


      

    

    

      
      

        


  

        

      Radford Neal
    

    

      
      #
    

  


  

          

      
      
      
    

        
There's certainly something to be said in favour of the "better safe
than sorry" approach to using PROTECT.  But in the context of a tutorial,
one shouldn't put one in that's unnecessary without a comment saying,
"not needed at the moment, but would be if code that allocates more
memory is added later, so let's be safe and do it now too".  Otherwise,
the reader may infer an incorrect model of when to PROTECT, such as 
"you should PROTECT every allocated object, then UNPROTECT at the end of
the procedure - do that and you'll be OK".  Of course, that's not
enough to be OK.  And to illustrate the correct model, one needs some
negative examples as well as positive examples.

   Radford Neal