sclang: fix hang on floating point arguments to Integer:forBy #3804

lnihlen · 2018-06-22T21:15:27Z

Regarding #2690.

Previously Integer:forBy would truncate floating point arguments, to integers,
which could potentially result in an infinite loop by the Integer:forBy
optimized special operation. This modification checks for any of the arguments
to be floating point, and if detecting that casts all of the arguments to
floating point. It also adds appropriate testing coverage.

Previously Integer:forBy would truncate floating point arguments, to integers, which could potentially result in an infinite loop by the Integer:forBy optimized special operation. This modification checks for any of the arguments to be floating point, and if detecting that casts all of the arguments to floating point. It also adds appropriate testing coverage.

nhthn

thanks! something i just realized -- there's still an infinite loop if you set the step to float or int 0. i think it's reasonable to guard against such cases.

mossheim

Thanks for this @lnihlen ! Sorry it took awhile to get attention on it. Comments below, just style points

mossheim · 2018-06-24T00:48:18Z

lang/LangSource/PyrInterpreter3.cpp

 					dispatch_opcode;
 				}
 				case 8 : {
 					PyrSlot * vars = g->frame->vars;
-					if ((slotRawInt(&vars[2]) >= 0 && slotRawInt(&vars[4]) <= slotRawInt(&vars[1]))
-							|| (slotRawInt(&vars[2]) < 0 && slotRawInt(&vars[4]) >= slotRawInt(&vars[1]))) {
+					bool continue_for_by = false;


the codebase isn't super consistent but I'd say we generally prefer camelCase naming for local variables.

mossheim · 2018-06-24T00:48:35Z

lang/LangSource/PyrInterpreter3.cpp

-						error("Integer-forBy : endval or stepval not an Integer.\n");
+					// If any argument is floating point we cast all arguments
+					// to floating point, including the accumulator. This avoids
+					// potential infinite loops due to integer truncation.


mossheim · 2018-06-24T00:50:07Z

testsuite/sclang/TestInteger.sc

@@ -0,0 +1,29 @@
+TestInteger : UnitTest {
+	test_forby_with_positive_integer_arguments {
+		var forby_arguments = List.new;


and in sclang code we definitely prefer camelCase

mossheim · 2018-06-24T00:56:25Z

testsuite/sclang/TestInteger.sc

@@ -0,0 +1,29 @@
+TestInteger : UnitTest {
+	test_forby_with_positive_integer_arguments {


It's not 100% standardized yet, but I've got a WIP document for the project test suite here. The naming convention I'm trying to adopt is test_methodName_inputConditions[_expectedResult], so following that this test should be named something like test_forBy_positiveIntegerArguments. Also, you can omit the messages if you want, it's nice to have but IMO they're not super helpful beyond the names of the test functions.

Done, thanks for the pointer to the guide.

Also responds to style feedback.

lnihlen · 2018-06-24T08:21:49Z

Thanks for the feedback. I added error detection for the case of the zero stepval. There's still a possibility of infinite loop by underflow but with 64-bit doubles this seems less troublesome.

lnihlen · 2018-06-24T08:25:02Z

Oh, on the testing - I looked but didn't see any way to assert in testing that a particular operation results in a language error. Something like this.assertAsserts(function, expectedError, ...)
would allow me to test the infinite loop error case as well as the non-integer or float argument error case.

mossheim · 2018-06-24T14:19:40Z

I added error detection for the case of the zero stepval. There's still a possibility of infinite loop by underflow but with 64-bit doubles this seems less troublesome.

Thanks for that!

Oh, on the testing - I looked but didn't see any way to assert in testing that a particular operation results in a language error.

We have an open PR for this actually - #3808 . If you don't mind waiting a few days, it should be merged soon and then we rebase this PR and use it!

lnihlen · 2018-06-24T14:34:26Z

In the interest of expedience would it be reasonable for me to accept an open issue to add test coverage for the two failure cases (non-numeric arguments and zero stepval), with the agreement that I would put that up for review once #3808 is in?

lnihlen · 2018-06-24T14:36:59Z

I mean, and then merge this PR in the meantime?

mossheim · 2018-06-24T16:01:36Z

In the interest of expedience would it be reasonable for me to accept an open issue to add test coverage for the two failure cases (non-numeric arguments and zero stepval), with the agreement that I would put that up for review once #3808 is in?

Not sure. In general our workflow has been to make things like this blocking, which I think helps to cut down on bookkeeping (and because people sometimes forget or fail to follow through on promises). But you've been staying on top of this one and it already has decent tests so let's give it a try. :)

mossheim

Approved as discussed :)

mossheim · 2018-06-24T16:03:36Z

testsuite/classlibrary/TestInteger.sc

+		21.forBy(14.5, -3.125, { | i, j | forByArguments.add([i, j]); });
+		this.assertEquals(forByArguments, List[[21.0, 0], [17.875, 1], [14.75, 2]]);
+	}
+}


An unfortunate bug that hasn't been addressed - the IDE doesn't append a newline to the end of the final line of a document, so most files edited with it will appear to be missing a final newline in git. I looked into it a couple months ago but didn't really come up with anything, and ended up moving on to other issues. Something to keep in mind for the future

nhthn

thanks! looks good

lnihlen · 2018-06-24T17:31:02Z

Thanks for the review. In general my inclination is to decouple PRs as much as possible, to avoid possible propagation of delay through the PR dependency chain. Very hand-wavy, and I can't come up with a more concrete reason, so I appreciate the accommodation.

lnihlen mentioned this pull request Jun 22, 2018

sclang: fixes hardlock on floating point increments to Integer::forBy #3798

Closed

nhthn self-requested a review June 22, 2018 21:16

nhthn added the comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" label Jun 22, 2018

nhthn requested changes Jun 24, 2018

View reviewed changes

mossheim suggested changes Jun 24, 2018

View reviewed changes

nhthn added this to the 3.10 milestone Jun 24, 2018

sclang: error in Integer:forBy if zero-valued stepval detected.

8052cb1

Also responds to style feedback.

mossheim approved these changes Jun 24, 2018

View reviewed changes

nhthn approved these changes Jun 24, 2018

View reviewed changes

nhthn merged commit bdc18cf into supercollider:develop Jun 24, 2018

mossheim mentioned this pull request Jun 24, 2018

Add assertException tests for Integer.forBy #3811

Closed

lnihlen deleted the topic/sclang_fix_integer_forby_hang branch June 24, 2018 17:31

nhthn mentioned this pull request Jun 25, 2018

Integer:forBy can hang interpreter with a floating point step #2690

Closed

lnihlen mentioned this pull request Jul 10, 2018

sclang: adds exception tests for Integer:forBy #3857

Merged

mossheim mentioned this pull request Nov 13, 2018

Interval(0,1,1/3).asArray crashes sclang because forBy doesn't count with floats <1 #4154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sclang: fix hang on floating point arguments to Integer:forBy #3804

sclang: fix hang on floating point arguments to Integer:forBy #3804

lnihlen commented Jun 22, 2018

nhthn left a comment

mossheim left a comment

mossheim Jun 24, 2018

lnihlen Jun 24, 2018

mossheim Jun 24, 2018

mossheim Jun 24, 2018

lnihlen Jun 24, 2018

mossheim Jun 24, 2018

lnihlen Jun 24, 2018

lnihlen commented Jun 24, 2018

lnihlen commented Jun 24, 2018

mossheim commented Jun 24, 2018

lnihlen commented Jun 24, 2018

lnihlen commented Jun 24, 2018

mossheim commented Jun 24, 2018

mossheim left a comment

mossheim Jun 24, 2018

nhthn left a comment

lnihlen commented Jun 24, 2018

		@@ -0,0 +1,29 @@
		TestInteger : UnitTest {
		test_forby_with_positive_integer_arguments {

sclang: fix hang on floating point arguments to Integer:forBy #3804

sclang: fix hang on floating point arguments to Integer:forBy #3804

Conversation

lnihlen commented Jun 22, 2018

nhthn left a comment

Choose a reason for hiding this comment

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnihlen commented Jun 24, 2018

lnihlen commented Jun 24, 2018

mossheim commented Jun 24, 2018

lnihlen commented Jun 24, 2018

lnihlen commented Jun 24, 2018

mossheim commented Jun 24, 2018

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhthn left a comment

Choose a reason for hiding this comment

lnihlen commented Jun 24, 2018