fixed `-Wmissing-prototypes` (and subsequent) compiler warnings

[firewave]

Fixing this warning mainly involved adding static identifiers or void parameters to the declaration or adding missing includes which contained the prototypes. This allowed the compiler to detect unused functions (mainly in common/pixman-region.c) and perform improved valueflow (detecting potentially uninitialized variables).

Some warnings remain which require the removal of extern declarations and usage of the proper headers with the respective prototypes instead.

[firewave]

I can still squash this if desired.

[firewave]

On a side note: IMO the variable prefix g_ should only be used for actual global variables (i.e. used in multiple TUs). If they are simply local and available in all scopes they should use something like s_ instead.

[matt335672]

Hi again.

I'd like to reproduce this got the purposes of reviewing the PR. How are you causing clang to generate the errors? It would be good to get any additional error checking into the CI process if possible.

[firewave]

I added CPPFLAGS="-Wmissing-prototypes" to the configure call to produce these warnings.

If you want to integrate this properly and portable you need to add a check to the autoconf process as not all compilers provide this warning. Unfortunately I have never worked with that and have no idea how to add such a check. It is probably just a one liner as this is something common.

[matt335672]

Are you aware of any compilers that don't support it?

I've just checked gcc 4.8.5 on CentOS 7 and that has the flag.

Here's a patch for you which makes the check in any case:-

diff --git a/configure.ac b/configure.ac
index 2f3d6938..e677ea46 100644
--- a/configure.ac
+++ b/configure.ac
@@ -205,6 +205,9 @@ AX_GCC_FUNC_ATTRIBUTE([format])
 AX_TYPE_SOCKLEN_T
 AX_CFLAGS_WARN_ALL
 AX_APPEND_COMPILE_FLAGS([-Wwrite-strings])
+AX_CHECK_COMPILE_FLAG([-Wmissing-prototypes],
+  [AX_APPEND_COMPILE_FLAGS([-Wmissing-prototypes])],
+  , [-Werror])
 
 AM_COND_IF([LINUX],
   [AX_APPEND_COMPILE_FLAGS([-Werror])]) # bsd has warnings that have not been fixed yet

[matt335672]

I've just pushed a branch with that patch in and no other changes. Failing CI results are here:-

https://github.com/matt335672/xrdp/actions/runs/6573596879

[firewave]

I will separate the changes into separate commits for the various warnings (the initial commit mixed together at least three different warnings) so it is better to follow.

I would add your change in a separate PR and clean up the extern (i.e. the remaining warnings) and prefix stuff in that as well as this PR is already quite big.

[firewave]

AX_APPEND_COMPILE_FLAGS already checks if the flag is supported: https://www.gnu.org/software/autoconf-archive/ax_append_compile_flags.html.

[matt335672]

It was even on the previous line in the config and I missed it - Good spot!

I'd suggest the following then:-

--- a/configure.ac
+++ b/configure.ac
@@ -205,6 +205,7 @@ AX_GCC_FUNC_ATTRIBUTE([format])
 AX_TYPE_SOCKLEN_T
 AX_CFLAGS_WARN_ALL
 AX_APPEND_COMPILE_FLAGS([-Wwrite-strings])
+AX_APPEND_COMPILE_FLAGS([-Wmissing-prototypes], ,[-Werror])
 
 AM_COND_IF([LINUX],
   [AX_APPEND_COMPILE_FLAGS([-Werror])]) # bsd has warnings that have not been fixed yet

The [-Werror] is needed as otherwise some compilers (particularly clang) may emit a warning rather than an error. See:-

https://stackoverflow.com/questions/52557417

[firewave]

The [-Werror] is needed as otherwise some compilers (particularly clang) may emit a warning rather than an error.

I was wondering about that but I omitted it anyways. Will adjust.

[firewave]

I am not 100% sure if the removal of the function is safe as those might be used when the code is in a shared object. But as PIXMAN_EXPORT is empty (and could even be removed) it seems like the original code was written for that but it no longer is being used that way. If the functions declared in the header are being checked for actual usage there might be more code which could be removed. You could automate such a check via Cppcheck - see https://github.com/danmar/cppcheck/blob/b61feaf77fdfe306034257e062b7b7ebc1ff25a3/.github/workflows/selfcheck.yml#L97.

[matt335672]

BTW the pixman files are an oddity. My understanding is they're copied from an old version of the pixman library and used if for some reason the user chooses the --disable-pixman compile-time option.

[firewave]

BTW the pixman files are an oddity. My understanding is they're copied from an old version of the pixman library and used if for some reason the user chooses the --disable-pixman compile-time option.

Which appears to be the default otherwise those warnings would have not shown up in my local or the CI builds. Or the files are not excluded from the build if they are not actually used.

[matt335672]

Yes you're right - disable is the default. Most if not all distros use --enable-pixman as they ship the pixman library anyway.

[firewave]

Yes you're right - disable is the default. Most if not all distros use --enable-pixman as they ship the pixman library anyway.

Maybe the next release could switch that around so that the code could even be deprecated. Feels kinda weird that a self-spun version differs from most of the otherwise available ones by default.

