tests/pthread_rwlock: run the test in CI#11688
Conversation
cladmi
added
Area: tests
|
The test failed on |
cladmi
force-pushed
the
pr/tests/pthread_rwlock/enable_in_ci
branch
from
778f130 to
9e835a8
Compare
|
Looking more at the code and the output, I think this is just a non de-scheduling aware test |
|
This now added a |
@cladmi do you have a way to reproduce locally? |
|
|
||
| /* The test assumes that 'printf/puts' are non interruptible operations | ||
| * use a mutex to guarantee it */ | ||
| static mutex_t stdout_mutex = MUTEX_INIT; |
There was a problem hiding this comment.
Couldn't this be generalized? I would imagine more use cases where interrupted prints can screw up a tests. #11818 deactivated ISR to achieve a similar result.
RIOT/examples/suit_update/main.c
Lines 43 to 47 in 0440c64
There was a problem hiding this comment.
Deactivated ISR do not work if the test is using DMA printfs.
As we cannot know in RIOT if interrupt are disabled, the dma implementation of uart_write cannot do an disabled interrupt safe implementation.
And printing is really slow so disabling interrupts for a long time seems bad.
I was talking about having a console_lock in the dma print, that could also be useful when you want a block to be printed as a transaction that could even be done in multiple printf by explicitly taking the lock.
#10800 (comment)
It would need to be a rlock though.
There was a problem hiding this comment.
I was talking about having a console_lock in the dma print, that could also be useful when you want a block to be printed as a transaction that could even be done in multiple printf by explicitly taking the lock.
#10800 (comment)
It would need to be a rlock though.
+1, even if it would need rlock, I think it would make sense to have a feature like this for testing. Do you have plans on working on this soon? If not we can go with the local implementation for now, but I wouldn't want it to start being replicated every time we need to make sure prints are correctly going threw.
There was a problem hiding this comment.
Edit: in the dma print discussion
No plans to work on it soon, I also have no idea how it can be done at the C library level "automatically" :/
There was a problem hiding this comment.
An implementation could be to define a console_lock rmutex and locked_printf, locked_puts macros or functions (and others).
But it would only solve the case where they are explicitly used, so more in the tests applications. It would be enough as long as there is no other debug outputs. So not when using the gnrc debug module at the same time, or drivers putting debug messages for example.
If this is enough I could try putting some time on it.
Managing to make it the default could be another task and split in a dedicated module like "printf_auto_rlock".
There was a problem hiding this comment.
An implementation could be to define a console_lock rmutex and locked_printf, locked_puts macros or functions (and others).
But it would only solve the case where they are explicitly used, so more in the tests applications. It would be enough as long as there is no other debug outputs. So not when using the gnrc debug module at the same time, or drivers putting debug messages for example.
Hmm, thats true. Many of the use cases I can think fall in the printf_auto_rlock case. I think it makes more sense working on "printf_auto_rlock", I'll think it over a little more and see if I can find more use cases, but if not we can go with this as is.
I am not sure, if I remember correctly, my comment in #11688 (comment) was copied from the I could try finding one failing node on IoT-LAB if it is board specific. |
|
I do not have a failing test here, but the mixed output. And there is no mixed output with this PR (rebased on master for good measure).
|
cladmi
force-pushed
the
pr/tests/pthread_rwlock/enable_in_ci
branch
from
6170862 to
48a4737
Compare
|
I re-ordered the commits to have the fix before enabling testing. |
That's enough for me, I can see why the issue can arise and it is present in the PR comment. |
|
Ok there is clearly an issue with |
fjmolinas
added
CI: ready for build
|
So #11995 is merged, @fjmolinas ? |
jcarrano
left a comment
jcarrano
left a comment
There was a problem hiding this comment.
I'm almost fine with this, except for uncontained multi-line macros. It is not terrible, but it gives a bad example to future contributors.
tests/pthread_rwlock/main.c
Outdated
| static mutex_t stdout_mutex = MUTEX_INIT; | ||
|
|
||
| #define PRINTF(FMT, ...) \ | ||
| mutex_lock(&stdout_mutex); \ |
There was a problem hiding this comment.
| mutex_lock(&stdout_mutex); \ | |
| do { mutex_lock(&stdout_mutex); \ |
tests/pthread_rwlock/main.c
Outdated
| sched_active_thread->priority, \ | ||
| (int)__VA_ARGS__) | ||
| (int)__VA_ARGS__); \ | ||
| mutex_unlock(&stdout_mutex) |
There was a problem hiding this comment.
| mutex_unlock(&stdout_mutex) | |
| mutex_unlock(&stdout_mutex) \ | |
| } while (0); |
There was a problem hiding this comment.
I did it without the terminating ;.
tests/pthread_rwlock/main.c
Outdated
| (int)__VA_ARGS__); \ | ||
| mutex_unlock(&stdout_mutex) | ||
|
|
||
| #define PUTS(s) \ |
There was a problem hiding this comment.
| #define PUTS(s) \ | |
| #define PUTS(s) do { \ |
tests/pthread_rwlock/main.c
Outdated
| #define PUTS(s) \ | ||
| mutex_lock(&stdout_mutex); \ | ||
| puts(s); \ | ||
| mutex_unlock(&stdout_mutex) |
There was a problem hiding this comment.
| mutex_unlock(&stdout_mutex) | |
| mutex_unlock(&stdout_mutex) \ | |
| } while (0); |
|
Indeed, they definitely need a |
|
I also indented to match the normal style in the source code. |
jcarrano
added
CI: needs squashing
cladmi
force-pushed
the
pr/tests/pthread_rwlock/enable_in_ci
branch
from
eecc37a to
0a138f6
Compare
|
Squashed. |
jcarrano
added
CI: ready for build
The test assumes that 'printf/puts' are non interruptible operations. Use a mutex to guarantee it. Without this, the automated test was failing in some configurations with lines being cut by others.
HACK, the test currently fails in CI for `native` due to `utf-8` handling.
cladmi
force-pushed
the
pr/tests/pthread_rwlock/enable_in_ci
branch
from
0a138f6 to
9cd08d4
Compare
|
I rebased to fix the conflicts from #11680 |
|
Thank you for the review. |
6 participants
Contribution description
Enable running the test as the test is available.
Testing procedure
The test must be successfully executed by CI (check the listed tests).
Issues/PRs references
Found it was not enabled while working on #11680
I blacklisted
nativedue to #11691