[Rd] Two bugs showing up mostly on SPARC systems

[Duncan Murdoch]

On 14/07/2015 9:29 PM, Radford Neal wrote:
> On Tue, Jul 14, 2015 at 07:52:56PM -0400, Duncan Murdoch wrote:
>> On 14/07/2015 6:08 PM, Radford Neal wrote:
>>> In testing pqR on Solaris SPARC systems, I have found two bugs that
>>> are also present in recent R Core versions.  You can see the bugs and
>>> fixes at the following URLs:
>>>
>>>   https://github.com/radfordneal/pqR/commit/739a4960a4d8f3a3b20cfc311518369576689f37
>>
>> Thanks for the report.  Just one followup on this one:
>>
>> There are two sections of code that are really similar.  Your patch
>> applies to the code in port_nlsb(), but there's a very similar test in
>> port_nlminb(), which is called from nlminb() in R.  Do you think it
>> would be a good idea to apply the same patch there as well?  It doesn't
>> look like it would hurt, but I don't know this code at all, so it might
>> be unnecessary.
> 
> Looking at nlminb, it seems that this bug doesn't exist there.  The 
> R code sets low <- upp <- NULL, as in nls, but later there is an 
> "else low <- upp <- numeric()" that ensures that low and upp are never
> actually NULL.  This may have been a fix for the bug showing up in
> nlminb that was not applied to nls as well (and of course, the fix
> didn't delete the now pointless low <- upp <- NULL).
> 
> The nlminb code might be a better fix, stylistically, after removing
> "low <- upp <- NULL", though it seems that both it and my fix for nls
> should work.  Of course, both assume that the call of the C function
> is done only from this R code, so no other types for low and upp are
> possible.  And really the whole thing ought to be rewritten, since the
> .Call functions modify variables without any regard for whether or not
> their values are shared.

Thanks.  I think I'll make the mods to the R code rather than the C
code, to make the two functions more consistent, and so as not to
suggest that port.c is actually okay.  The modification of variables
without checking for sharing is definitely something that should be
fixed.  It looks to me as though we're likely to get away with it here
(both m and iv are allocated by functions called by nls(), not by the
user), but it's still really bad practice.

Duncan Murdoch