You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/28 16:36:28 UTC
[GitHub] [incubator-nuttx] Ouss4 opened a new pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Ouss4 opened a new pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246
## Summary
libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes. It looks like the code was copied from pthread_barrierattr_init.c
## Impact
N/A
## Testing
esp32-devkitc:smp
esp32-devkitc:ostest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888492380
> Quoting from the Description section of standard latest version:
>
> > The behavior is undefined if the value specified by the attr argument to pthread_barrierattr_destroy() does not refer to an initialized barrier attributes object.
>
> And the update on Issue 7:
>
> > The [EINVAL] error for an uninitialized barrier attributes object is removed; this condition results in undefined behavior.
>
I think that "an uninitialized barrier attributes" mean the content of pointer(*attr), not pointer self(attr). The NULL pointer is always invalid.
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_barrierattr_destroy.html
>
> So `pthread_barrierattr_destroy` should simply return 0, which is the same behavior from other implementations:
>
> * glibc: https://chromium.googlesource.com/chromiumos/third_party/glibc/+/refs/heads/master/nptl/pthread_barrierattr_destroy.c
> * musl: http://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_barrierattr_destroy.c
But openbsd check it:
https://github.com/openbsd/src/blob/master/lib/librthread/rthread_barrier_attr.c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888480481
Quoting from the Description section of standard latest version:
> The behavior is undefined if the value specified by the attr argument to pthread_barrierattr_destroy() does not refer to an initialized barrier attributes object.
And the update on Issue 7:
> The [EINVAL] error for an uninitialized barrier attributes object is removed; this condition results in undefined behavior.
So `pthread_barrierattr_destroy` should simply return 0, which is the same behavior from other implementations:
- glibc: https://chromium.googlesource.com/chromiumos/third_party/glibc/+/refs/heads/master/nptl/pthread_barrierattr_destroy.c
- musl: http://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_barrierattr_destroy.c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888480481
Quoting from the Description section of standard latest version:
> The behavior is undefined if the value specified by the attr argument to pthread_barrierattr_destroy() does not refer to an initialized barrier attributes object.
And the update on Issue 7:
> The [EINVAL] error for an uninitialized barrier attributes object is removed; this condition results in undefined behavior.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_barrierattr_destroy.html
So `pthread_barrierattr_destroy` should simply return 0, which is the same behavior from other implementations:
- glibc: https://chromium.googlesource.com/chromiumos/third_party/glibc/+/refs/heads/master/nptl/pthread_barrierattr_destroy.c
- musl: http://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_barrierattr_destroy.c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888487331
@acassis but @gustavonihei 's comment isn't related to this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888554822
I don't know how any of this is related to this PR.
> So pthread_barrierattr_destroy should simply return 0, which is the same behavior from other implementations:
The current implementation returns just 0. Return `EINVAL` for these situations is common and is part of the standard.
I did see the glibc code before, that just returns 0, but we should not just take that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888568641
> @Ouss4 was this issue happening when running "ostest"? Or should a new test be added to verify it?
No, I just saw that that part was copied from `pthread_barrierattr_init` and is not supposed to be in `pthread_barrierattr_destroy`. ostest tests the functionality of barriers in general.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888477624
@Ouss4 was this issue happening when running "ostest"? Or should a new test be added to verify it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-889337785
> @acassis but @gustavonihei 's comment isn't related to this change.
Indeed, not strictly related, but impacts the same function, which has less than 10 LOC.
The topic seems more controversial than I expected, let's just merge this PR and leave the discussion to a future PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei merged pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
gustavonihei merged pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888497055
> > Quoting from the Description section of standard latest version:
> > > The behavior is undefined if the value specified by the attr argument to pthread_barrierattr_destroy() does not refer to an initialized barrier attributes object.
> >
> >
> > And the update on Issue 7:
> > > The [EINVAL] error for an uninitialized barrier attributes object is removed; this condition results in undefined behavior.
>
> I think that "an uninitialized barrier attributes" mean the content of pointer(*attr), not pointer self(attr). The NULL pointer is always invalid.
>
Does the `-EINVAL` return code really matters for the callers of `pthread_barrierattr_destroy`? Probably no one will explicitly check for `-EINVAL`.
If the idea for returning the error code is just for it to not go unnoticed, a `DEBUGASSERT(attr != NULL)` does a better job.
And still the implementation remains compliant to the standard.
> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_barrierattr_destroy.html
> > So `pthread_barrierattr_destroy` should simply return 0, which is the same behavior from other implementations:
> >
> > * glibc: https://chromium.googlesource.com/chromiumos/third_party/glibc/+/refs/heads/master/nptl/pthread_barrierattr_destroy.c
> > * musl: http://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_barrierattr_destroy.c
>
> But openbsd check it:
> https://github.com/openbsd/src/blob/master/lib/librthread/rthread_barrier_attr.c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888497055
> > Quoting from the Description section of standard latest version:
> > > The behavior is undefined if the value specified by the attr argument to pthread_barrierattr_destroy() does not refer to an initialized barrier attributes object.
> >
> >
> > And the update on Issue 7:
> > > The [EINVAL] error for an uninitialized barrier attributes object is removed; this condition results in undefined behavior.
>
> I think that "an uninitialized barrier attributes" mean the content of pointer(*attr), not pointer self(attr). The NULL pointer is always invalid.
>
Does the `-EINVAL` return code really matters for the callers of `pthread_barrierattr_destroy`? Probably no one will explicitly check for `-EINVAL`.
If the idea for returning the error code is just for it to not go unnoticed, a `DEBUGASSERT(attr != NULL)` does a better job.
And still the implementation remains compliant to the standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888484447
Good catch @gustavonihei ! I converted it to Draft to avoid someone merging it before the improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4246: libc/pthread_barrierattr_destory.c: Destroy shouldn't reinitialize the attributes
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4246:
URL: https://github.com/apache/incubator-nuttx/pull/4246#issuecomment-888492380
> Quoting from the Description section of standard latest version:
>
> > The behavior is undefined if the value specified by the attr argument to pthread_barrierattr_destroy() does not refer to an initialized barrier attributes object.
>
> And the update on Issue 7:
>
> > The [EINVAL] error for an uninitialized barrier attributes object is removed; this condition results in undefined behavior.
>
I think that "an uninitialized barrier attributes" mean the content of pointer, not pointer self. The NULL pointer is always invalid.
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_barrierattr_destroy.html
>
> So `pthread_barrierattr_destroy` should simply return 0, which is the same behavior from other implementations:
>
> * glibc: https://chromium.googlesource.com/chromiumos/third_party/glibc/+/refs/heads/master/nptl/pthread_barrierattr_destroy.c
> * musl: http://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_barrierattr_destroy.c
But openbsd check it:
https://github.com/openbsd/src/blob/master/lib/librthread/rthread_barrier_attr.c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org