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 2022/04/06 10:01:58 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request, #5992: sched/note: correct flatten format
anchao opened a new pull request, #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992
## Summary
sched/note: correct flatten format
## Impact
Fix endian issue reported by @pkarashchenko :
https://github.com/apache/incubator-nuttx-apps/pull/1114#discussion_r842466352
## Testing
sched note trace test
--
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] pkarashchenko commented on a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844689585
##########
sched/sched/sched_note.c:
##########
@@ -120,20 +120,21 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
switch (len)
{
-#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
-#endif
case 4:
- dst[3] = (uint8_t)((*(uint32_t *)src >> 24) & 0xff);
- dst[2] = (uint8_t)((*(uint32_t *)src >> 16) & 0xff);
case 2:
- dst[1] = (uint8_t)((*(uint16_t *)src >> 8) & 0xff);
case 1:
- dst[0] = (uint8_t)(*(uint8_t *)src & 0xff);
+#ifdef CONFIG_ENDIAN_BIG
Review Comment:
Switch partially ensures that we are handling basic types only.
--
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 a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844893057
##########
sched/sched/sched_note.c:
##########
@@ -120,20 +120,21 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
switch (len)
{
-#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
-#endif
case 4:
- dst[3] = (uint8_t)((*(uint32_t *)src >> 24) & 0xff);
- dst[2] = (uint8_t)((*(uint32_t *)src >> 16) & 0xff);
case 2:
- dst[1] = (uint8_t)((*(uint16_t *)src >> 8) & 0xff);
case 1:
- dst[0] = (uint8_t)(*(uint8_t *)src & 0xff);
+#ifdef CONFIG_ENDIAN_BIG
Review Comment:
Yes, it's much better than switch/case.
--
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] pkarashchenko commented on a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844689585
##########
sched/sched/sched_note.c:
##########
@@ -120,20 +120,21 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
switch (len)
{
-#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
-#endif
case 4:
- dst[3] = (uint8_t)((*(uint32_t *)src >> 24) & 0xff);
- dst[2] = (uint8_t)((*(uint32_t *)src >> 16) & 0xff);
case 2:
- dst[1] = (uint8_t)((*(uint16_t *)src >> 8) & 0xff);
case 1:
- dst[0] = (uint8_t)(*(uint8_t *)src & 0xff);
+#ifdef CONFIG_ENDIAN_BIG
Review Comment:
Switch partially ensures that we are handling standard types only.
--
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] pkarashchenko commented on a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r843867339
##########
sched/sched/sched_note.c:
##########
@@ -122,18 +123,17 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
+ *(uint64_t *)dst = htole64(*(uint64_t *)src);
Review Comment:
Are we sure that `dst` points to properly aligned memory?
--
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] anchao commented on a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
anchao commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r843996689
##########
sched/sched/sched_note.c:
##########
@@ -122,18 +123,17 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
+ *(uint64_t *)dst = htole64(*(uint64_t *)src);
Review Comment:
Yes, there are potential issues if the hardware do not support aligned access
--
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] pkarashchenko commented on a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844309610
##########
sched/sched/sched_note.c:
##########
@@ -116,24 +117,35 @@ static unsigned int g_note_disabled_irq_nest[CONFIG_SMP_NCPUS];
****************************************************************************/
static inline void sched_note_flatten(FAR uint8_t *dst,
- FAR void *src, size_t len)
+ FAR void *src_, size_t len)
{
+ FAR uint8_t *src = src_;
+
switch (len)
Review Comment:
Code is fine, but I was just thinking why just not to:
```
switch (len)
{
case 8:
case 4:
case 2:
case 1:
#ifdef CONFIG_ENDIAN_BIG
{
FAR uint8_t *end = (FAR uint8_t *)src + len - 1;
while (len-- > 0)
{
*dst++ = *end--;
}
}
#else
memcpy(dst, src, len);
#endif
break;
default:
DEBUGASSERT(FALSE);
break;
}
```
Seems to be more clear.
--
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 a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844838244
##########
sched/sched/sched_note.c:
##########
@@ -120,20 +120,21 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
switch (len)
{
-#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
-#endif
case 4:
- dst[3] = (uint8_t)((*(uint32_t *)src >> 24) & 0xff);
- dst[2] = (uint8_t)((*(uint32_t *)src >> 16) & 0xff);
case 2:
- dst[1] = (uint8_t)((*(uint16_t *)src >> 8) & 0xff);
case 1:
- dst[0] = (uint8_t)(*(uint8_t *)src & 0xff);
+#ifdef CONFIG_ENDIAN_BIG
Review Comment:
But it's just an internal function, we can trust caller.
--
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] pkarashchenko commented on a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844883924
##########
sched/sched/sched_note.c:
##########
@@ -120,20 +120,21 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
switch (len)
{
-#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
-#endif
case 4:
- dst[3] = (uint8_t)((*(uint32_t *)src >> 24) & 0xff);
- dst[2] = (uint8_t)((*(uint32_t *)src >> 16) & 0xff);
case 2:
- dst[1] = (uint8_t)((*(uint16_t *)src >> 8) & 0xff);
case 1:
- dst[0] = (uint8_t)(*(uint8_t *)src & 0xff);
+#ifdef CONFIG_ENDIAN_BIG
Review Comment:
makes sense
--
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] anchao commented on a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
anchao commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844605547
##########
sched/sched/sched_note.c:
##########
@@ -116,24 +117,35 @@ static unsigned int g_note_disabled_irq_nest[CONFIG_SMP_NCPUS];
****************************************************************************/
static inline void sched_note_flatten(FAR uint8_t *dst,
- FAR void *src, size_t len)
+ FAR void *src_, size_t len)
{
+ FAR uint8_t *src = src_;
+
switch (len)
Review Comment:
Done
--
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 a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844649749
##########
sched/sched/sched_note.c:
##########
@@ -120,20 +120,21 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
switch (len)
{
-#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
-#endif
case 4:
- dst[3] = (uint8_t)((*(uint32_t *)src >> 24) & 0xff);
- dst[2] = (uint8_t)((*(uint32_t *)src >> 16) & 0xff);
case 2:
- dst[1] = (uint8_t)((*(uint16_t *)src >> 8) & 0xff);
case 1:
- dst[0] = (uint8_t)(*(uint8_t *)src & 0xff);
+#ifdef CONFIG_ENDIAN_BIG
Review Comment:
let's remove switch
--
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] pkarashchenko commented on a diff in pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992#discussion_r844885303
##########
sched/sched/sched_note.c:
##########
@@ -120,20 +120,21 @@ static inline void sched_note_flatten(FAR uint8_t *dst,
{
switch (len)
{
-#ifdef CONFIG_HAVE_LONG_LONG
case 8:
- dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
- dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
- dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
- dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
-#endif
case 4:
- dst[3] = (uint8_t)((*(uint32_t *)src >> 24) & 0xff);
- dst[2] = (uint8_t)((*(uint32_t *)src >> 16) & 0xff);
case 2:
- dst[1] = (uint8_t)((*(uint16_t *)src >> 8) & 0xff);
case 1:
- dst[0] = (uint8_t)(*(uint8_t *)src & 0xff);
+#ifdef CONFIG_ENDIAN_BIG
Review Comment:
we can leave `DEBUGASSERT(len == 1 || len == 2 || len == 4 || len == 8);` optionally.
--
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 merged pull request #5992: sched/note: correct flatten format
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #5992:
URL: https://github.com/apache/incubator-nuttx/pull/5992
--
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