[firewave]

See https://github.com/neutrinolabs/libpainter/pull/21 and https://github.com/neutrinolabs/librfxcodec/pull/59 for upstream fixes.

[matt335672]

--- a/xrdp/Makefile.am
+++ b/xrdp/Makefile.am
@@ -56,6 +56,7 @@ xrdp_SOURCES = \
   xrdp_listen.c \
   xrdp_login_wnd.c \
   xrdp_mm.c \
+  xrdp_mm.h \
   xrdp_painter.c \
   xrdp_process.c \
   xrdp_region.c \

[firewave]

I finally figured out how to update the submodules so finally here is a rebase.

[firewave]

I think I have addressed everything noted except for the following.

Build also fails with --enable-neutrinordp option. To get this option to work you'll need to build NeutrinoRDP:-

[firewave]

libpainter submodule still hasn't been updated yet although it was suggested otherwise.

[matt335672]

Sorry - that appears to be a drop-off on my part.

#3042 is running through CI at the moment. Assuming it's successful I'll merge it, and then if you rebase we should be OK.

Also, here's a patch for neutrinordp. I've also attached it below as a file if you just want to apply it.

patch.txt

diff --git a/neutrinordp/xrdp-color.c b/neutrinordp/xrdp-color.c
index b0f94c7c..b93bebb3 100644
--- a/neutrinordp/xrdp-color.c
+++ b/neutrinordp/xrdp-color.c
@@ -27,6 +27,8 @@
 #include "os_calls.h"
 #include "string_calls.h"
 
+#include "xrdp-color.h"
+
 char *
 convert_bitmap(int in_bpp, int out_bpp, char *bmpdata,
                int width, int height, int *palette)
diff --git a/neutrinordp/xrdp-neutrinordp.c b/neutrinordp/xrdp-neutrinordp.c
index 6574c3fa..b33bd042 100644
--- a/neutrinordp/xrdp-neutrinordp.c
+++ b/neutrinordp/xrdp-neutrinordp.c
@@ -2041,7 +2041,7 @@ lfreerdp_pre_connect(freerdp *instance)
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_WindowCreate(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
                    WINDOW_STATE_ORDER *window_state)
 {
@@ -2122,7 +2122,7 @@ lrail_WindowCreate(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_WindowUpdate(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
                    WINDOW_STATE_ORDER *window_state)
 {
@@ -2131,7 +2131,7 @@ lrail_WindowUpdate(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_WindowDelete(rdpContext *context, WINDOW_ORDER_INFO *orderInfo)
 {
     struct mod *mod;
@@ -2142,7 +2142,7 @@ lrail_WindowDelete(rdpContext *context, WINDOW_ORDER_INFO *orderInfo)
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_WindowIcon(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
                  WINDOW_ICON_ORDER *window_icon)
 {
@@ -2168,7 +2168,7 @@ lrail_WindowIcon(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_WindowCachedIcon(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
                        WINDOW_CACHED_ICON_ORDER *window_cached_icon)
 {
@@ -2183,7 +2183,7 @@ lrail_WindowCachedIcon(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_NotifyIconCreate(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
                        NOTIFY_ICON_STATE_ORDER *notify_icon_state)
 {
@@ -2240,7 +2240,7 @@ lrail_NotifyIconCreate(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_NotifyIconUpdate(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
                        NOTIFY_ICON_STATE_ORDER *notify_icon_state)
 {
@@ -2249,7 +2249,7 @@ lrail_NotifyIconUpdate(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_NotifyIconDelete(rdpContext *context, WINDOW_ORDER_INFO *orderInfo)
 {
     struct mod *mod;
@@ -2261,7 +2261,7 @@ lrail_NotifyIconDelete(rdpContext *context, WINDOW_ORDER_INFO *orderInfo)
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_MonitoredDesktop(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
                        MONITORED_DESKTOP_ORDER *monitored_desktop)
 {
@@ -2293,7 +2293,7 @@ lrail_MonitoredDesktop(rdpContext *context, WINDOW_ORDER_INFO *orderInfo,
 }
 
 /*****************************************************************************/
-void
+static void
 lrail_NonMonitoredDesktop(rdpContext *context, WINDOW_ORDER_INFO *orderInfo)
 {
     struct mod *mod;

[matt335672]

#3042 is now merged.

[firewave]

#3042 is now merged.

Great.

Also, here's a patch for neutrinordp. I've also attached it below as a file if you just want to apply it.

Thanks a lot! I integrated it.

[firewave]

Somehow the dependency installation is extremely slow. But it also looks like GitHub is not properly refreshing so it might be a general issue.

[matt335672]

The runners aren't updating properly at all at the moment.

I'm happy to merge this now, but I'll wait until the inevitable checks are likely to succeed.

[matt335672]

Ref the CI problems:-

microsoft/linux-package-repositories#130

[matt335672]

Thanks for your contribution @firewave

[firewave]

Thanks for your contribution @firewave

You're welcome. I hope the next one is going smoother.