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

firewave · 2023-10-17T13:47:10Z

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 · 2023-10-17T13:51:11Z

I can still squash this if desired.

firewave · 2023-10-17T21:13:19Z

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 · 2023-10-18T08:43:22Z

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 · 2023-10-18T09:23:10Z

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 · 2023-10-19T10:58:09Z

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 · 2023-10-19T11:02:16Z

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 · 2023-10-19T12:46:20Z

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 · 2023-10-20T14:46:48Z

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

matt335672 · 2023-10-20T15:03:16Z

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 · 2023-10-20T15:24:28Z

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 · 2023-10-20T15:34:27Z

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

Largely happy with this. Just one comment.

matt335672 · 2023-10-20T15:50:55Z

configure.ac

@@ -205,6 +205,8 @@ 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])
+AX_APPEND_COMPILE_FLAGS([-Wno-error=missing-prototypes], ,[-Werror])


Don't understand the 2nd line here. What is this doing?

That tells the compiler not to treat that warning as an error as I still haven't fixed all of them. As it is just two functions I might still do it in this PR.

BTW that is also like the remaining compiler warnings on BSD should be handled a few lines below to make sure none of a different type are being introduced.

If it's being removed later, I'm fine with that - thanks.

Yeah. Any such things are (hopefully) temporary. Such things are always fidgety if you apply them to a "legacy" code base.

These lines can be removed now I think.

Still required until the latest libpainter and librfxcodec are being used.

Those changes have been merged.

You can add a commit to your PR which pulls in the latest versions:-

cd libpainter git checkout devel git pull cd ../librfxcodec git checkout devel git pull cd .. git add libpainter librfxcodec git commit -m "Update submodules for -Wmissing-prototypes"

Or something like that. You get the idea.

matt335672 · 2023-10-20T15:58:18Z

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 · 2023-10-20T16:01:27Z

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 · 2023-10-20T16:16:35Z

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

firewave · 2023-10-20T16:22:34Z

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 · 2023-10-28T20:23:51Z

See neutrinolabs/libpainter#21 and neutrinolabs/librfxcodec#59 for upstream fixes.

matt335672 · 2023-10-31T10:12:29Z

--- 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 \

matt335672

Thanks for this @firewave. Two in-line comments below and one general one here:-

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

https://github.com/neutrinolabs/xrdp/wiki/NeutrinoRDP-proxy-module-for-xrdp

Gimme a shout if you need a hand with that.

matt335672 · 2023-11-01T10:34:44Z

configure.ac

@@ -205,6 +205,8 @@ 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])
+AX_APPEND_COMPILE_FLAGS([-Wno-error=missing-prototypes], ,[-Werror])


These lines can be removed now I think.

matt335672 · 2023-11-01T10:37:57Z

common/pixman-region.c

