[PATCH] Fix missing break

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix missing break

Steve Grubb
Hello,

There appears to be a break missing in the switch/case for the LISTSXP case.
If this is supposed to fall through, I'd suggest a comment so that others
know its by design.

Signed-off-by: Steve Grubb <[hidden email]>

Index: src/main/builtin.c
===================================================================
--- src/main/builtin.c (revision 72935)
+++ src/main/builtin.c (working copy)
@@ -888,6 +888,7 @@
     SETCAR(t, CAR(x));
     SET_TAG(t, TAG(x));
  }
+ break;
     case VECSXP:
  for (i = 0; i < len; i++)
     if (i < lenx) {

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix missing break

Duncan Murdoch-2
Thanks for posting this series of patches.  Unfortunately, there's a
good chance they'll get lost in all the traffic on R-devel.  If you
don't hear that they've been fixed in the next couple of weeks, could
you post them to bugs.r-project.org, and post future patches there as well?

In examples like the one below, if you have R code that shows symptoms,
it would really help in the bug report.  Otherwise, someone else will
have to analyze the code to decide whether it's a bug or missing
comment.  That takes time, and if there are no known symptoms, it's
likely to be assigned a low priority.  The sad truth is that very few
members of R Core are currently actively fixing bugs.

Duncan Murdoch



On 20/07/2017 5:02 PM, Steve Grubb wrote:

> Hello,
>
> There appears to be a break missing in the switch/case for the LISTSXP case.
> If this is supposed to fall through, I'd suggest a comment so that others
> know its by design.
>
> Signed-off-by: Steve Grubb <[hidden email]>
>
> Index: src/main/builtin.c
> ===================================================================
> --- src/main/builtin.c (revision 72935)
> +++ src/main/builtin.c (working copy)
> @@ -888,6 +888,7 @@
>      SETCAR(t, CAR(x));
>      SET_TAG(t, TAG(x));
>   }
> + break;
>      case VECSXP:
>   for (i = 0; i < len; i++)
>      if (i < lenx) {
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix missing break

Steve Grubb
On Thursday, July 20, 2017 7:41:00 PM EDT Duncan Murdoch wrote:
> Thanks for posting this series of patches.  Unfortunately, there's a
> good chance they'll get lost in all the traffic on R-devel.  If you
> don't hear that they've been fixed in the next couple of weeks, could
> you post them to bugs.r-project.org, and post future patches there as well?

That was my first inclination. But there is no way to create an account unlike
most open source projects I work with. And I work with quite a lot.


> In examples like the one below, if you have R code that shows symptoms,
> it would really help in the bug report.

I am hoping that we can look at the code as seasoned programmers and say yeah,
that is a bug. I run the code through Coverity and have quite a lot of
problems to tell you about. I run these 5 out as tests to see how this
community works. I am new to this community but not necessarily R and just
want to contribute back to something I am using. But believe me, I have a
bunch more that seasoned programmers can eyeball and say yep - that's a bug.


> Otherwise, someone else will have to analyze the code to decide whether it's
> a bug or missing comment.  That takes time, and if there are no known
> symptoms, it's likely to be assigned a low priority.  The sad truth is that
> very few members of R Core are currently actively fixing bugs.

That's a shame. I'd be happy to give the scan to people in core so they can
see what the lay of the land looks like. R works amazingly good. So much so I
decided to dig deeper. I'd recommend to the core developers that they ask to
get on Coverity's open source scan list.

https://scan.coverity.com/

It's free to open source projects like this. :-)

-Steve


> On 20/07/2017 5:02 PM, Steve Grubb wrote:
> > Hello,
> >
> > There appears to be a break missing in the switch/case for the LISTSXP
> > case. If this is supposed to fall through, I'd suggest a comment so that
> > others know its by design.
> >
> > Signed-off-by: Steve Grubb <[hidden email]>
> >
> > Index: src/main/builtin.c
> > ===================================================================
> > --- src/main/builtin.c (revision 72935)
> > +++ src/main/builtin.c (working copy)
> > @@ -888,6 +888,7 @@
> >
> >      SETCAR(t, CAR(x));
> >      SET_TAG(t, TAG(x));
> >  
> >   }
> >
> > + break;
> >
> >      case VECSXP:
> >   for (i = 0; i < len; i++)
> >  
> >      if (i < lenx) {
> >
> > ______________________________________________
> > [hidden email] mailing list
> > https://stat.ethz.ch/mailman/listinfo/r-devel

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix missing break

Martin Morgan-3
In reply to this post by Steve Grubb
On 07/20/2017 05:02 PM, Steve Grubb wrote:
> Hello,
>
> There appears to be a break missing in the switch/case for the LISTSXP case.
> If this is supposed to fall through, I'd suggest a comment so that others
> know its by design.
>
> Signed-off-by: Steve Grubb <[hidden email]>

An example is

$ R --vanilla -e "pl = pairlist(1, 2); length(pl) = 1; pl"
 > pl = pairlist(1, 2); length(pl) = 1; pl
Error in length(pl) = 1 :
   SET_VECTOR_ELT() can only be applied to a 'list', not a 'pairlist'
Execution halted

fixed in r72936 (R-devel) / 72937 (R-3-4-branch).

Martin Morgan

>
> Index: src/main/builtin.c
> ===================================================================
> --- src/main/builtin.c (revision 72935)
> +++ src/main/builtin.c (working copy)
> @@ -888,6 +888,7 @@
>      SETCAR(t, CAR(x));
>      SET_TAG(t, TAG(x));
>   }
> + break;
>       case VECSXP:
>   for (i = 0; i < len; i++)
>      if (i < lenx) {
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>


This email message may contain legally privileged and/or...{{dropped:2}}

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix missing break

Martin Maechler
In reply to this post by Steve Grubb
>>>>> Steve Grubb <[hidden email]>
>>>>>     on Thu, 20 Jul 2017 22:20:33 -0400 writes:

    > On Thursday, July 20, 2017 7:41:00 PM EDT Duncan Murdoch wrote:
    >> Thanks for posting this series of patches.  Unfortunately, there's a
    >> good chance they'll get lost in all the traffic on R-devel.  If you
    >> don't hear that they've been fixed in the next couple of weeks, could
    >> you post them to bugs.r-project.org, and post future patches there as well?

    > That was my first inclination. But there is no way to create an account unlike
    > most open source projects I work with. And I work with quite a lot.

It used to be easily possible, until someone urged students or
paid people to create such accounts and create crap bug reports.
For a while, we tried a few measures to deal with that but we
found no measure which was both fast and relatively foolproof.

--> See https://www.r-project.org/bugs.html and look for  "abuse".

I have now created an account for you.

    >> In examples like the one below, if you have R code that shows symptoms,
    >> it would really help in the bug report.


    > I am hoping that we can look at the code as seasoned programmers and say yeah,
    > that is a bug.

I agree in this case.
OTOH, it is exactly one of the case where the bug is not
triggerable currently:

  al <- formals(ls); length(al) <- 3

would trigger the bug... but you get an error message ".. vector .."
and as I now found that is from a slightly misguided check:
isVectorizable()  is not approriate here and should really be
replaced by isList().
 
So .. indeed, your report will have triggered an improvement in
the code, which I'm about to commit.

Thank you very much Steve!

    > I run the code through Coverity and have quite a lot of
    > problems to tell you about.

I'm not the expert on static code analysis, but as a seasoned
statistician (*and* from experience with other such analyses) I
know that you always get false positives.

    > I run these 5 out as tests to see how this
    > community works. I am new to this community but not necessarily R and just
    > want to contribute back to something I am using. But believe me, I have a
    > bunch more that seasoned programmers can eyeball and say yep - that's a bug.

Good, looking forward to see them.

    >> Otherwise, someone else will have to analyze the code to decide whether it's
    >> a bug or missing comment.  That takes time, and if there are no known
    >> symptoms, it's likely to be assigned a low priority.  The sad truth is that
    >> very few members of R Core are currently actively fixing bugs.

    > That's a shame. I'd be happy to give the scan to people in core so they can
    > see what the lay of the land looks like.

 (hmm... the above does look a teeny tiny bit arrogant in my
  eyes; but then I'm not a native English (nor "American" :-)
  speaker ...)


    > R works amazingly good. So much so I decided to dig
    > deeper. I'd recommend to the core developers that they ask
    > to get on Coverity's open source scan list.

    > https://scan.coverity.com/

    > It's free to open source projects like this. :-)

    > -Steve


    >> On 20/07/2017 5:02 PM, Steve Grubb wrote:
    >> > Hello,
    >> >
    >> > There appears to be a break missing in the switch/case for the LISTSXP
    >> > case. If this is supposed to fall through, I'd suggest a comment so that
    >> > others know its by design.
    >> >
    >> > Signed-off-by: Steve Grubb <[hidden email]>
    >> >
    >> > Index: src/main/builtin.c
    >> > ===================================================================
    >> > --- src/main/builtin.c (revision 72935)
    >> > +++ src/main/builtin.c (working copy)
    >> > @@ -888,6 +888,7 @@
    >> >
    >> >      SETCAR(t, CAR(x));
    >> >      SET_TAG(t, TAG(x));
    >> >  
    >> >   }
    >> >
    >> > + break;
    >> >
    >> >      case VECSXP:
    >> >   for (i = 0; i < len; i++)
    >> >  
    >> >      if (i < lenx) {
    >> >
    >> > ______________________________________________
    >> > [hidden email] mailing list
    >> > https://stat.ethz.ch/mailman/listinfo/r-devel

    > ______________________________________________
    > [hidden email] mailing list
    > https://stat.ethz.ch/mailman/listinfo/r-devel

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix missing break

Martin Maechler
In reply to this post by Martin Morgan-3
>>>>> Martin Morgan <[hidden email]>
>>>>>     on Fri, 21 Jul 2017 03:43:48 -0400 writes:

    > On 07/20/2017 05:02 PM, Steve Grubb wrote:
    >> Hello,
    >>
    >> There appears to be a break missing in the switch/case for the LISTSXP case.
    >> If this is supposed to fall through, I'd suggest a comment so that others
    >> know its by design.
    >>
    >> Signed-off-by: Steve Grubb <[hidden email]>

    > An example is

    > $ R --vanilla -e "pl = pairlist(1, 2); length(pl) = 1; pl"
    >> pl = pairlist(1, 2); length(pl) = 1; pl
    > Error in length(pl) = 1 :
    > SET_VECTOR_ELT() can only be applied to a 'list', not a 'pairlist'
    > Execution halted

    > fixed in r72936 (R-devel) / 72937 (R-3-4-branch).

    > Martin Morgan

Cool: The two  Martin M*'s  in the R core team have been "running in parallel".

The example which I meant where the missing break was hidden by
an earlier (wrong) errror message is this one:

> al <- formals(ls); length(al) <- 9
Error in length(al) <- 9 : invalid argument

which my extra changes will fix.

--
The other Martin M* - from the R Core Team

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix missing break

Steve Grubb
In reply to this post by Martin Maechler
Hello Martin,

On Friday, July 21, 2017 4:21:21 AM EDT Martin Maechler wrote:
> I have now created an account for you.

Thanks. Is that the preferred method of transferring these patches?


> >> In examples like the one below, if you have R code that shows symptoms,
> >> it would really help in the bug report.
> >
> > I am hoping that we can look at the code as seasoned programmers and say
> > yeah, that is a bug.
>
> I agree in this case.
> OTOH, it is exactly one of the case where the bug is not
> triggerable currently:
>
>   al <- formals(ls); length(al) <- 3
>
> would trigger the bug... but you get an error message ".. vector .."
> and as I now found that is from a slightly misguided check:
> isVectorizable()  is not approriate here and should really be
> replaced by isList().
>
> So .. indeed, your report will have triggered an improvement in
> the code, which I'm about to commit.

That's what it's all about.  :-)
 
