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

Fix warnings on 32-bit platforms #159

Merged
merged 1 commit into from
Jul 17, 2024
Merged

Conversation

voidbert
Copy link
Contributor

Fixed code that assumed the size of some integer types to be a fixed number of bytes, when it can very between platforms. The warnings fixed were:

  • Wrong printf formats for size_t. Now, the platform-independent %zu (C99 required);
  • time_t may be less than 64 bits, requiring a cast to a bigger size if a left shift by 32 bits is used.
  • POS_FROM_PARENT may assume a value too large for ssize_t.

I didn't test if these warnings caused any runtime errors, but they can do so, so it's better to have them fixed.

@@ -299,7 +299,8 @@ bool config_to_color(const char* text, argb_t* color)
++text;
}

if (!str_to_num(text, 0, &num, 16) || num < 0 || num > 0xffffffff) {
if (!str_to_num(text, 0, &num, 16) || num < 0 ||
(uint64_t)num > (uint64_t)0xffffffff) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do we really need these type casts?
0xffffffff fits uint32_t. Also color is always 32 bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 32-bit systems, 0xffffffff will be considered a 4-byte unsigned int by compilers, as it can't fit in an int without overflowing. Also, ssize_t num will be a 4-byte int (not guaranteed by the C spec, but what compilers usually do). The compiler will emit a warning because these two different types are being compared, and the compiler doesn't know whether it should generate a signed or unsigned comparison. The cast to uint64_t assures that these two values can be safely compared on both 32-bit and 64-bit systems. On 64-bit systems, because this code checks for the validity of a color, num can be greater than 32-bits (it's inputted by the user in the configuration file). A cast of num to uint32_t wouldn't suffice, as values higher than 0xffffffff would be accepted as valid colors, as they'd be truncated before the comparison.

@@ -157,8 +157,8 @@ static struct wl_buffer* create_buffer(void)

// generate unique file name
clock_gettime(CLOCK_MONOTONIC, &ts);
snprintf(path, sizeof(path), "/" APP_NAME "_%lx",
(ts.tv_sec << 32) | ts.tv_nsec);
snprintf(path, sizeof(path), "/" APP_NAME "_%" PRIx64,
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to use one style for printf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I don't know what you mean by this. PRIx64 is the best way to print an uint64_t independently of the size unsigned long ("%lx") or unsigned long long ("%llx") might have. Another option would be casting the value in the arguments to an unsigned long long, and %llx can be used instead of PRIx64 . I can patch that in if you wish.

@@ -15,7 +17,7 @@
// Special ids for windows size and position
#define SIZE_FROM_IMAGE 0
#define SIZE_FROM_PARENT 1
#define POS_FROM_PARENT 0xffffffff
#define POS_FROM_PARENT SSIZE_MAX
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see a reason to change this. What the purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0xffffffff may be too large for an ssize_t, and some functions (ui_get_x and ui_get_y) return this value as an ssize_t. I see that POS_FROM_PARENT is a special return value: normal positions can be returned, so can this very large value, which has a special meaning. This way, SSIZE_MAX is for sure the greatest value that can fit in an ssize_t.

@voidbert
Copy link
Contributor Author

I hereby attach the warnings that resulted from compiling swayimg on GCC 13.2.0, on a 32-bit glibc system.

warnings.txt
ninja: Entering directory `/root/swayimg/build'
[1/34] Generating xdg-shell-protocol.c with a custom command
[2/34] Generating xdg-shell-protocol.h with a custom command
[3/34] Compiling C object swayimg.p/meson-generated_.._xdg-shell-protocol.c.o
[4/34] Compiling C object swayimg.p/src_action.c.o
[5/34] Compiling C object swayimg.p/src_image.c.o
[6/34] Compiling C object swayimg.p/src_cache.c.o
[7/34] Compiling C object swayimg.p/src_config.c.o
../src/config.c: In function 'load_config':
../src/config.c:122:65: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  122 |                 fprintf(stderr, "Invalid section define in %s:%lu\n", path,
      |                                                               ~~^
      |                                                                 |
      |                                                                 long unsigned int
      |                                                               %u
  123 |                         line_num);
      |                         ~~~~~~~~                                 
      |                         |
      |                         size_t {aka unsigned int}
../src/config.c:135:63: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  135 |             fprintf(stderr, "Invalid key=value format in %s:%lu\n", path,
      |                                                             ~~^
      |                                                               |
      |                                                               long unsigned int
      |                                                             %u
  136 |                     line_num);
      |                     ~~~~~~~~                                   
      |                     |
      |                     size_t {aka unsigned int}
../src/config.c:154:60: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  154 |             fprintf(stderr, "Invalid configuration in %s:%lu\n", path,
      |                                                          ~~^
      |                                                            |
      |                                                            long unsigned int
      |                                                          %u
  155 |                     line_num);
      |                     ~~~~~~~~                                
      |                     |
      |                     size_t {aka unsigned int}
../src/config.c: In function 'config_to_color':
../src/config.c:302:58: warning: comparison of integer expressions of different signedness: 'ssize_t' {aka 'int'} and 'unsigned int' [-Wsign-compare]
  302 |     if (!str_to_num(text, 0, &num, 16) || num < 0 || num > 0xffffffff) {
      |                                                          ^
[8/34] Compiling C object swayimg.p/src_font.c.o
[9/34] Compiling C object swayimg.p/src_keybind.c.o
[10/34] Compiling C object swayimg.p/src_main.c.o
../src/main.c: In function 'sway_setup':
../src/main.c:170:27: warning: comparison of integer expressions of different signedness: 'ssize_t' {aka 'int'} and 'unsigned int' [-Wsign-compare]
  170 |     absolute = ui_get_x() != POS_FROM_PARENT && ui_get_y() != POS_FROM_PARENT;
      |                           ^~
../src/main.c:170:60: warning: comparison of integer expressions of different signedness: 'ssize_t' {aka 'int'} and 'unsigned int' [-Wsign-compare]
  170 |     absolute = ui_get_x() != POS_FROM_PARENT && ui_get_y() != POS_FROM_PARENT;
      |                                                            ^~
[11/34] Compiling C object swayimg.p/src_imagelist.c.o
[12/34] Compiling C object swayimg.p/src_info.c.o
../src/info.c: In function 'info_update':
../src/info.c:400:45: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  400 |         snprintf(buffer, sizeof(buffer), "%lu of %lu", ctx.frame + 1,
      |                                           ~~^          ~~~~~~~~~~~~~
      |                                             |                    |
      |                                             long unsigned int    size_t {aka unsigned int}
      |                                           %u
../src/info.c:400:52: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  400 |         snprintf(buffer, sizeof(buffer), "%lu of %lu", ctx.frame + 1,
      |                                                  ~~^
      |                                                    |
      |                                                    long unsigned int
      |                                                  %u
  401 |                  ctx.frame_total);
      |                  ~~~~~~~~~~~~~~~                    
      |                     |
      |                     size_t {aka unsigned int}
../src/info.c:407:45: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  407 |         snprintf(buffer, sizeof(buffer), "%lu of %lu", ctx.index + 1,
      |                                           ~~^          ~~~~~~~~~~~~~
      |                                             |                    |
      |                                             long unsigned int    size_t {aka unsigned int}
      |                                           %u
../src/info.c:407:52: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  407 |         snprintf(buffer, sizeof(buffer), "%lu of %lu", ctx.index + 1,
      |                                                  ~~^
      |                                                    |
      |                                                    long unsigned int
      |                                                  %u
  408 |                  image_list_size());
      |                  ~~~~~~~~~~~~~~~~~                  
      |                  |
      |                  size_t {aka unsigned int}
../src/info.c:416:49: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  416 |             snprintf(buffer, sizeof(buffer), "%ld%%", ctx.scale);
      |                                               ~~^     ~~~~~~~~~
      |                                                 |        |
      |                                                 long int size_t {aka unsigned int}
      |                                               %d
../src/info.c:426:49: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  426 |             snprintf(buffer, sizeof(buffer), "%lux%lu", ctx.width, ctx.height);
      |                                               ~~^       ~~~~~~~~~
      |                                                 |          |
      |                                                 |          size_t {aka unsigned int}
      |                                                 long unsigned int
      |                                               %u
../src/info.c:426:53: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
  426 |             snprintf(buffer, sizeof(buffer), "%lux%lu", ctx.width, ctx.height);
      |                                                   ~~^              ~~~~~~~~~~
      |                                                     |                 |
      |                                                     long unsigned int size_t {aka unsigned int}
      |                                                   %u
[13/34] Compiling C object swayimg.p/src_text.c.o
[14/34] Compiling C object swayimg.p/src_str.c.o
[15/34] Compiling C object swayimg.p/src_loader.c.o
[16/34] Compiling C object swayimg.p/src_sway.c.o
[17/34] Compiling C object swayimg.p/src_formats_pnm.c.o
[18/34] Compiling C object swayimg.p/src_ui.c.o
../src/ui.c: In function 'create_buffer':
../src/ui.c:161:25: warning: left shift count >= width of type [-Wshift-count-overflow]
  161 |              (ts.tv_sec << 32) | ts.tv_nsec);
      |                         ^~
../src/ui.c: In function 'ui_create':
../src/ui.c:701:47: warning: left shift count >= width of type [-Wshift-count-overflow]
  701 |         const uint64_t timestamp = (ts.tv_sec << 32) | ts.tv_nsec;
      |                                               ^~
In file included from ../src/ui.c:7:
./buildcfg.h:8:18: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'uint64_t' {aka 'long long unsigned int'} [-Wformat=]
    8 | #define APP_NAME "swayimg"
      |                  ^~~~~~~~~
../src/ui.c:702:42: note: in expansion of macro 'APP_NAME'
  702 |         snprintf(app_id, sizeof(app_id), APP_NAME "_%lx", timestamp);
      |                                          ^~~~~~~~
../src/ui.c:702:55: note: format string is defined here
  702 |         snprintf(app_id, sizeof(app_id), APP_NAME "_%lx", timestamp);
      |                                                     ~~^
      |                                                       |
      |                                                       long unsigned int
      |                                                     %llx
[19/34] Compiling C object swayimg.p/src_formats_bmp.c.o
[20/34] Compiling C object swayimg.p/src_formats_tga.c.o
[21/34] Compiling C object swayimg.p/src_pixmap.c.o
[22/34] Compiling C object swayimg.p/src_exif.c.o
[23/34] Compiling C object swayimg.p/src_viewer.c.o
[24/34] Compiling C object swayimg.p/src_formats_avif.c.o
[25/34] Compiling C object swayimg.p/src_formats_jpeg.c.o
[26/34] Compiling C object swayimg.p/src_formats_heif.c.o
[27/34] Compiling C object swayimg.p/src_formats_jxl.c.o
[28/34] Compiling C object swayimg.p/src_formats_gif.c.o
[29/34] Compiling C object swayimg.p/src_formats_png.c.o
[30/34] Compiling C object swayimg.p/src_formats_webp.c.o
[31/34] Compiling C object swayimg.p/src_formats_exr.c.o
[32/34] Compiling C object swayimg.p/src_formats_tiff.c.o
[33/34] Compiling C object swayimg.p/src_formats_svg.c.o
[34/34] Linking target swayimg
INFO: autodetecting backend as ninja
INFO: calculating backend command to run: /bin/ninja -C /root/swayimg/build

@voidbert voidbert requested a review from artemsen July 17, 2024 15:23
@artemsen artemsen merged commit 8784b29 into artemsen:master Jul 17, 2024
4 checks passed
@artemsen
Copy link
Owner

Sounds reasonable, thank you!

@voidbert voidbert deleted the 32bits branch July 17, 2024 16:10
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