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.

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

Warning Fix: Current code produces warnings for possible value overflows. #6665

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

bevanweiss
Copy link
Contributor

As the input values are uint8_t types, any shift may result in value loss.
Explicit promotion to the output type (uint32_t) keeps things safe.

Have also changed the int32_t in ads1220_read_adc to uint32_t, type promotion and bit manipulation are a bit 'weird' on signed integers, so keep it as an unsigned to align with following function call parameter type.
Have retained the prior explicit sign extension logic however.

@KevinOConnor
Copy link
Collaborator

Thanks. The change looks fine, but commits need to have a signed-off-by line - see https://www.klipper3d.org/CONTRIBUTING.html .

Also, fwiw, the issue isn't with uint8_t as C will implicitly convert most operations to an int type. The problem is that int is 16-bit on AVR, which can cause overflows (and compile warnings) on that architecture.

Cheers,
-Kevin

@bevanweiss
Copy link
Contributor Author

Thanks. The change looks fine, but commits need to have a signed-off-by line - see https://www.klipper3d.org/CONTRIBUTING.html .

Was there a different issue?
the commit definitely had a signed-off line (I've done a great job spelling my last name wrong of course...)
image

@KevinOConnor
Copy link
Collaborator

Sorry, I missed that the signed-off-by line was in fact present in the commit message.

Just for completeness, can you provide the signed-off-by line with the correct spelling?

-Kevin

As the input values are uint8_t types, any shift may result in value loss.
Explicit promotion to the output type (uint32_t) keeps things safe.
Have also changed the int32_t in ads1220_read_adc to uint32_t, type
promotion and bit manipulation are a bit 'weird' on signed integers, so
keep it as an unsigned to align with following function call parameter type.
Have retained the prior explicit sign extension logic however.


Signed-off-by: Bevan Weiss <[email protected]>
@bevanweiss
Copy link
Contributor Author

Just for completeness, can you provide the signed-off-by line with the correct spelling?

-Kevin

Done, thanks

@KevinOConnor KevinOConnor merged commit c0edfbc into Klipper3d:master Aug 15, 2024
1 check passed
@KevinOConnor
Copy link
Collaborator

Thanks.

-Kevin

@bevanweiss bevanweiss deleted the fix_shift_warnings branch August 15, 2024 06:37
tobyfu pushed a commit to tobyfu/klipper that referenced this pull request Sep 11, 2024
…ipper3d#6665)

As the input values are uint8_t types, any shift may result in value loss.
Explicit promotion to the output type (uint32_t) keeps things safe.
Have also changed the int32_t in ads1220_read_adc to uint32_t, type
promotion and bit manipulation are a bit 'weird' on signed integers, so
keep it as an unsigned to align with following function call parameter type.
Have retained the prior explicit sign extension logic however.

Signed-off-by: Bevan Weiss <[email protected]>
miklschmidt pushed a commit to Rat-OS/klipper that referenced this pull request Oct 16, 2024
…ipper3d#6665)

As the input values are uint8_t types, any shift may result in value loss.
Explicit promotion to the output type (uint32_t) keeps things safe.
Have also changed the int32_t in ads1220_read_adc to uint32_t, type
promotion and bit manipulation are a bit 'weird' on signed integers, so
keep it as an unsigned to align with following function call parameter type.
Have retained the prior explicit sign extension logic however.

Signed-off-by: Bevan Weiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants