Retroarch: Minor warnings during compilation (GCC 6.3.0 on standard Raspbian Stretch)
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 [fix merged]%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.
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.
hhromic
All 75 comments
A bit further on the topic of warnings, the deps/discord-rpc dependency codease is using #pragma warning pre-processor macros that are not supported by GCC, but by Microsoft compilers.
Making these pragmas portable seems to be a hard problem so I would ignore that for now.
hhromic
on 6 Feb 2019
Okay, the warnings seem to be because PRI_SIZET is not being defined portably in deps/libretro-common/include/retro_miscellaneous.h:
As suggested here, probably defining zu instead of lu in the last define would be the correct solution.
hhromic
on 6 Feb 2019
I saw similar warnings as in the OP with 32-bit linux too.
orbea
on 6 Feb 2019
Thinking more about it, probably the issue doesn't manifest in 64 bits OSs because size_t probably matches long unsigned int and the warning is then not triggered.
As you noted, 32-bit linuxs such as armv7 devices (e.g. the RPI) probably always manifest the warnings.
What C std is RetroArch aiming for? %zu should be available from c99 onwards.
hhromic
on 6 Feb 2019
It needs to be c89 compatible, unfortunately while %zu silences the warnings here with 32-bit linux, it causes new warnings with C89_BUILD=1.
Try:
./configure && make C89_BUILD=1
orbea
on 6 Feb 2019
Just tested compiling in 64 bits linux and the format warnings are not there as expected, so they are being triggered in 32 bits machines only (probably).
Um I see, the easy solution using %zu is unavailable then. Maybe a selective define to set lu or li depending on bitness should be c89-compatible. Similar to what is done in the windows defines.
hhromic
on 6 Feb 2019
Thanks for the C89_BUILD pointer, if you are interested on silencing these warnings, I can think of a suitable solution and make sure it's c89 compliant.
hhromic
on 6 Feb 2019
Yes, silencing warnings is always desirable if it doesn't cause regressions. :)
orbea
on 6 Feb 2019
I tried %li and got these warnings still with 32-bit linux.
setting_list.c: In function 'setting_get_string_representation_size':
setting_list.c:179:24: warning: format '%li' expects argument of type 'long int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
snprintf(s, len, "%" PRI_SIZET,
^
setting_list.c:179:24: warning: format '%li' expects argument of type 'long int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
setting_list.c: In function 'setting_get_string_representation_size_in_mb':
setting_list.c:187:24: warning: format '%li' expects argument of type 'long int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
snprintf(s, len, "%" PRI_SIZET,
^
setting_list.c:187:24: warning: format '%li' expects argument of type 'long int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat=]
setting_list.c: In function 'setting_set_with_string_representation':
setting_list.c:511:24: warning: format '%li' expects argument of type 'long int *', but argument 3 has type 'size_t * {aka unsigned int *}' [-Wformat=]
sscanf(value, "%" PRI_SIZET, setting->value.target.sizet);
^
libretro-common/file/config_file.c: In function 'config_get_size_t':
libretro-common/file/config_file.c:694:32: warning: format '%li' expects argument of type 'long int *', but argument 3 has type 'size_t * {aka unsigned int *}' [-Wformat=]
if (sscanf(entry->value, "%" PRI_SIZET, &val) == 1)
^
I think for 32 bits should be just %u (for unsigned int as it's complaining) and for 64 bits should be the curent %lu (for long unsigned int), mirroring the windows defines above. The problem is how to reliably detect the bitness for the non-windows case and set the define accordingly. The best I could find so far is this:
#if __GNUC__
#if __x86_64__ || __ppc64__
#define ENVIRONMENT64
#else
#define ENVIRONMENT32
#endif
#endif
However it's GCC-dependent. If RetroArch is required to be built using gcc compilers, then that should work reliably for all the platforms supported. What do you think?
Edit: the retroarch build system doesn't already provide a bitness flag, right?
hhromic
on 6 Feb 2019
Also, care must be taken to also consider defines such as __aarch64__ in that check.
hhromic
on 6 Feb 2019
Yes, %u works for 32-bit linux, I might have something that works, but I need to fix some c89 build failures with 32-bit linux first...
orbea
on 6 Feb 2019
Great! Looking forward to see your idea, take your time.
Also, let's not forget about the other puzzle:
The third warning is because
gl2_renderchain_init_pbois 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.
It's fun to hunt warnings :)
hhromic
on 6 Feb 2019
However it's GCC-dependent. If RetroArch is required to be built using
gcccompilers, then that should work reliably for all the platforms supported. What do you think?
Nah, we try to be compiler-agnostic. Clang/GCC/MSVC should all be viable options for compiling RetroArch.
twinaphex
on 6 Feb 2019
That means these conditionals are a no-no as well. I found other methods but I'm not sure of their robustness, for example comparing sizeof(void *) == 8 to detect pointer size. But nothing is safe in cross-platform. Let's wait for what orbea has in mind.
hhromic
on 6 Feb 2019
@hhromic This works for 64/32-bit linux, can you test it too?
diff --git a/libretro-common/include/retro_miscellaneous.h b/libretro-common/include/retro_miscellaneous.h
index bdb41f7bba..9114357675 100644
--- a/libretro-common/include/retro_miscellaneous.h
+++ b/libretro-common/include/retro_miscellaneous.h
@@ -159,14 +159,18 @@ typedef struct
# ifdef _WIN64
# define PRI_SIZET PRIu64
# else
-#if _MSC_VER == 1800
-# define PRI_SIZET PRIu32
+# if _MSC_VER == 1800
+# define PRI_SIZET PRIu32
+# else
+# define PRI_SIZET "u"
+# endif
+# endif
#else
+# ifdef __x86_64__
+# define PRI_SIZET "lu"
+# else
# define PRI_SIZET "u"
-#endif
# endif
-#else
-# define PRI_SIZET "lu"
#endif
#endif
That probably works on x86/64 machines but I don't think it will work for example in ARM/ARM64 devices where there is no __x86_64__ define and will fall-back to u for example in ARM64 where it should be lu.
I saw there are many bitness-related conditionals in retroarch files such as config.def.h, perhaps a collation can be performed to safely cover most of the supported platforms. I would think creating a more abstract ENV32BITS / ENV64BITS set of defines would be beneficial as well.
hhromic
on 6 Feb 2019
Actually, this whole thing can be simpler. In the end we are trying to condition the formatting for size_t, which by definition is always an unsigned integer. So we just need to figure out if it's unsigned int, unsigned long or unsigned long long (rare but possible!), and define the format accordingly: u, lu or llu. I will investigate later how this can be determined in the pre-processor, maybe in limits.h there is a clue, and come back with an idea.
hhromic
on 6 Feb 2019
Yea, I only have access to linux 64/32 bit build environments so testing arm or other platforms is hard for me...
orbea
on 6 Feb 2019
I can test on RPI (ARM32), Win32/64 and Linux64.
Once we found a suitable solution I will test on these and report back.
hhromic
on 6 Feb 2019
Thanks! The biggest worry are consoles, but the buildbot should find hopefully find those issues where they can then be fixed...
orbea
on 6 Feb 2019
@orbea so, I put the idea from https://github.com/libretro/RetroArch/issues/8191#issuecomment-461138089 in practice and came up with the following define logic for non-windows platforms:
#include <stdint.h>
#include <stdio.h>
#if (SIZE_MAX == 0xFFFF)
#define PRI_SIZET "hu"
#elif (SIZE_MAX == 0xFFFFFFFF)
#define PRI_SIZET "u"
#elif (SIZE_MAX == 0xFFFFFFFFFFFFFFFF)
#define PRI_SIZET "lu"
#else
#error PRI_SIZET: unknown SIZE_MAX
#endif
int main() {
size_t test = 123456;
printf("PRI_SIZET = %s\n", PRI_SIZET);
printf("size_t test = %" PRI_SIZET "\n", test);
return 0;
}
I got the idea from here and basically it compares SIZE_MAX (from stdint.h) which is the size of size_t against known sizes and consequently defines the format specification accodingly. It recognises three very common sizes for 16, 32 and 64 bits.
When compiled using c89 mode it produces no warnings in 32 nor 64 bits platforms:
gcc -std=c89 -pedantic -Wall pri_sizet.c -o pri_sizet
Please note that stdint.h is not strictly part of c89, however it is used all around the RetroArch codebase, so should be fine. I tested this using a Linux64 and ARM32 platforms. How does it look?
hhromic
on 6 Feb 2019
I tried this.
-#ifdef _WIN32
-# ifdef _WIN64
-# define PRI_SIZET PRIu64
-# else
-#if _MSC_VER == 1800
-# define PRI_SIZET PRIu32
+#if (SIZE_MAX == 0xFFFF)
+#define PRI_SIZET "hu"
+#elif (SIZE_MAX == 0xFFFFFFFF)
+#define PRI_SIZET "u"
+#elif (SIZE_MAX == 0xFFFFFFFFFFFFFFFF)
+#define PRI_SIZET "lu"
#else
-# define PRI_SIZET "u"
-#endif
-# endif
-#else
-# define PRI_SIZET "lu"
+#error PRI_SIZET: unknown SIZE_MAX
#endif
Bluntly I am not an experienced programmer so my feedback on correctness is not worth that much, but I ran it with 64/32-bit linux without any new warnings and it did solve the already relevant warnings. :)
That said I next tried it with travis which found these new warnings with the 64-bit mingw build.
setting_list.c: In function ‘setting_set_with_string_representation’:
setting_list.c:511:10: warning: format ‘%lu’ expects argument of type ‘long unsigned int *’, but argument 3 has type ‘size_t *’ [-Wformat=]
sscanf(value, "%" PRI_SIZET, setting->value.target.sizet);
^
libretro-common/file/config_file.c: In function ‘config_get_size_t’:
libretro-common/file/config_file.c:694:7: warning: format ‘%lu’ expects argument of type ‘long unsigned int *’, but argument 3 has type ‘size_t *’ [-Wformat=]
if (sscanf(entry->value, "%" PRI_SIZET, &val) == 1)
^
md5-55f7b3ce07651374839743b03797f68d
diff --git a/libretro-common/include/retro_miscellaneous.h b/libretro-common/include/retro_miscellaneous.h
index bdb41f7bba..13364c8859 100644
--- a/libretro-common/include/retro_miscellaneous.h
+++ b/libretro-common/include/retro_miscellaneous.h
@@ -159,14 +159,22 @@ typedef struct
# ifdef _WIN64
# define PRI_SIZET PRIu64
# else
-#if _MSC_VER == 1800
-# define PRI_SIZET PRIu32
+# if _MSC_VER == 1800
+# define PRI_SIZET PRIu32
+# else
+# define PRI_SIZET "u"
+# endif
+# endif
#else
+# if (SIZE_MAX == 0xFFFF)
+# define PRI_SIZET "hu"
+# elif (SIZE_MAX == 0xFFFFFFFF)
# define PRI_SIZET "u"
-#endif
+# elif (SIZE_MAX == 0xFFFFFFFFFFFFFFFF)
+# define PRI_SIZET "lu"
+# else
+# error PRI_SIZET: unknown SIZE_MAX
# endif
-#else
-# define PRI_SIZET "lu"
#endif
#endif
Yes, as I stated in my post, the example alone I posted was meant for non-windows platforms only, and should been integrated with the existing windows-based defines, as you correctly did. So, it should look like this in its final form:
#ifdef _WIN32
# ifdef _WIN64
# define PRI_SIZET PRIu64
# else
# if _MSC_VER == 1800
# define PRI_SIZET PRIu32
# else
# define PRI_SIZET "u"
# endif
# endif
#else
# if (SIZE_MAX == 0xFFFF)
# define PRI_SIZET "hu"
# elif (SIZE_MAX == 0xFFFFFFFF)
# define PRI_SIZET "u"
# elif (SIZE_MAX == 0xFFFFFFFFFFFFFFFF)
# define PRI_SIZET "lu"
# else
# error PRI_SIZET: unknown SIZE_MAX
# endif
#endif
That is working fine for me in Linux64, Windows64 and ARM32. If it works for you in Travis without any warnings, then I think it should be the correct way to handle PRI_SIZET cross-platform.
Do you want me to send a PR for this?
hhromic
on 7 Feb 2019
I misunderstood/misread inititially and yes, if you can send a PR that would be great. :)
orbea
on 7 Feb 2019
So what are the remaining warnings in our code? Lets not consider stuff in deps and similar for now which ideally should be fixed upstream.
Here with gcc-8.2.0 I only see.
gfx/display_servers/dispserv_x11.c: In function ‘x11_display_server_set_resolution’:
gfx/display_servers/dispserv_x11.c:136:12: warning: variable ‘scrn’ set but not used [-Wunused-but-set-variable]
Screen *scrn = NULL;
^~~~
This will probably be fixed soon, see: https://github.com/libretro/RetroArch/pull/8132#issuecomment-459861231
With clang-7.0.1 I see.
ui/drivers/qt/qt_playlist.cpp:268:20: warning: unused function 'comp_hash_label_key_lower' [-Wunused-function]
inline static bool comp_hash_label_key_lower(const QHash<QString, QString> &lhs, const QHash<QString, QString> &rhs)
^
1 warning generated.
cores/libretro-gong/gong.c:121:20: warning: unused function 'pressed' [-Wunused-function]
static INLINE bool pressed(Game_Button_State state)
^
1 warning generated.
md5-44452f48c03197360054f54dce8eb705
menu/menu_driver.c: In function ‘menu_subsystem_populate’:
menu/menu_driver.c:2698:65: warning: universal character names are only valid in C++ and C99
snprintf(star_char, sizeof(star_char), "%s", is_rgui ? "*" : "★");
^~~~~~~~
md5-097b2093b53ddfade653d42de10c8868
runahead/secondary_core.c: In function ‘char* get_temp_directory_alloc()’:
runahead/secondary_core.c:84:11: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings]
path = "/tmp";
^~~~~~
Do you see any others?
orbea
on 8 Feb 2019
Here is a potential fix for the CXX_BUILD warning, I'm not sure its ideal though...
diff --git a/runahead/secondary_core.c b/runahead/secondary_core.c
index f597c1afaa..606b36c1cf 100644
--- a/runahead/secondary_core.c
+++ b/runahead/secondary_core.c
@@ -51,7 +51,7 @@ void clear_controller_port_map(void);
static char *get_temp_directory_alloc(void)
{
- char *path = NULL;
+ const char *path = NULL;
#ifdef _WIN32
#ifdef LEGACY_WIN32
DWORD pathLength = GetTempPath(0, NULL) + 1;
@@ -87,7 +87,7 @@ static char *get_temp_directory_alloc(void)
path = strcpy_alloc_force(path);
#endif
- return path;
+ return (char*)path;
}
static bool write_file_with_random_name(char **tempDllPath,
So what are the remaining warnings in our code? Lets not consider stuff in deps and similar for now which ideally should be fixed upstream.
I have to check but last time the third warning mentioned in the OP.
By the way, what is your invocation for configure? Is there any configure option to "enable everything"? Otherwise we won't be covering all the code for testing.
Here with
gcc-8.2.0I only see.gfx/display_servers/dispserv_x11.c: In function ‘x11_display_server_set_resolution’: gfx/display_servers/dispserv_x11.c:136:12: warning: variable ‘scrn’ set but not used [-Wunused-but-set-variable] Screen *scrn = NULL; ^~~~This will probably be fixed soon, see: #8132 (comment)
I guess we have to wait for that issue/PR to be completed first.
With
clang-7.0.1I see.ui/drivers/qt/qt_playlist.cpp:268:20: warning: unused function 'comp_hash_label_key_lower' [-Wunused-function] inline static bool comp_hash_label_key_lower(const QHash<QString, QString> &lhs, const QHash<QString, QString> &rhs) ^ 1 warning generated.
cores/libretro-gong/gong.c:121:20: warning: unused function 'pressed' [-Wunused-function] static INLINE bool pressed(Game_Button_State state) ^ 1 warning generated.
(And in libchdr which is not our code.)It seems clang will warn on unused inline functions while gcc won't.
Yes I think that is known. Most of the unused warnings can be fixed by better ifdef handling. Will take a look later when I get a bit more time.
With
C89_BUILDthis one still remains with both gcc and clang.menu/menu_driver.c: In function ‘menu_subsystem_populate’: menu/menu_driver.c:2698:65: warning: universal character names are only valid in C++ and C99 snprintf(star_char, sizeof(star_char), "%s", is_rgui ? "*" : "★"); ^~~~~~~~
Ah I mentioned that one before, it's tricky because it's a unicode character. We could replace it with the raw bytes but the "star" character might not display correctly in some platforms, it requires testing. Another alternative is to use the ICU library, but might be overkill.
With
CXX_BUILD=1using either gcc or clang.runahead/secondary_core.c: In function ‘char* get_temp_directory_alloc()’: runahead/secondary_core.c:84:11: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings] path = "/tmp"; ^~~~~~
(will answer your other message about this)
Do you see any others?
I haven't check again, but will soon. Let me know your configure arguments!
hhromic
on 8 Feb 2019
Here is a potential fix for the
CXX_BUILDwarning, I'm not sure its ideal though...diff --git a/runahead/secondary_core.c b/runahead/secondary_core.c index f597c1afaa..606b36c1cf 100644 --- a/runahead/secondary_core.c +++ b/runahead/secondary_core.c @@ -51,7 +51,7 @@ void clear_controller_port_map(void); static char *get_temp_directory_alloc(void) { - char *path = NULL; + const char *path = NULL; #ifdef _WIN32 #ifdef LEGACY_WIN32 DWORD pathLength = GetTempPath(0, NULL) + 1; @@ -87,7 +87,7 @@ static char *get_temp_directory_alloc(void) path = strcpy_alloc_force(path); #endif - return path; + return (char*)path; } static bool write_file_with_random_name(char **tempDllPath,
That fix is not appropiate as path needs to be modifiable, i.e. not a constant const. The problem is the direct assignment of a string literal (const) to the char pointer. As you can see a little below that point in the code, what you need to do is to "copy" the string literal into the path variable:
path = strcpy_alloc_force("/tmp");
That should be safe and should silence the warning.
hhromic
on 8 Feb 2019
Actually, that block of code is a bit convoluted:
path = "/tmp";
if (getenv("TMPDIR"))
path = getenv("TMPDIR");
path = strcpy_alloc_force(path);
A better/simpler code block would be:
if (getenv("TMPDIR"))
path = strcpy_alloc_force(getenv("TMPDIR"));
else
path = strcpy_alloc_force("/tmp");
I am just doing ./configure && make, ./configure && make C89_BUILD=1 and ./configure && make CXX_BUILD=1 with most possible optional dependencies installed. Its hard to find all the warnings because of the sheer volume of features and supported platforms, the configure script still has a few issues left to handle... For example try setting set -eu... I am better with shell than C so I have been slowly working on that, hopefully in the future it will be easier to figure these things out... However I can't test some of the supported options like HAVE_VIDEOCORE which certainly need improvement...
orbea
on 8 Feb 2019
with most possible optional dependencies installed
ahh that is what I'm not doing in my build environment.
do you have list of these handy?
hhromic
on 8 Feb 2019
I don't have a complete list, but from my own list of optional dependencies for slackbuilds.org I have.
ffmpeg jack-audio-connection-kit libsixel libxkbcommon mbedtls
miniupnpc nvidia-cg-toolkit OpenAL python3 qt5 SDL2 vulkansdk
wayland-egl wayland-protocols
I haven't checked the build with jack-audio-connection-kit, nvidia-cg-toolkit (Deprecated by nvidia in 2012) or wayland. Some of them like mbedtls or miniupnpc require to use the related --disable-builtinmbedtls or --disable-builtinminiupnpc options to actually use the system lib.
Also this list doesn't include dependencies already in the Slackware main tree like flac or v4l2-utils which I don't have a complete list of...
This would be hard for me to document well except for with Slackware unfortunately where the packages aren't split up like in debian or ubuntu meaning it wouldn't be helpful for a lot of users.
There are also more warnings with --enable-sixel, @bparker06 might have a better idea since he implemented this driver.
gfx/drivers_font/sixel_font.c: In function ‘sixel_render_msg’:
gfx/drivers_font/sixel_font.c:83:19: warning: variable ‘newY’ set but not used [-Wunused-but-set-variable]
unsigned newX, newY;
^~~~
gfx/drivers_font/sixel_font.c:83:13: warning: variable ‘newX’ set but not used [-Wunused-but-set-variable]
unsigned newX, newY;
^~~~
gfx/drivers_font/sixel_font.c: At top level:
gfx/drivers_font/sixel_font.c:133:4: warning: initialization of ‘void (*)(video_frame_info_t *, void *, const char *, const struct font_params *)’ {aka ‘void (*)(struct video_frame_info *, void *, const char *, const struct font_params *)’} from incompatible pointer type ‘void (*)(video_frame_info_t *, void *, const char *, const void *)’ {aka ‘void (*)(struct video_frame_info *, void *, const char *, const void *)’} [-Wincompatible-pointer-types]
sixel_render_msg,
^~~~~~~~~~~~~~~~
gfx/drivers_font/sixel_font.c:133:4: note: (near initialization for ‘sixel_font.render_msg’)
CC gfx/drivers_context/sixel_ctx.c
CC menu/drivers_display/menu_display_sixel.c
menu/drivers_display/menu_display_sixel.c:93:4: warning: initialization of ‘void (*)(menu_display_ctx_draw_t *, video_frame_info_t *)’ {aka ‘void (*)(struct menu_display_ctx_draw *, struct video_frame_info *)’} from incompatible pointer type ‘void (*)(void *)’ [-Wincompatible-pointer-types]
menu_display_sixel_draw,
^~~~~~~~~~~~~~~~~~~~~~~
menu/drivers_display/menu_display_sixel.c:93:4: note: (near initialization for ‘menu_display_ctx_sixel.draw’)
menu/drivers_display/menu_display_sixel.c:94:4: warning: initialization of ‘void (*)(menu_display_ctx_draw_t *, video_frame_info_t *)’ {aka ‘void (*)(struct menu_display_ctx_draw *, struct video_frame_info *)’} from incompatible pointer type ‘void (*)(void *)’ [-Wincompatible-pointer-types]
menu_display_sixel_draw_pipeline,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
menu/drivers_display/menu_display_sixel.c:94:4: note: (near initialization for ‘menu_display_ctx_sixel.draw_pipeline’)
menu/drivers_display/menu_display_sixel.c:95:4: warning: initialization of ‘void (*)(menu_display_ctx_draw_t *, video_frame_info_t *)’ {aka ‘void (*)(struct menu_display_ctx_draw *, struct video_frame_info *)’} from incompatible pointer type ‘void (*)(void *)’ [-Wincompatible-pointer-types]
menu_display_sixel_viewport,
^~~~~~~~~~~~~~~~~~~~~~~~~~~
menu/drivers_display/menu_display_sixel.c:95:4: note: (near initialization for ‘menu_display_ctx_sixel.viewport’)
menu/drivers_display/menu_display_sixel.c:96:4: warning: initialization of ‘void (*)(video_frame_info_t *)’ {aka ‘void (*)(struct video_frame_info *)’} from incompatible pointer type ‘void (*)(void)’ [-Wincompatible-pointer-types]
menu_display_sixel_blend_begin,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
menu/drivers_display/menu_display_sixel.c:96:4: note: (near initialization for ‘menu_display_ctx_sixel.blend_begin’)
menu/drivers_display/menu_display_sixel.c:97:4: warning: initialization of ‘void (*)(video_frame_info_t *)’ {aka ‘void (*)(struct video_frame_info *)’} from incompatible pointer type ‘void (*)(void)’ [-Wincompatible-pointer-types]
menu_display_sixel_blend_end,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
menu/drivers_display/menu_display_sixel.c:97:4: note: (near initialization for ‘menu_display_ctx_sixel.blend_end’)
menu/drivers_display/menu_display_sixel.c:99:4: warning: initialization of ‘void (*)(menu_display_ctx_clearcolor_t *, video_frame_info_t *)’ {aka ‘void (*)(struct menu_display_ctx_clearcolor *, struct video_frame_info *)’} from incompatible pointer type ‘void (*)(menu_display_ctx_clearcolor_t *)’ {aka ‘void (*)(struct menu_display_ctx_clearcolor *)’} [-Wincompatible-pointer-types]
menu_display_sixel_clear_color,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
menu/drivers_display/menu_display_sixel.c:99:4: note: (near initialization for ‘menu_display_ctx_sixel.clear_color’)
menu/drivers_display/menu_display_sixel.c:100:4: warning: initialization of ‘void * (*)(video_frame_info_t *)’ {aka ‘void * (*)(struct video_frame_info *)’} from incompatible pointer type ‘void * (*)(void)’ [-Wincompatible-pointer-types]
menu_display_sixel_get_default_mvp,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
menu/drivers_display/menu_display_sixel.c:100:4: note: (near initialization for ‘menu_display_ctx_sixel.get_default_mvp’)
@orbea - wayland is not building for me in Debian 9 (with backports). Unfortunately wayland-protocols is at version 1.13 there and RetroArch seems to require 1.15 minimum. This makes RetroArch+Wayland unbuildable in standard Debian 9 even with backports enabled.
Is this a desirable situation? Debian is a very widely used Linux OS :(
hhromic
on 9 Feb 2019
Regarding the warnings hunting, I'm only getting the following types of warnings with CXX_BUILD=1:
menu/menu_displaylist.c: In function ‘bool menu_displaylist_ctl(menu_displaylist_ctl_state, menu_displaylist_info_t*)’:
menu/menu_displaylist.c:8495:60: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (!checked_found && val == orig_value)
~~~~^~~~~~~~~~~~~
menu/menu_displaylist.c:8521:60: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (!checked_found && val == orig_value)
~~~~^~~~~~~~~~~~~
menu/menu_displaylist.c:8846:57: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (!checked_found && val == orig_value)
~~~~^~~~~~~~~~~~~
menu/menu_displaylist.c:8872:57: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
if (!checked_found && val == orig_value)
~~~~^~~~~~~~~~~~~
There are other source files with exactly the same warning.
The problem is simple, there are comparisons in if statements that are being done between signed/unsigned types. For example in menu/menu_displaylist.c, line 8495:
if (!checked_found && val == orig_value)
The type of val is int and the type of orig_value is unsigned. It seems to me that a bit earlier, the line:
int val = (int)i;
Should be:
unsigned val = (unsigned)i;
The same rationale is for all the other warnings of this type.
I don't have time this weekend to help with this, but I will have later during the week if you wish for me to provide a PR.
hhromic
on 9 Feb 2019
It would be nice to add support for older versions of wayland-protocols like was done with the wayland-scanner, but I use x11 and have very little knowledge of wayland so I am not sure how it should be done.
The problem would need to be addressed in this script.
https://github.com/libretro/RetroArch/blob/master/gfx/common/wayland/generate_wayland_protos.sh
Also see commits https://github.com/libretro/RetroArch/commit/63c7abef067704d8ece0a8b6563c48880a156134 and https://github.com/libretro/RetroArch/commit/677395f05e71f3090c7de9f7bd8c05cbc5ec4229.
@Sunderland93 Would you have any ideas about this? Is it even possible?
Fwiw with Slackware I just asked the wayland-protocols maintainer to update his SlackBuild and this was done immediately, it should be an easy and painless upgrade for the debian maintainers. Its also very easy to build from source even inside travis as some projects have been forced to do...
Regarding the warnings hunting, I'm only getting the following types of warnings with CXX_BUILD=1
I haven't seen these warnings yet, but I'll try and look harder to see if I can reproduce them. Otherwise I think it might be better if you could fix them when time permits. :)
orbea
on 9 Feb 2019
We support xdg-decoration-unstable-v1 protocol, which was introduced in wayland-protocols 1.15. That's why the minimum required version is 1.15. Actually, RetroArch needs four protocols: xdg-shell stable, xdg-shell-unstable-v6, xdg-decoration-unstable-v1 and idle-inhibit-unstable-v1. We may bundle them with RetroArch, and not depend on full wayland-protocols set.
Sunderland93
on 10 Feb 2019
Do the generated files differ at all between systems? I wonder if files generated on Slackware would work on Debian or vica versa?
From a package management point of perspective bundling libraries or generated files is sometimes frowned upon and if this is done it would be good to retain both paths so that users could still use their own wayland-protocols packages.
orbea
on 10 Feb 2019
I wonder if files generated on Slackware would work on Debian or vica versa?
Of course
From a package management point of perspective bundling libraries or generated files is sometimes frowned upon and if this is done it would be good to retain both paths so that users could still use their own wayland-protocols packages.
Many apps contains build-time dependencies (as submodules or in "externals" dir) and provide build options to choose between system libraries and external (such as -DUSE_SYSTEM_LIBS=On). Maybe we could do something like that?
Sunderland93
on 10 Feb 2019
On the other hand, wayland-protocols is just set of .xml's, it doesn't break anything in system...
Sunderland93
on 10 Feb 2019
I have one idea for that. I'll make PR later today.
Sunderland93
on 10 Feb 2019
@Sunderland93 , I was thinking exactly the same about wayland-protocols being just a set of xml descriptors, so I think I know what your idea is :) looking forward for your PR then. Thanks!
Debian 9 (stretch) comes with wayland 1.12.0-1, which should be good enough. The Debian 9 backports come with wayland 1.16.0-1, which is what I'm using for building.
For the record, the next Debian version (buster) is set to include wayland-protocols 1.17-1, so Debian should be soon supported with RetroArch as-is:
https://packages.debian.org/search?keywords=wayland-protocols
Debian 10 is set to be released mid-2019, which is quite soon. However, eliminating the current dependency on wayland-protocols 1.15 would be a great thing for compatibility due to current Debian 9 being very widely used nowadays.
hhromic
on 10 Feb 2019
Many apps contains build-time dependencies (as submodules or in "externals" dir) and provide build options to choose between system libraries and external (such as -DUSE_SYSTEM_LIBS=On). Maybe we could do something like that?
Well, I suppose if the correct wayland-scanner version is found we could use the system version or fallback to bundled versions when its missing?
orbea
on 10 Feb 2019
@orbea @Sunderland93
How about bundling just the wayland protocols files, i.e. not the main wayland packages? I mean that is not necessary to bundle wayland-scanner, etc. This wouldn't even require a configure flag for it.
For example SDL2 does only bundle their own copy of wayland protocols (not even the complete set, just the specific files they need), which is just a set of xml files (see: https://github.com/SDL-mirror/SDL).
Moreover, as the wayland protocols should be pretty fixed/static/stable for the project, probably they won't change at all for RetroArch even if wayland gets updated, right? Only when things related to the protocols are changed, then the xml files would require updating. In the end these xml files are a type of source code more than a functional library dependency.
hhromic
on 10 Feb 2019
@hhromic Yes, I like SDL2-way for that thing.
Sunderland93
on 10 Feb 2019
How about bundling just the wayland protocols files, i.e. not the main wayland packages? I mean that is not necessary to bundle wayland-scanner, etc. This wouldn't even require a configure flag for it.
I think this is the only one, best solution. Also MATE folks did the same https://github.com/mate-desktop/mate-panel/tree/master/mate-panel/wayland-protocols
Sunderland93
on 10 Feb 2019
I think this is the only one, best solution.
So if you agree it's a sensible solution then maybe we should follow the example of these projects :)
I have zero experience on the RetroArch build system, would you or @orbea be able to create a PR for integrating the necessary wayland-protocols files into this repository and update the build system to use these instead of trying to look for the package in the system? All the other wayland packages should be left as they are, being looked in the system.
Maybe gfx/common/wayland-protocols is a good place to put the protocol files so they don't polute the repository main directory? You know best guys :)
Edit: sorry I meant gfx/common/wayland-protocols alongside wayland
hhromic
on 10 Feb 2019
Its still not ideal to force bundling the generated files, there may be cases where newer versions of wayland-protocols would be preferred by specific distributions.
orbea
on 10 Feb 2019
@orbea the idea is not to bundle the generated files, but the source protocol xml files.
This way the generation is done exactly as it is now but with the bundled xml files.
Also the bundled version in RetroArch can be kept always up-to-date (newer), so they never depend on the distribution (if installed).
I'm no expert on wayland but that's what I understood from @Sunderland93 on how the protocol xml files work.
Edit: I understand that the generator takes the xml protocol descriptor files and generate header files to be compiled. These xml protocol descriptors can be bundled (in an always known updated version) but still be generated on build. This is how SDL2/MATE seem to do it.
Edit2: the xml files are not system-dependent but used by the system-installed wayland-scanner to generate the final code (which is system-dependant). This is how the generated code is guaranteed to work on the system being built for, no matter where the xml files come from. @Sunderland93 could you confirm this?
hhromic
on 10 Feb 2019
I see I misunderstood, so the idea would be to bundle parts of wayland-protocols into the code base and use those to generate the files?
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.
orbea
on 10 Feb 2019
could you confirm this?
Yes
Sunderland93
on 10 Feb 2019
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.
Sunderland93
on 10 Feb 2019
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 ).
hhromic
on 10 Feb 2019
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.
orbea
on 10 Feb 2019
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 :)
hhromic
on 10 Feb 2019
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.
orbea
on 10 Feb 2019
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).
hhromic
on 10 Feb 2019
I force pushed some cleanups to my branch and will make PR later today, thanks for testing!
orbea
on 10 Feb 2019
I made a PR.
orbea
on 11 Feb 2019
@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)
{
@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.
hhromic
on 12 Feb 2019
Thanks for the feedback, I wasn't sure if that or the above patch was a better idea.
orbea
on 12 Feb 2019
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.
hhromic
on 12 Feb 2019
I don't really mind either way, like you said its a good learning experience and it makes repeated compiles more tolerable.
orbea
on 12 Feb 2019
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;
^
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;
@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
on 15 Feb 2019
@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:
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
on 22 Feb 2019
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.
hhromic
on 22 Feb 2019
Sounds good to me, I have xrandr installed so I didn't notice this. I imagine this is the same for many users...
orbea
on 22 Feb 2019
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
on 22 Feb 2019
@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);
^
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
on 23 Feb 2019
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
on 23 Feb 2019
@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!
hhromic
on 23 Apr 2019
Related issues
fr500
·
4Comments
hyarsan
·
4Comments
meepingsnesroms
·
4Comments
wrldwzrd89
·
3Comments
GoronMegaZord
·
3Comments
Most helpful comment
I force pushed some cleanups to my branch and will make PR later today, thanks for testing!