-
-PIXMAN_EXPORT pixman_bool_t
-PREFIX (_selfcheck) (region_type_t *reg)
-{


This function is used if building with --enable-devel-logging or --eanable-devel-all (and without --enable-pixman). Building with these options now gives me a link error.

I thought I did a full tree search for all function before removing them. Must have been my recent sloppiness. Will add back.

Looks like I already addressed this.

firewave · 2024-04-22T14:18:01Z

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

firewave · 2024-04-22T14:27:01Z

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 · 2024-04-22T14:37:20Z

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

matt335672 · 2024-04-23T11:22:37Z

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 · 2024-04-23T11:36:12Z

#3042 is now merged.

Co-authored-by: matt335672 <[email protected]>

Co-authored-by: matt335672 <[email protected]>

firewave · 2024-04-23T16:46:50Z

#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 · 2024-04-23T17:19:09Z

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 · 2024-04-24T09:16:30Z

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 · 2024-04-24T10:18:31Z

Ref the CI problems:-

microsoft/linux-package-repositories#130

matt335672 · 2024-04-24T14:50:15Z

Thanks for your contribution @firewave

firewave · 2024-04-24T16:52:26Z

Thanks for your contribution @firewave

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

matt335672 mentioned this pull request Oct 20, 2023

enabled and mitigated -Wdocumentation and -Wdocumentation-unknown-command Clang compiler warnings #2831

Draft

firewave force-pushed the missing-prototypes branch from 49e5cec to 926cadc Compare October 20, 2023 15:21

firewave force-pushed the missing-prototypes branch from 926cadc to d66fffe Compare October 20, 2023 15:26

firewave changed the title ~~fixed some -Wmissing-prototypes Clang compiler warnings~~ fixed some -Wmissing-prototypes (and related) compiler warnings Oct 20, 2023

firewave marked this pull request as draft October 20, 2023 15:47

matt335672 reviewed Oct 20, 2023

View reviewed changes

firewave force-pushed the missing-prototypes branch from d66fffe to 4ab82c8 Compare October 28, 2023 19:41

firewave changed the title ~~fixed some -Wmissing-prototypes (and related) compiler warnings~~ fixed -Wmissing-prototypes (and related) compiler warnings Oct 28, 2023

firewave force-pushed the missing-prototypes branch from 4ab82c8 to fc86aac Compare October 28, 2023 19:47

firewave changed the title ~~fixed -Wmissing-prototypes (and related) compiler warnings~~ fixed -Wmissing-prototypes (and subsequent) compiler warnings Oct 28, 2023

firewave force-pushed the missing-prototypes branch 4 times, most recently from 6c5d095 to ac28738 Compare October 28, 2023 20:18

firewave force-pushed the missing-prototypes branch 2 times, most recently from 9403afe to 058a015 Compare October 30, 2023 20:15

firewave force-pushed the missing-prototypes branch from 058a015 to 9c04a67 Compare October 31, 2023 11:50

firewave marked this pull request as ready for review October 31, 2023 11:54

matt335672 requested changes Nov 1, 2023

View reviewed changes

matt335672 mentioned this pull request Nov 1, 2023

Improve Unicode support #2794

Merged

firewave force-pushed the missing-prototypes branch from 9c04a67 to 8f068b5 Compare April 22, 2024 14:16

firewave force-pushed the missing-prototypes branch from 8f068b5 to 7b94704 Compare April 22, 2024 14:26

firewave and others added 3 commits April 23, 2024 18:38

enabled and fixed -Wmissing-prototypes compiler warnings

fb9c175

Co-authored-by: matt335672 <[email protected]>

fixed -Wunused-function compiler warnings

5eed76f

firewave force-pushed the missing-prototypes branch from 7b94704 to 5eed76f Compare April 23, 2024 16:38

xrdp/neutrinordp: fixed -Wmissing-prototypes compiler warning

c7070b9

Co-authored-by: matt335672 <[email protected]>

matt335672 approved these changes Apr 24, 2024

View reviewed changes

matt335672 merged commit 15b46f8 into neutrinolabs:devel Apr 24, 2024
13 checks passed

firewave deleted the missing-prototypes branch April 24, 2024 16:52

matt335672 mentioned this pull request May 2, 2024

Add ibus unicode input support #3055

Closed

fixed -Wmissing-prototypes (and subsequent) compiler warnings #2829

fixed -Wmissing-prototypes (and subsequent) compiler warnings #2829

Conversation

firewave commented Oct 17, 2023

firewave commented Oct 17, 2023

firewave commented Oct 17, 2023

matt335672 commented Oct 18, 2023

firewave commented Oct 18, 2023

matt335672 commented Oct 19, 2023

matt335672 commented Oct 19, 2023

firewave commented Oct 19, 2023

firewave commented Oct 20, 2023 • edited Loading

matt335672 commented Oct 20, 2023

firewave commented Oct 20, 2023

firewave commented Oct 20, 2023

matt335672 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt335672 commented Oct 20, 2023

firewave commented Oct 20, 2023

matt335672 commented Oct 20, 2023

firewave commented Oct 20, 2023

firewave commented Oct 28, 2023 • edited Loading

matt335672 commented Oct 31, 2023

matt335672 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firewave commented Apr 22, 2024

firewave commented Apr 22, 2024

firewave commented Apr 22, 2024

matt335672 commented Apr 23, 2024

matt335672 commented Apr 23, 2024

firewave commented Apr 23, 2024

firewave commented Apr 23, 2024

matt335672 commented Apr 24, 2024

matt335672 commented Apr 24, 2024

matt335672 commented Apr 24, 2024

firewave commented Apr 24, 2024

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

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

firewave commented Oct 20, 2023 •

edited

Loading

firewave commented Oct 28, 2023 •

edited

Loading