Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: replace sprintf with snprintf #746

Closed
wants to merge 8 commits into from
Closed

feat: replace sprintf with snprintf #746

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b530791
feat: replace sprintf with snprintf
araujo88 Aug 10, 2023
c2746bf
Merge branch 'main' into feat/replace-sprintf-with-snprintf
araujo88 Aug 17, 2023
0e4f0c6
fix: remove macro
araujo88 Aug 17, 2023
842b0ac
Merge branch 'main' into feat/replace-sprintf-with-snprintf
Skptak Aug 19, 2023
e08494f
Merge branch 'main' into feat/replace-sprintf-with-snprintf
araujo88 Aug 23, 2023
6295112
Merge branch 'main' into feat/replace-sprintf-with-snprintf
paulbartell Aug 28, 2023
22b33da
Merge branch 'main' into feat/replace-sprintf-with-snprintf
araujo88 Sep 13, 2023
2a19f7d
Uncrustify: triggered by comment.
actions-user Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 50 additions & 10 deletions tasks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6617,8 +6617,16 @@ static void prvResetNextTaskUnblockTime( void )
pcWriteBuffer = prvWriteNameToBuffer( pcWriteBuffer, pxTaskStatusArray[ x ].pcTaskName );

/* Write the rest of the string. */
sprintf( pcWriteBuffer, "\t%c\t%u\t%u\t%u\r\n", cStatus, ( unsigned int ) pxTaskStatusArray[ x ].uxCurrentPriority, ( unsigned int ) pxTaskStatusArray[ x ].usStackHighWaterMark, ( unsigned int ) pxTaskStatusArray[ x ].xTaskNumber ); /*lint !e586 sprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
pcWriteBuffer += strlen( pcWriteBuffer ); /*lint !e9016 Pointer arithmetic ok on char pointers especially as in this case where it best denotes the intent of the code. */
#ifndef configTASK_WRITE_BUFFER_LENGTH
Copy link
Contributor

@jefftenney jefftenney Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce conditional code, configTASK_WRITE_BUFFER_LENGTH could have a default value of SIZE_MAX if the developer doesn't define it. Actually, it seems SIZE_MAX was introduced in C99, so that symbol would need to be defined conditionally first, e.g., ( (size_t) -1 ). This change would then eliminate all the calls to sprintf() as they would all be calls to snprintf().

As a (better?) alternative, if configUSE_STATS_FORMATTING_FUNCTIONS > 0 then we could generate a compile-time error if configTASK_WRITE_BUFFER_LENGTH is not defined. This is not backward compatible but perhaps is justified. Would need direction from the FreeRTOS team on this question. EDIT: See next comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the FreeRTOS team thinks this improvement warrants breaking backward compatibility for vTaskList() vTaskGetRunTimeStats(), then the simpler, direct fix is to add a second formal parameter, eg, bufferSize, to these two functions, and not to add configTASK_WRITE_BUFFER_LENGTH at all. I think most developers would appreciate being "forced" into this improvement during a FreeRTOS upgrade. Otherwise, they likely wouldn't benefit from this improvement during the upgrade (and likely wouldn't even know about it). These are peripheral functions to the kernel, thus not being backward compatible might be OK.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current thought is that we don't want to break backwards compatibility (demo code maybe using this function and we want to minimize the impact to that), so I think adding just a config for this is fine.

{
sprintf( pcWriteBuffer, "\t%c\t%u\t%u\t%u\r\n", cStatus, ( unsigned int ) pxTaskStatusArray[ x ].uxCurrentPriority, ( unsigned int ) pxTaskStatusArray[ x ].usStackHighWaterMark, ( unsigned int ) pxTaskStatusArray[ x ].xTaskNumber ); /*lint !e586 sprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
}
#else
{
snprintf( pcWriteBuffer, configTASK_WRITE_BUFFER_LENGTH, "\t%c\t%u\t%u\t%u\r\n", cStatus, ( unsigned int ) pxTaskStatusArray[ x ].uxCurrentPriority, ( unsigned int ) pxTaskStatusArray[ x ].usStackHighWaterMark, ( unsigned int ) pxTaskStatusArray[ x ].xTaskNumber ); /*lint !e586 snprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of configTASK_WRITE_BUFFER_LENGTH here should instead be a local size_t variable that is initialized to configTASK_WRITE_BUFFER_LENGTH, and it should shrink with each loop iteration wherever pcWriteBuffer is advanced. The two variables should stay in sync.

}
#endif
pcWriteBuffer += strlen( pcWriteBuffer ); /*lint !e9016 Pointer arithmetic ok on char pointers especially as in this case where it best denotes the intent of the code. */
}

