Fixes found with -ffreestanding disabled #45179
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
keith-packard
requested review from
MaureenHelm,
andyross,
avisconti,
bbolen,
carlescufi,
dcpleung,
de-nordic,
galak,
ioannisg,
kgugala,
mniestroj,
nashif,
nordic-krch,
pfalcon,
pgielda,
rlubos,
stephanosio,
tbursztyka and
tejlmand
as code owners
github-actions
bot
added
area: API
andyross
previously approved these changes
May 12, 2022
Contributor
andyross
left a comment
andyross
left a comment
There was a problem hiding this comment.
Scanned all the code I recognize well enough to review and all looks great. This +1 will be wiped out with the next rev of course, but putting it here anyway so there's a historical record.
keith-packard
force-pushed
the
compiler-hosted-mode
branch
3 times, most recently
from
b12c0c9 to
ff1b506
Compare
keith-packard
force-pushed
the
compiler-hosted-mode
branch
from
ff1b506 to
0183709
Compare
github-actions
bot
added
the
area: Tests
gcc in 'hosted' mode checks the implementation of 'exit' to make sure it doesn't return. Signed-off-by: Keith Packard <[email protected]>
This driver carefully saved the errno value from the failing call and then didn't use it in the ERROR report, using the potentially invalid current errno value (which may have been set by the close call). Signed-off-by: Keith Packard <[email protected]>
Sensor value computation was creating a 64-bit integer value, passing that to 'abs' and assigning that to an int32_t result and then sanity checking the result. If the computation goes badly wrong, then the range reduction of 64-bit to 32-bit values could generate a falsely in-range value. Instead, perform the computation in 64-bits, range check that value and then assign to a 32-bit variable. Signed-off-by: Keith Packard <[email protected]>
Not sure why this is needed for this branch, but it pretty clearly is -- there are hundreds of set-but-unused variables in the codebase. Signed-off-by: Keith Packard <[email protected]>
idle_rate is uint8_t, sof_cnt is uint32_t. The result is uint32_t, which is the wrong type for 'abs'. Explicitly cast idle_rate to uint32_t, subtract sof_cnt and then explicitly cast to int32_t and then use abs, storing the result in another int32_t which matches the return type for abs. This quiets clang warnings about passing unsigned values to abs. Signed-off-by: Keith Packard <[email protected]>
POSIX main is supposed to return int, and there's no reason not to have this example do that. Signed-off-by: Keith Packard <[email protected]>
Subtracting with a uint64_t operand yields a uint64_t result, for which the absolute value is not terribly interesting. Cast the operand to int64_t. Use llabs instead of abs as abs takes an int parameter and not an int64_t. This appears to work even with the minimal C library. Signed-off-by: Keith Packard <[email protected]>
When the compiler is allowed to know the semantics of malloc and free, this test needs to be careful to not have both calls elided by not using the result at all. Simply storing the malloc pointer in a global variable is sufficient here. Signed-off-by: Keith Packard <[email protected]>
keith-packard
force-pushed
the
compiler-hosted-mode
branch
from
0183709 to
1345886
Compare
When defined as 'char', the compiler notices that the memcpy targeting that address will write more than one byte which generates a warning. Use an array instead so that the compiler doesn't assume a specific size. Signed-off-by: Keith Packard <[email protected]>
keith-packard
force-pushed
the
compiler-hosted-mode
branch
from
1345886 to
cd5338f
Compare
Contributor
Author
|
I rebased this back to main so it can be reviewed independently of the picolibc merge. |
nvlsianpu
approved these changes
Jun 13, 2022
Contributor
nvlsianpu
left a comment
nvlsianpu
left a comment
There was a problem hiding this comment.
+1 to the flash driver
dcpleung
approved these changes
Jun 13, 2022
stephanosio
approved these changes
Jun 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here's a collection of fixes to drivers, subsys, tests and samples to resolve compiler warnings and errors found when -ffreestanding is taken out of the compiler command line, leaving it able to detect problems when using standard C library functions.
I think there are a couple of real bugs found here, although most of the changes relate to buffers sized for numbers not expected to span the full range of their declared type.
This series is no longer stacked on top of the picolibc-module series so printk/cbprintf errors will not be checked until those patches are also merged.
Closes: #45157
Depends on #44096 (includes commits from #44096)