> Thank you very much Steve!
>
> > I run the code through Coverity and have quite a lot of
> > problems to tell you about.
>
> I'm not the expert on static code analysis, but as a seasoned
> statistician (*and* from experience with other such analyses) I
> know that you always get false positives.

Absolutely. I weeded the report down to 15 issues to start with. There are
also ways to annotate the code so that checkers dismiss something it would
otherwise be inclined to report.

> >> Otherwise, someone else will have to analyze the code to decide whether
> >> it's a bug or missing comment.  That takes time, and if there are no
> >> known symptoms, it's likely to be assigned a low priority.  The sad
> >> truth is that very few members of R Core are currently actively fixing
> >> bugs.
> >
> > That's a shame. I'd be happy to give the scan to people in core so they
> > can see what the lay of the land looks like.
>
>  (hmm... the above does look a teeny tiny bit arrogant in my
>   eyes; but then I'm not a native English (nor "American"
>   speaker ...)

I apologize if that is the way it came across. "That's a shame" can also mean
"That's unfortunate" because I was thinking that I spent some time fixing up
patches that might not be wanted. However, I see that you have looked at the
patches and I thank you for that.  :-)

The second sentence above is an honest offer. I'd be happy to send the output
of the report off list (in case anything sensitive is listed). In this and the
other patches I haven't sent, I'm just picking the low hanging fruit.

