Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

initialize variables in wlr_surface.c #2736

Closed
wants to merge 7 commits into from

Conversation

ppascher
Copy link

@ppascher ppascher commented Feb 9, 2021

This fixes the following warnings/errors with -Werror set (default):

ninja: Entering directory `build'
[26/47] Compiling C object libwlroots.so.7.p/types_wlr_surface.c.o
FAILED: libwlroots.so.7.p/types_wlr_surface.c.o
cc -Ilibwlroots.so.7.p -I. -I.. -Iinclude -I../include -Iprotocol -I../protocol -I/usr/include/libdrm -I/usr/include/pixman-1 -I/usr/include/uuid -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 -O3 -DWLR_USE_UNSTABLE -Wundef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wstrict-prototypes -Wimplicit-fallthrough=2 -Wendif-labels -Wstrict-aliasing=2 -Woverflow -Wmissing-prototypes -Wno-missing-braces -Wno-missing-field-initializers -Wno-unused-parameter -fmacro-prefix-map=../= -DHAS_LIBUUID=1 '-DICONDIR="/usr/share/icons"' -fPIC -MD -MQ libwlroots.so.7.p/types_wlr_surface.c.o -MF libwlroots.so.7.p/types_wlr_surface.c.o.d -o libwlroots.so.7.p/types_wlr_surface.c.o -c ../types/wlr_surface.c
../types/wlr_surface.c: In function ‘surface_commit_pending’:
../types/wlr_surface.c:235:55: error: ‘src_width’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  235 |    float scale_x = (float)pending->viewport.dst_width / src_width;
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../types/wlr_surface.c:233:8: note: ‘src_width’ was declared here
  233 |    int src_width, src_height;
      |        ^~~~~~~~~
../types/wlr_surface.c:236:56: error: ‘src_height’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  236 |    float scale_y = (float)pending->viewport.dst_height / src_height;
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../types/wlr_surface.c:233:19: note: ‘src_height’ was declared here
  233 |    int src_width, src_height;
      |                   ^~~~~~~~~~
../types/wlr_surface.c: In function ‘wlr_surface_get_effective_damage’:
../types/wlr_surface.c:1293:62: error: ‘src_width’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1293 |   float scale_x = (float)surface->current.viewport.dst_width / src_width;
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../types/wlr_surface.c:1294:63: error: ‘src_height’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1294 |   float scale_y = (float)surface->current.viewport.dst_height / src_height;
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
cc1: all warnings being treated as errors
ninja: build stopped: subcommand failed.

This fixes the following warnings/errors with -Werror set (default):
```ninja -C build
ninja: Entering directory `build'
[26/47] Compiling C object libwlroots.so.7.p/types_wlr_surface.c.o
FAILED: libwlroots.so.7.p/types_wlr_surface.c.o
cc -Ilibwlroots.so.7.p -I. -I.. -Iinclude -I../include -Iprotocol -I../protocol -I/usr/include/libdrm -I/usr/include/pixman-1 -I/usr/include/uuid -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 -O3 -DWLR_USE_UNSTABLE -Wundef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wstrict-prototypes -Wimplicit-fallthrough=2 -Wendif-labels -Wstrict-aliasing=2 -Woverflow -Wmissing-prototypes -Wno-missing-braces -Wno-missing-field-initializers -Wno-unused-parameter -fmacro-prefix-map=../= -DHAS_LIBUUID=1 '-DICONDIR="/usr/share/icons"' -fPIC -MD -MQ libwlroots.so.7.p/types_wlr_surface.c.o -MF libwlroots.so.7.p/types_wlr_surface.c.o.d -o libwlroots.so.7.p/types_wlr_surface.c.o -c ../types/wlr_surface.c
../types/wlr_surface.c: In function ‘surface_commit_pending’:
../types/wlr_surface.c:235:55: error: ‘src_width’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  235 |    float scale_x = (float)pending->viewport.dst_width / src_width;
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../types/wlr_surface.c:233:8: note: ‘src_width’ was declared here
  233 |    int src_width, src_height;
      |        ^~~~~~~~~
../types/wlr_surface.c:236:56: error: ‘src_height’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  236 |    float scale_y = (float)pending->viewport.dst_height / src_height;
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
../types/wlr_surface.c:233:19: note: ‘src_height’ was declared here
  233 |    int src_width, src_height;
      |                   ^~~~~~~~~~
../types/wlr_surface.c: In function ‘wlr_surface_get_effective_damage’:
../types/wlr_surface.c:1293:62: error: ‘src_width’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1293 |   float scale_x = (float)surface->current.viewport.dst_width / src_width;
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
../types/wlr_surface.c:1294:63: error: ‘src_height’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1294 |   float scale_y = (float)surface->current.viewport.dst_height / src_height;
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
cc1: all warnings being treated as errors
ninja: build stopped: subcommand failed.
```
@ppascher
Copy link
Author

ppascher commented Feb 9, 2021

Maybe it is not a good idea to initialize to 0?

@emersion
Copy link
Member

emersion commented Feb 9, 2021

Hm, this will just cause a division by zero in case the buffer size is not divisible by the scale.

I wonder if we should keep the scale check in surface_state_viewport_src_size. Maybe we should move it out of the function into surface_state_finalize.

prevent pontential division by 0.
@ppascher
Copy link
Author

ppascher commented Feb 9, 2021

I wonder if we should keep the scale check in surface_state_viewport_src_size. Maybe we should move it out of the function into surface_state_finalize.

This would have to be answered by someone else I am afraid as I do not know how it all fits together.

@emersion emersion mentioned this pull request Feb 12, 2021
@gnzlbg
Copy link

gnzlbg commented Feb 12, 2021

This is preventing wlroots-git from building in the AUR.

FWIW I think the warning is just wrong here, initializing these variables is pointless, and one should disable the warning, e.g., by using the proper pragma to turn it off locally here.

Because of warnings with false-positives / incorrect diagnostics, it makes little sense to use -Werror as a default for everybody.

I think -Werror should be optional, and instead CI should always turn it on to catch these things.

@gnzlbg
Copy link

gnzlbg commented Feb 12, 2021

If you want to initialize this variables to something, initialize them to some invalid value, e.g., 0 or -1, and after calling the API, check that this value has changed.

types/wlr_surface.c Outdated Show resolved Hide resolved
types/wlr_surface.c Show resolved Hide resolved
types/wlr_surface.c Outdated Show resolved Hide resolved
types/wlr_surface.c Show resolved Hide resolved
@emersion
Copy link
Member

I think -Werror should be optional, and instead CI should always turn it on to catch these things.

NACK. See https://github.com/swaywm/wlroots/wiki/Packaging-recommendations.

ppascher and others added 5 commits February 12, 2021 14:48
Co-authored-by: gnzlbg <gnzlbg@users.noreply.github.com>
Co-authored-by: gnzlbg <gnzlbg@users.noreply.github.com>
Co-authored-by: gnzlbg <gnzlbg@users.noreply.github.com>
Co-authored-by: gnzlbg <gnzlbg@users.noreply.github.com>
emersion added a commit to emersion/wlroots that referenced this pull request Feb 15, 2021
This fixes some build warnings.

Closes: swaywm#2740
References: swaywm#2736
@emersion
Copy link
Member

See the first commit in #2742.

@ppascher
Copy link
Author

fixed with #2742

@ppascher ppascher closed this Feb 16, 2021
emersion added a commit that referenced this pull request Feb 17, 2021
This fixes some build warnings.

Closes: #2740
References: #2736
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants