Minor warnings during compilation (GCC 6.3.0 on standard Raspbian Stretch)

hhromic commented 5 years ago

Description

Summary: Minor warnings during compilation (GCC 6.3.0 on standard Raspbian Stretch).

The build goes fine and runs very well however there ~~are couple of warnings~~ is a warning in ~~three files~~ one file. All the rest compiles absolutely cleanly. This is not critical nor priority, just perfectionism.

The first two warnings are for the same problem: using %lu format to print a size_t type that is not necessarily a long unsigned int. This could be fixed portably (C99) by using the %zu modifier as suggested here. [fix merged]

The ~~third~~ warning is because gl2_renderchain_init_pbo is always defined but populated conditionally using compiler flags. Then it's usage is conditioned. Therefore when these flags are not active, the function is left defined but not used as the warning indicates correctly. This doesn't happen with other similar functions in the same file because their calls aren't conditioned. A possible solution could be to move the definition inside the pre-processor conditional, however it will break the code style compared to the other functions.

For now I didn't want to make a PR because this sort of changes require senior developer reviewing. If you prefer I can also send a PR and further review/discuss there.

gfx/drivers/gl.c

gfx/drivers/gl.c:1613:13: warning: ‘gl2_renderchain_init_pbo’ defined but not used [-Wunused-function]
static void gl2_renderchain_init_pbo(unsigned size,
         ^~~~~~~~~~~~~~~~~~~~~~~~

Expected behavior

Compilation of RetroArch without any warnings from the compiler or linker.

Actual behavior

Getting 3 minor warnings during compilation.

Steps to reproduce the bug

Compile the source using GCC 6.3.0 and observe the output log

Version/Commit

You can find this information under Information/System Information

RetroArch: v1.7.6 (stable/tag)

Environment information

OS: Raspbian Stretch
Compiler: GCC 6.3.0 Note: this should affect any gcc compiler in any OS.

Sunderland93 commented 5 years ago

could you confirm this?

Yes

Sunderland93 commented 5 years ago

I don't have a problem with this as a fallback, but I still think it would be preferable to use the system version when available.

For now, wayland-protocols contains 21 protocol. RetroArch uses only 4. It is not necessary to depend on full set.

hhromic commented 5 years ago

so the idea would be to bundle parts of wayland-protocols into the code base and use those to generate the files?

Yes, in particular the gfx/common/wayland/generate_wayland_protos.sh script only needs:

unstable/xdg-shell/xdg-shell-unstable-v6.xml
stable/xdg-shell/xdg-shell.xml
unstable/idle-inhibit/idle-inhibit-unstable-v1.xml
unstable/xdg-decoration/xdg-decoration-unstable-v1.xml

These are in the official repository: https://github.com/wayland-project/wayland-protocols If more spec files are required in the future, they can be safely added. This way RetroArch always uses well-known spec files, even if they ever break in upstream (not gonna happen but is an added benefit).

The wayland-protocols package doesn't add anything else to the system (besides the pkgconfig configuration), see for example the filelist of the Debian 10 package, which is the same for all platforms: https://packages.debian.org/buster/all/wayland-protocols/filelist

it would be preferable to use the system version when available

If you want to stay safe and can be done easily (support system or bundled), sure, but I strongly think this is not required. The files from the upstream repository are exactly the same as they would be installed on any system, the are platform-independent.

Edit: please note that, again, I don't have any experience on wayland. I'm speaking based on software engineering practices and how I think wayland works based on reading the docs. A full confirmation by an experienced developer should def be taken in consideration over my words (@Sunderland93 ).

orbea commented 5 years ago

For now, wayland-protocols contains 21 protocol. RetroArch uses only 4. It is not necessary to depend on full set.

If you want to stay safe and can be done easily (support system or bundled), sure, but I strongly think this is not required. The files from the upstream repository are exactly the same as they would be installed on any system, the are platform-independent.

It shouldn't be too hard, I can try to take a look soon(ish).

While I agree the whole set it not needed, the idea is that distributions may trust their own packages more than bundled versions by developers they might not know or they may find it preferable to use a different version with bug fixes or even for the purpose of debugging. I think we can support both use cases.

hhromic commented 5 years ago

the idea is that distributions may trust their own packages more than bundled versions by developers they might not know or they may find it preferable to use a different version with bug fixes or even for the purpose of debugging

That is very true for software dependencies indeed, e.g. libraries. However the particular case of the wayland protocols package are descriptor files, not functional artifacts such as libraries or binaries, and they are highly likely provided always as-is from upstream.

I think we can support both use cases.

While we can, I still believe that it is easier to implement and maintain in the long run if we just bundle the files in this case. Also the build environment would be more stable (we always know which spec files are being used). Also don't forget that major libraries such as SDL2 are using the same approach :)

orbea commented 5 years ago

On the other hand lot of other programs seem to use the system version of wayland-protocols.

https://aur.archlinux.org/packages/wayland-protocols-git/

This branch should work.

https://github.com/orbea/RetroArch/tree/protocols

While we can, I still believe that it is easier to implement and maintain in the long run if we just bundle the files in this case. Also the build environment would be more stable (we always know which spec files are being used).

I honestly don't think this is a big concern, it should be very clear what is being used from the configure output.

hhromic commented 5 years ago

On the other hand lot of other programs seem to use the system version of wayland-protocols.

Well the package has a reason to exist too (make dep handling easier), I was mostly arguing that only bundling the protocols is not a bad thing to do either. But I guess the proposed dual approach is good too.

This branch should work.

Just tested on Debian 9 + Backports and works like a charm :) Nice shell scripting!

$ ./configure
(...)
Checking presence of package wayland-egl >= 10.1.0 ... 18.1.0
Checking presence of package wayland-cursor >= 1.12 ... 1.16.0
Checking presence of package wayland-protocols >= 1.15 ... no
Checking presence of package wayland-scanner >= 1.12 ... 1.16.0
Checking presence of package xkbcommon >= 0.3.2 ... 0.7.1
Checking presence of package xext ... 1.3.3
Checking presence of package xxf86vm ... 1.1.4
Notice: Using the bundled wayland-protocols.
(...)
$ make
(...)
CC gfx/common/wayland/xdg-shell.c
CC gfx/common/wayland/xdg-shell-unstable-v6.c
CC gfx/common/wayland/idle-inhibit-unstable-v1.c
CC gfx/common/wayland/xdg-decoration-unstable-v1.c
(...)
$ ./retroarch --features
(...)
  wayland:
                Wayland input/video drivers: yes
(...)

I honestly don't think this is a big concern, it should be very clear what is being used from the configure output.

Yes now that I see it implemented, I guess is fine and we shouldn't drown on a glass of water :) Great job and thanks! you just enabled building in Debian 9 + Backports (and probably vanilla too as it comes with Wayland 1.12 which should be supported).

orbea commented 5 years ago

I force pushed some cleanups to my branch and will make PR later today, thanks for testing!

orbea commented 5 years ago

I made a PR.

https://github.com/libretro/RetroArch/pull/8250

orbea commented 5 years ago

@hhromic Back on topic I found this warning with --enable-python and 32-bit linux.

gfx/drivers_tracker/video_state_python.c: In function 'py_state_new':
gfx/drivers_tracker/video_state_python.c:320:37: warning: passing argument 3 of 'filestream_read_file' from incompatible pointer type [-Wincompatible-pointer-types]
          (script, (void**)&script_, &len);
                                     ^
In file included from gfx/drivers_tracker/video_state_python.c:25:0:
./libretro-common/include/streams/file_stream.h:77:9: note: expected 'int64_t * {aka long long int *}' but argument is of type 'ssize_t * {aka int *}'
 int64_t filestream_read_file(const char *path, void **buf, int64_t *len);
         ^

So I came up with this patch to silence it, does this look right?

diff --git a/gfx/drivers_tracker/video_state_python.c b/gfx/drivers_tracker/video_state_python.c
index 8c95734771..bf65cf28c7 100644
--- a/gfx/drivers_tracker/video_state_python.c
+++ b/gfx/drivers_tracker/video_state_python.c
@@ -317,7 +317,7 @@ py_state_t *py_state_new(const char *script,
       ssize_t len;
       char *script_ = NULL;
       bool ret      = filestream_read_file
-         (script, (void**)&script_, &len);
+         (script, (void**)&script_, (int64_t*)&len);

       if (!ret || len < 0)
       {

hhromic commented 5 years ago

@orbea mmm I would rather change the type of len form ssize_t to the required int64_t in: https://github.com/libretro/RetroArch/blob/19c4fcda885f7627f55d43e9532254cfc230654c/gfx/drivers_tracker/video_state_python.c#L317

That len variable there doesn't seem to be used for anything else than to check for errors, so it's pretty harmless (and correct) to use the required type from the signature of filestream_read_file.

orbea commented 5 years ago

Thanks for the feedback, I wasn't sure if that or the above patch was a better idea.

hhromic commented 5 years ago

We should open an issue (or re-purpose this one) specifically for tracking and solving compilation warnings in the codebase. It's a fun activity for relaxing and one learns a lot from what the compiler has to say.

orbea commented 5 years ago

I don't really mind either way, like you said its a good learning experience and it makes repeated compiles more tolerable.

orbea commented 5 years ago

Warnings with --enable-new_cheevos.

In function ‘command_reply’,
    inlined from ‘command_read_ram’ at command.c:299:7:
command.c:159:10: warning: argument 1 null where non-null expected [-Wnonnull]
          fwrite(data, 1,len, stdout);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from command.c:18:
command.c: In function ‘command_read_ram’:
/usr/include/stdio.h:652:15: note: in a call to function ‘fwrite’ declared here
 extern size_t fwrite (const void *__restrict __ptr, size_t __size,
               ^~~~~~
In file included from command.c:22:
command.c:303:48: warning: argument 1 null where non-null expected [-Wnonnull]
       strlcpy(reply_at, " -1\n", sizeof(reply)-strlen(reply));
                                                ^~~~~~~~~~~~~
./libretro-common/include/compat/strl.h:46:59: note: in definition of macro ‘strlcpy’
 #define strlcpy(dst, src, size) strlcpy_retro__(dst, src, size)
                                                           ^~~~
In file included from command.c:19:
/usr/include/string.h:384:15: note: in a call to function ‘strlen’ declared here
 extern size_t strlen (const char *__s)
               ^~~~~~
In function ‘command_reply’,
    inlined from ‘command_read_ram’ at command.c:304:7:
command.c:159:10: warning: argument 1 null where non-null expected [-Wnonnull]
          fwrite(data, 1,len, stdout);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from command.c:18:
command.c: In function ‘command_read_ram’:
/usr/include/stdio.h:652:15: note: in a call to function ‘fwrite’ declared here
 extern size_t fwrite (const void *__restrict __ptr, size_t __size,
               ^~~~~~

cheevos-new/cheevos.c: In function ‘cheevos_award’:
cheevos-new/cheevos.c:510:45: warning: ‘%s’ directive output may be truncated writing up to 4095 bytes into a region of size 256 [-Wformat-truncation=]
       snprintf(shotname, sizeof(shotname), "%s/%s-cheevo-%u",
                                             ^~
cheevos-new/cheevos.c:510:7: note: ‘snprintf’ output 11 or more bytes (assuming 4106) into a destination of size 256
       snprintf(shotname, sizeof(shotname), "%s/%s-cheevo-%u",
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       settings->paths.directory_screenshot,
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       path_basename(path_get(RARCH_PATH_BASENAME)),
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       cheevo->info->id);
       ~~~~~~~~~~~~~~~~~
cheevos-new/cheevos.c: In function ‘cheevos_iterate’:
cheevos-new/cheevos.c:1812:36: warning: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 237 [-Wformat-truncation=]
                "RetroAchievements: %s",
                                    ^~
                tok);
                ~~~                  
cheevos-new/cheevos.c:1811:10: note: ‘snprintf’ output between 20 and 275 bytes into a destination of size 256
          snprintf(msg, sizeof(msg),
          ^~~~~~~~~~~~~~~~~~~~~~~~~~
                "RetroAchievements: %s",
                ~~~~~~~~~~~~~~~~~~~~~~~~
                tok);
                ~~~~
cheevos-new/cheevos.c:1946:14: warning: ‘i’ may be used uninitialized in this function [-Wmaybe-uninitialized]
          int i, ret;
              ^

orbea commented 5 years ago

This diff silences the first half of the cheevos warnings, but I am not sure if this is the correct behavior or if I am only hiding a bug...

diff --git a/command.c b/command.c
index 61f1554054..206552788b 100644
--- a/command.c
+++ b/command.c
@@ -291,19 +291,22 @@ static bool command_read_ram(const char *arg)

    data = cheevos_patch_address(addr, cheevos_get_console());

-   if (data)
-   {
-      for (i = 0; i < nbytes; i++)
-         sprintf(reply_at+3*i, " %.2X", data[i]);
-      reply_at[3*nbytes] = '\n';
-      command_reply(reply, reply_at+3*nbytes+1 - reply);
-   }
-   else
+   if(reply != NULL)
    {
-      strlcpy(reply_at, " -1\n", sizeof(reply)-strlen(reply));
-      command_reply(reply, reply_at+strlen(" -1\n") - reply);
+      if (data)
+      {
+         for (i = 0; i < nbytes; i++)
+            sprintf(reply_at+3*i, " %.2X", data[i]);
+         reply_at[3*nbytes] = '\n';
+         command_reply(reply, reply_at+3*nbytes+1 - reply);
+      }
+      else
+      {
+         strlcpy(reply_at, " -1\n", sizeof(reply)-strlen(reply));
+         command_reply(reply, reply_at+strlen(" -1\n") - reply);
+      }
+      free(reply);
    }
-   free(reply);
 #else
    cheevos_var_t var;
    unsigned i;

hhromic commented 5 years ago

@orbea I haven't forget about your last message, I'm just finding the time to review it more closely. As you say, we have to make sure it's not hiding a real bug. Will come back as soon as I can :)

hhromic commented 5 years ago

@orbea I'm testing now your proposed patch for the cheevos warnings, however I found out some issues while building regarding detection of X11 and Xrandr.

In my test system, I don't have libxrandr-dev installed and the configure script correctly detects this, all good. Morover, I do have libx11-dev installed as part of OpenGL dependencies and the configure script does define HAVE_X11 (correctly).

However, the problem is that gfx/display_servers/dispserv_x11.c of course is tried to be compiled and fails because it requires the Xrandr headers installed. If you look at its source code:

https://github.com/libretro/RetroArch/blob/d2f73e3f33f93f8606bcf7e60d09cfc74a73d304/gfx/display_servers/dispserv_x11.c#L23-L31

you can see that the Xrandr include is actually duplicated, one inside and one outside the define check (wrong). There are other xrandr-related includes outside the define check too. However, if you move these includes inside the define block, like this:

#ifdef HAVE_XRANDR
#include <X11/extensions/Xrandr.h> /* run pkg-config --static --libs xrandr */
#include <X11/extensions/randr.h>
#include <X11/extensions/Xrender.h>
#endif

You get a lot of compilation errors because the code is all hard-depending on Xrandr. So in conclusion it looks like HAVE_XRANDR and HAVE_X11 are both actually required at the same time. If Xrandr is not found, X11 shouldn't be enabled at all. The alternative is to add defines wrapping all the code that uses Xrandr, which is not trivial and should be done by whoever wrote the code IMHO.

What do you think?

hhromic commented 5 years ago

Actually, doesn't look that hard to properly protect the code for HAVE_XRANDR now that I pay a closer look. If you agree, I can work out a PR to address this. Let me know.

orbea commented 5 years ago

Sounds good to me, I have xrandr installed so I didn't notice this. I imagine this is the same for many users...

hhromic commented 5 years ago

Yes, it's highly likely that xrandr is installed alongside x11, but is not strictly necessary as in my minimal build test environment. So either we stricten the HAVE_X11 and HAVE_XRANDR requirement for building X11 or we properly put defines in the source code to account for HAVE_X11 but no HAVE_XRANDR.

I've been reviewing the code in gfx/display_servers/dispserv_x11.c (the only place using xrandr) and looks like the xrandr extensions are only used for crt mode settings. The code is kind of messy (lots of redundant variables for example) in there but I'm currently cleaning it out and adding define checks for HAVE_XRANDR. If a system doesn't have xrandr, it just won't support crt mode changes gracefully.

Will be sending a PR soon.

hhromic commented 5 years ago

@orbea regarding the cheevos warnings, I'm not seeing any of these warnings with gcc-6.3 and C89_BUILD=1. I guess you are testing with gcc-8 as you said in the PR. Will try to setup a build environment with gcc-8.

By the way, in case you are not aware, with CXX_BUILD=1 it is not building currently:

cheevos-new/cheevos.c: In function ‘int cheevos_iterate(coro_t*)’:
cheevos-new/cheevos.c:1623:29: error: jump to case label [-fpermissive]
          CORO_GOSUB(HTTP_GET);
                             ^
cheevos-new/cheevos.c:1796:26: error: jump to case label [-fpermissive]
       CORO_GOSUB(HTTP_GET);
                          ^

hhromic commented 5 years ago

Okay managed to install gcc-8 from upcoming Debian Buster and I can reproduce your warnings (and some more X_X) now using make without C89_BUILD nor CXX_BUILD.

hhromic commented 5 years ago

This diff silences the first half of the cheevos warnings, but I am not sure if this is the correct behavior or if I am only hiding a bug...

diff --git a/command.c b/command.c
index 61f1554054..206552788b 100644
--- a/command.c
+++ b/command.c
@@ -291,19 +291,22 @@ static bool command_read_ram(const char *arg)

    data = cheevos_patch_address(addr, cheevos_get_console());

-   if (data)
-   {
-      for (i = 0; i < nbytes; i++)
-         sprintf(reply_at+3*i, " %.2X", data[i]);
-      reply_at[3*nbytes] = '\n';
-      command_reply(reply, reply_at+3*nbytes+1 - reply);
-   }
-   else
+   if(reply != NULL)
    {
-      strlcpy(reply_at, " -1\n", sizeof(reply)-strlen(reply));
-      command_reply(reply, reply_at+strlen(" -1\n") - reply);
+      if (data)
+      {
+         for (i = 0; i < nbytes; i++)
+            sprintf(reply_at+3*i, " %.2X", data[i]);
+         reply_at[3*nbytes] = '\n';
+         command_reply(reply, reply_at+3*nbytes+1 - reply);
+      }
+      else
+      {
+         strlcpy(reply_at, " -1\n", sizeof(reply)-strlen(reply));
+         command_reply(reply, reply_at+strlen(" -1\n") - reply);
+      }
+      free(reply);
    }
-   free(reply);
 #else
    cheevos_var_t var;
    unsigned i;

Okay, so this patch won't really work because reply is initialised as NULL, therefore the code block will never be executed: if (reply != NULL). I believe the current code is buggy as command_reply is supposed to use the content of reply and that is never initialised. Take for example the code further down below (after #else) and you will see reply is populated before caling command_reply.

I strongly think the current code is work in progress as is clearly incomplete :)

hhromic commented 5 years ago

@orbea I'm going to close this issue for now, if I find more warnings I will simply send PRs for revision and merging :) Cheers!

libretro / RetroArch