Skip to content

Fix Steam input "crc" errors, and some other Coverity reports of uninitialized scalar variable #91454

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
Merged
merged 1 commit into from
Jun 3, 2024
Merged

Fix Steam input "crc" errors, and some other Coverity reports of uninitialized scalar variable #91454

merged 1 commit into from
Jun 3, 2024

Conversation

@akien-mga
Copy link
Member

@akien-mga akien-mga commented May 2, 2024 •
edited
Loading

Going through a few reports on https://scan.coverity.com/projects/godotengine-godot?tab=overview

Lots of low hanging fruits there to improve the code quality and find potential bugs.

The input mapping change fixes these issues:

@akien-mga akien-mga added this to the 4.3 milestone May 2, 2024
@akien-mga akien-mga requested review from a team as code owners May 2, 2024 11:13
akien-mga
akien-mga commented May 2, 2024
View reviewed changes
drivers/gles3/storage/light_storage.h Outdated
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

Fixes this:
image

CC @clayjohn @BastiaanOlij

There's a bunch more issues with uninitialized stuff in rendering code, would be good for the rendering team to make a pass on the reports and decide whether to init all the structs properly, or what other approach to take. We've had a number of bugs on some platforms/GPUs due to uninitialized struct members.

drivers/gles3/rasterizer_gles3.cpp Outdated
Comment on lines +152 to +153
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

These fix these errors:
image

CC @lawnjelly

Copy link
Member

@lawnjelly lawnjelly May 4, 2024

Choose a reason for hiding this comment

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

Although I'm in the blame this is because I'm registered as initial author for 4.x, but this was copy pasted from 3.x GLES2, which is karrofel it looks like.

But fix looks fine to me. 👍

drivers/gles3/rasterizer_gles3.cpp Outdated
Comment on lines -165 to -168
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

Removed as the early return makes it dead code.

modules/gdscript/language_server/godot_lsp.h Outdated
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

Fixes this:
image

But this is actually never set anywhere, so the code where it was being used is bogus.

It was added in #80973, CC @ryanabx.

scene/resources/2d/tile_set.h Outdated
Comment on lines +268 to +269
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

Fixes this:
image

CC @groud

core/input/input.cpp Outdated
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

This fixes what I reported here: #73224 (comment)

image

CC @rsubtil

Copy link
Member

@rsubtil rsubtil May 4, 2024

Choose a reason for hiding this comment

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

Yes, it makes sense, I didn't notice that detail when changing the error message to a warning 👍

Copy link
Member Author

@akien-mga akien-mga May 31, 2024

Choose a reason for hiding this comment

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

I believe this should also fix these issues:

drivers/gles3/rasterizer_canvas_gles3.h Outdated
Comment on lines 111 to 112
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

image

CC @clayjohn @BastiaanOlij

Copy link
Member

@clayjohn clayjohn May 2, 2024

Choose a reason for hiding this comment

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

I'm not sure about these. To me it seems totally pointless to initialize certain variables when we don't know if they will be used. The trouble is that we assign an RID before configuring the light. So the Coverity scan doesn't have enough information here to be really useful.

Copy link
Member Author

@akien-mga akien-mga May 31, 2024

Choose a reason for hiding this comment

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

I removed these changes from the PR for now.

thirdparty/enet/enet/godot.h Outdated
Comment on lines 75 to 77
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

image

CC @Faless

Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

This is C code so my fix was invalid, would need a different approach. Removed from this PR for now.

editor/debugger/debug_adapter/debug_adapter_types.h Outdated
Copy link
Member Author

@akien-mga akien-mga May 2, 2024

Choose a reason for hiding this comment

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

Fixes this type of errors:
image

CC @rsubtil

Copy link
Member

@rsubtil rsubtil May 4, 2024

Choose a reason for hiding this comment

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

Good catch, expensive ended up never being set in the code, so this was actually causing undefined behavior.

I can confirm it should be false as AFAIK Godot still doesn't support partial/pagination behavior to requesting variables from stack dumps, so there's no point in hinting such behavior to DAP when Godot will already have all that information.

@akien-mga akien-mga mentioned this pull request May 2, 2024
Merged
rsubtil
rsubtil reviewed May 4, 2024
View reviewed changes
editor/debugger/debug_adapter/debug_adapter_types.h Outdated
Copy link
Member

@rsubtil rsubtil May 4, 2024

Choose a reason for hiding this comment

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

There goes my little easter egg 😅

Copy link
Member Author

@akien-mga akien-mga May 31, 2024

Choose a reason for hiding this comment

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

Brought it back ;)

@akien-mga akien-mga modified the milestones: 4.3, 4.4 May 29, 2024
@akien-mga akien-mga modified the milestones: 4.4, 4.3 May 31, 2024
@akien-mga akien-mga changed the title Fix various Coverity reports of uninitialized scalar variable Fix various Coverity reports of uninitialized scalar variable, fix Steam input CRC errors May 31, 2024
@akien-mga akien-mga changed the title Fix various Coverity reports of uninitialized scalar variable, fix Steam input CRC errors Fix Steam input "crc" errors, and some other Coverity reports of uninitialized scalar variable May 31, 2024
@akien-mga akien-mga added topic:input cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:codestyle cherrypick:4.1 and removed topic:codestyle labels May 31, 2024
@akien-mga akien-mga merged commit 41e762c into godotengine:master Jun 3, 2024
@akien-mga akien-mga deleted the coverity-checks branch June 3, 2024 08:59
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rsubtil rsubtil rsubtil left review comments

@clayjohn clayjohn clayjohn left review comments

@lawnjelly lawnjelly lawnjelly left review comments

Assignees

No one assigned

Labels

bug enhancement topic:codestyle topic:input

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

Parse_mapping error on Steam version only [Steam] [Linux] XBox Series X controller shows CRC errors

4 participants

@akien-mga @rsubtil @clayjohn @lawnjelly