/* Free the array again. NOTE! If configSUPPORT_DYNAMIC_ALLOCATION
Expand Down Expand Up @@ -6707,31 +6715,63 @@ static void prvResetNextTaskUnblockTime( void )
{
#ifdef portLU_PRINTF_SPECIFIER_REQUIRED
{
sprintf( pcWriteBuffer, "\t%lu\t\t%lu%%\r\n", pxTaskStatusArray[ x ].ulRunTimeCounter, ulStatsAsPercentage );
#ifndef configTASK_WRITE_BUFFER_LENGTH
{
sprintf( pcWriteBuffer, "\t%lu\t\t%lu%%\r\n", pxTaskStatusArray[ x ].ulRunTimeCounter, ulStatsAsPercentage );
}
#else
{
snprintf( pcWriteBuffer, configTASK_WRITE_BUFFER_LENGTH, "\t%lu\t\t%lu%%\r\n", pxTaskStatusArray[ x ].ulRunTimeCounter, ulStatsAsPercentage );
}
#endif
}
#else
#else /* ifdef portLU_PRINTF_SPECIFIER_REQUIRED */
{
/* sizeof( int ) == sizeof( long ) so a smaller
* printf() library can be used. */
sprintf( pcWriteBuffer, "\t%u\t\t%u%%\r\n", ( unsigned int ) pxTaskStatusArray[ x ].ulRunTimeCounter, ( unsigned int ) ulStatsAsPercentage ); /*lint !e586 sprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
#ifndef configTASK_WRITE_BUFFER_LENGTH
{
sprintf( pcWriteBuffer, "\t%u\t\t%u%%\r\n", ( unsigned int ) pxTaskStatusArray[ x ].ulRunTimeCounter, ( unsigned int ) ulStatsAsPercentage ); /*lint !e586 sprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
}
#else
{
snprintf( pcWriteBuffer, configTASK_WRITE_BUFFER_LENGTH, "\t%u\t\t%u%%\r\n", ( unsigned int ) pxTaskStatusArray[ x ].ulRunTimeCounter, ( unsigned int ) ulStatsAsPercentage ); /*lint !e586 snprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
}
#endif
}
#endif
#endif /* ifdef portLU_PRINTF_SPECIFIER_REQUIRED */
}
else
{
/* If the percentage is zero here then the task has
* consumed less than 1% of the total run time. */
#ifdef portLU_PRINTF_SPECIFIER_REQUIRED
{
sprintf( pcWriteBuffer, "\t%lu\t\t<1%%\r\n", pxTaskStatusArray[ x ].ulRunTimeCounter );
#ifndef configTASK_WRITE_BUFFER_LENGTH
{
sprintf( pcWriteBuffer, "\t%lu\t\t<1%%\r\n", pxTaskStatusArray[ x ].ulRunTimeCounter );
}
#else
{
snprintf( pcWriteBuffer, configTASK_WRITE_BUFFER_LENGTH, "\t%lu\t\t<1%%\r\n", pxTaskStatusArray[ x ].ulRunTimeCounter );
}
#endif
}
#else
#else /* ifdef portLU_PRINTF_SPECIFIER_REQUIRED */
{
/* sizeof( int ) == sizeof( long ) so a smaller
* printf() library can be used. */
sprintf( pcWriteBuffer, "\t%u\t\t<1%%\r\n", ( unsigned int ) pxTaskStatusArray[ x ].ulRunTimeCounter ); /*lint !e586 sprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
#ifndef configTASK_WRITE_BUFFER_LENGTH
{
sprintf( pcWriteBuffer, "\t%u\t\t<1%%\r\n", ( unsigned int ) pxTaskStatusArray[ x ].ulRunTimeCounter ); /*lint !e586 sprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
}
#else
{
snprintf( pcWriteBuffer, configTASK_WRITE_BUFFER_LENGTH, "\t%u\t\t<1%%\r\n", ( unsigned int ) pxTaskStatusArray[ x ].ulRunTimeCounter ); /*lint !e586 snprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
}
#endif
}
#endif
#endif /* ifdef portLU_PRINTF_SPECIFIER_REQUIRED */
}

pcWriteBuffer += strlen( pcWriteBuffer ); /*lint !e9016 Pointer arithmetic ok on char pointers especially as in this case where it best denotes the intent of the code. */
Expand Down