Timer race condition #243

2021-03-08T15:47:01-06:00

kilograham commented

2021-03-08 15:47:01 -06:00

(Migrated from github.com)

fixes #195

lurch (Migrated from github.com) reviewed

2021-03-08 16:28:04 -06:00

test/pico_time_test/pico_time_test.c

					
				@ -221,0 +231,4 @@

				}

				int issue_195_test(void) {

				    PICOTEST_START_SECTION("Issue #195 race condition - without fix may hang on gcc 10.2.1 release builds");

Apologies if this is a stupid question, but this seems to be checking the return from issue_195_test() but AFAICT that function will only ever return 0 ? Was that function supposed to return 1 if the delta between expected_count and issue_195_counter was too big?

Apologies if this is a stupid question, but this seems to be checking the return from `issue_195_test()` but AFAICT that function will only ever `return 0` ? Was that function supposed to `return 1` if the delta between `expected_count` and `issue_195_counter` was too big?

kilograham (Migrated from github.com) reviewed

2021-03-08 18:07:39 -06:00

test/pico_time_test/pico_time_test.c

					
				@ -221,0 +231,4 @@

				}

				int issue_195_test(void) {

				    PICOTEST_START_SECTION("Issue #195 race condition - without fix may hang on gcc 10.2.1 release builds");

nope; there are macros

lurch (Migrated from github.com) reviewed

2021-03-08 19:54:18 -06:00

test/pico_time_test/pico_time_test.c

					
				@ -221,0 +231,4 @@

				}

				int issue_195_test(void) {

				    PICOTEST_START_SECTION("Issue #195 race condition - without fix may hang on gcc 10.2.1 release builds");

Perhaps I'm being really stupid, but I can see that issue_195_test() has a PICOTEST_START_SECTION macro which sets picotest_error_code = 0 and there's a PICOTEST_END_SECTION macro which checks if (picotest_error_code != 0), but I can't tell where (in issue_195_test()) picotest_error_code actually gets modified 😕

Perhaps I'm being _really_ stupid, but I can see that `issue_195_test()` has a `PICOTEST_START_SECTION` macro which [sets](https://github.com/raspberrypi/pico-sdk/blob/master/test/pico_test/include/pico/test.h#L22) `picotest_error_code = 0` and there's a `PICOTEST_END_SECTION` macro which [checks](https://github.com/raspberrypi/pico-sdk/blob/master/test/pico_test/include/pico/test.h#L24) `if (picotest_error_code != 0)`, but I can't tell where (in `issue_195_test()`) `picotest_error_code` actually gets modified :confused:

kilograham (Migrated from github.com) reviewed

2021-03-08 20:56:31 -06:00

test/pico_time_test/pico_time_test.c

					
				@ -221,0 +231,4 @@

				}

				int issue_195_test(void) {

				    PICOTEST_START_SECTION("Issue #195 race condition - without fix may hang on gcc 10.2.1 release builds");

the convention i am propagating is that tests return 0 on success or otherwise on failure which is what the macros do in general. Yes there is no actual check here atm (as it causes a hang in the failure case)... it does not matter

the convention i am propagating is that tests return 0 on success or otherwise on failure which is what the macros do in general. Yes there is no actual check here atm (as it causes a hang in the failure case)... it _does not matter_

lurch (Migrated from github.com) reviewed

2021-03-09 04:23:10 -06:00

test/pico_time_test/pico_time_test.c

					
				@ -221,0 +231,4 @@

				}

				int issue_195_test(void) {

				    PICOTEST_START_SECTION("Issue #195 race condition - without fix may hang on gcc 10.2.1 release builds");

(as it causes a hang in the failure case)... it does not matter

Oh, that's the bit I was missing. Sorry for pestering you!

> (as it causes a hang in the failure case)... it does not matter Oh, that's the bit I was missing. Sorry for pestering you!

Wren6991 (Migrated from github.com) approved these changes

2021-03-10 06:21:37 -06:00

Wren6991 (Migrated from github.com) left a comment

So the failure mode was an alarm firing at the point we were in the critical section setting an immediately-expiring timer on the same alarm, and getting dropped as a result?

kilograham commented

2021-03-10 07:06:15 -06:00

(Migrated from github.com)

So the failure mode was an alarm firing at the point we were in the critical section setting an immediately-expiring timer on the same alarm, and getting dropped as a result?

setting an immediately expiring timer when there was already a timer that had fired but not yet been serviced (because we were under lock)

> So the failure mode was an alarm firing at the point we were in the critical section setting an immediately-expiring timer on the same alarm, and getting dropped as a result? setting an immediately expiring timer when there was already a timer that had fired but not yet been serviced (because we were under lock)

kilograham commented

2021-03-10 07:26:08 -06:00

(Migrated from github.com)

note it also requires a mix of timers adding in IRQ (i.e. repeating) and non IRQ contexts (we already had a test which created a huge number of timers with random timeouts and makes sure they all happen and in the right order - just none of those repeated)

Sign in to join this conversation.

No reviewers

Wren6991