acassis commented on code in PR #20057:
URL: https://github.com/apache/nuttx/pull/20057#discussion_r3935927919
##########
drivers/timers/arch_alarm.c:
##########
@@ -288,16 +289,7 @@ void weak_function up_timer_getmask(FAR clock_t *mask)
ONESHOT_TICK_MAX_DELAY(g_oneshot_lower, &maxticks);
- for (; ; )
- {
- clock_t next = (*mask << 1) | 1;
- if (next > maxticks)
- {
- break;
- }
-
- *mask = next;
- }
+ *mask = CLOCK_MAX >> (sizeof(clock_t) * 8u - flsx(maxticks));
Review Comment:
I think there is an issue here if maxticks == 0, in the original code if
maxticks is equal 0, that "if (next > maxticks)" was preventing the *mask =
next to be executed, so *mask = 0 still valid. with you modification we will
have an issue because clock_t is 64-bit (8 bytes), so 8 * 8u = 64 and CLOCK_MAX
<< 64 is undefined behavior on C:
"If the value of the right operand is negative or is greater than or equal
to the width of the promoted left operand, the behavior is undefined."
Please change to:
```
*mask = maxticks == 0
? 0
: CLOCK_MAX >> (sizeof(clock_t) * 8u - flsx(maxticks));
```
##########
drivers/timers/arch_timer.c:
##########
@@ -280,17 +282,7 @@ void weak_function up_timer_getmask(FAR clock_t *mask)
TIMER_TICK_MAXTIMEOUT(g_timer.lower, &maxticks);
- *mask = 0;
- while (1)
- {
- clock_t next = (*mask << 1) | 1;
- if (next > maxticks)
- {
- break;
- }
-
- *mask = next;
- }
+ *mask = CLOCK_MAX >> (sizeof(clock_t) * 8u - flsx(maxticks));
Review Comment:
Ditto
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