-Steve

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix missing break

Martin Maechler
>>>>> Steve Grubb <[hidden email]>
>>>>>     on Fri, 21 Jul 2017 13:53:12 -0400 writes:

    > Hello Martin,
    > On Friday, July 21, 2017 4:21:21 AM EDT Martin Maechler wrote:
    >> I have now created an account for you.

    > Thanks. Is that the preferred method of transferring these patches?

in such a case, yes .. but don't ask for a full definition of "such
a case" ;-)
If the issue may be somewhat controversial and rather in the
spirit of "I don't like what R is doing here, and I think we
          should change ..."
we'd prefer it be posted here, first, in any case;  but you had
no such examples.  

    >> >> In examples like the one below, if you have R code that shows symptoms,
    >> >> it would really help in the bug report.
    >> >
    >> > I am hoping that we can look at the code as seasoned programmers and say
    >> > yeah, that is a bug.
    >>
    >> I agree in this case.
    >> OTOH, it is exactly one of the case where the bug is not
    >> triggerable currently:
    >>
    >> al <- formals(ls); length(al) <- 3
    >>
    >> would trigger the bug... but you get an error message ".. vector .."
    >> and as I now found that is from a slightly misguided check:
    >> isVectorizable()  is not approriate here and should really be
    >> replaced by isList().
    >>
    >> So .. indeed, your report will have triggered an improvement in
    >> the code, which I'm about to commit.

    > That's what it's all about.  :-)
 
    >> Thank you very much Steve!
    >>
    >> > I run the code through Coverity and have quite a lot of
    >> > problems to tell you about.
    >>
    >> I'm not the expert on static code analysis, but as a seasoned
    >> statistician (*and* from experience with other such analyses) I
    >> know that you always get false positives.

    > Absolutely. I weeded the report down to 15 issues to start with. There are
    > also ways to annotate the code so that checkers dismiss something it would
    > otherwise be inclined to report.

    >> >> Otherwise, someone else will have to analyze the code to decide whether
    >> >> it's a bug or missing comment.  That takes time, and if there are no
    >> >> known symptoms, it's likely to be assigned a low priority.  The sad
    >> >> truth is that very few members of R Core are currently actively fixing
    >> >> bugs.
    >> >
    >> > That's a shame. I'd be happy to give the scan to people in core so they
    >> > can see what the lay of the land looks like.
    >>
    >> (hmm... the above does look a teeny tiny bit arrogant in my
    >> eyes; but then I'm not a native English (nor "American"
    >> speaker ...)

    > I apologize if that is the way it came across. "That's a shame" can also mean
    > "That's unfortunate" because I was thinking that I spent some time fixing up
    > patches that might not be wanted. However, I see that you have looked at the
    > patches and I thank you for that.  :-)

    > The second sentence above is an honest offer. I'd be happy to send the output
    > of the report off list (in case anything sensitive is listed). In this and the
    > other patches I haven't sent, I'm just picking the low hanging fruit.

    > -Steve

Ok, thank you for the offer!   In general, we would prefer public
communication of such issues because it can help to spread the
volunteer work load a bit wider than only to R Core.   OTOH,
yes, there are important exceptions to this rule, as we know.

Martin

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel