We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 1646

Summary: Warnings from clang with -Weverything
Product: SDL Reporter: Robin Kaup <k.robin64>
Component: *don't know*Assignee: Ryan C. Gordon <icculus>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: trivial    
Priority: P2 CC: sylvain.becker
Version: HG 2.0   
Hardware: x86_64   
OS: Linux   
Attachments: Warnings coming from SDL headers
patch
patch 2
list of remaining constants
patch 3
patch warning documentation
patch warning padding
patch warning reserved macro id
Android patch

Description Robin Kaup 2012-11-17 03:12:07 UTC
Created attachment 992 [details]
Warnings coming from SDL headers

Compiling code using SDL 2 with clang in C++ mode with the -Weverything flag produces a long list of warnings. Some of these relate to missing macros, like __ANDROID__ on a system that isn't Android. Many of them relate to padding structs, and to implicit signedness conversion. These warnings should not show up when I'm not compiling SDL itself, but they do because my SDL headers are not in the system include directory and do not contain any preprocessor directives to tell the compiler to suppress those warnings.
Comment 1 Sam Lantinga 2012-11-22 11:57:22 UTC
A bunch of warnings were fixed in this commit:
http://hg.libsdl.org/SDL/rev/b1124b32b07d

I'm not seeing the padding warnings here.  Do you have a specific alignment set in your compile environment?

Can you paste the full clang command line?

Thanks!
Comment 2 Robin Kaup 2012-11-22 13:12:44 UTC
Hmm, I've been trying to come up with a minimal test case for this. It seems that clang does not come up with the warnings unless you actually use those structs. This works for me:

% cat main.c
#include <SDL.h>

int main(int argc, char** argv)
{
    SDL_Event ev;
    return EXIT_SUCCESS;
}

% /home/robin/local/bin/sdl2-config --cflags
-I/home/robin/local/include/SDL2 -D_REENTRANT
% clang `/home/robin/local/bin/sdl2-config --cflags` -Wpadded main.c
In file included from main.c:1:
In file included from /home/robin/local/include/SDL2/SDL.h:81:
In file included from /home/robin/local/include/SDL2/SDL_events.h:34:
/home/robin/local/include/SDL2/SDL_keyboard.h:52:12: warning: padding struct 'struct SDL_Keysym' with 2 bytes to align 'unicode' [-Wpadded]
    Uint32 unicode;             /**< \deprecated use SDL_TextInputEvent instead */
           ^
In file included from main.c:1:
In file included from /home/robin/local/include/SDL2/SDL.h:81:
/home/robin/local/include/SDL2/SDL_events.h:298:17: warning: padding struct 'struct SDL_TouchFingerEvent' with 4 bytes to align 'touchId' [-Wpadded]
    SDL_TouchID touchId;        /**< The touch device id */
                ^
/home/robin/local/include/SDL2/SDL_events.h:292:16: warning: padding size of 'struct SDL_TouchFingerEvent' with 2 bytes to alignment boundary [-Wpadded]
typedef struct SDL_TouchFingerEvent
               ^
/home/robin/local/include/SDL2/SDL_events.h:320:17: warning: padding struct 'struct SDL_TouchButtonEvent' with 4 bytes to align 'touchId' [-Wpadded]
    SDL_TouchID touchId;        /**< The touch device index */
                ^
/home/robin/local/include/SDL2/SDL_events.h:315:16: warning: padding size of 'struct SDL_TouchButtonEvent' with 4 bytes to alignment boundary [-Wpadded]
typedef struct SDL_TouchButtonEvent
               ^
/home/robin/local/include/SDL2/SDL_events.h:336:17: warning: padding struct 'struct SDL_MultiGestureEvent' with 4 bytes to align 'touchId' [-Wpadded]
    SDL_TouchID touchId;        /**< The touch device index */
                ^
/home/robin/local/include/SDL2/SDL_events.h:331:16: warning: padding size of 'struct SDL_MultiGestureEvent' with 4 bytes to alignment boundary [-Wpadded]
typedef struct SDL_MultiGestureEvent
               ^
/home/robin/local/include/SDL2/SDL_events.h:351:17: warning: padding struct 'struct SDL_DollarGestureEvent' with 4 bytes to align 'touchId' [-Wpadded]
    SDL_TouchID touchId;        /**< The touch device index */
                ^
8 warnings generated.


(In reply to comment #1)
> A bunch of warnings were fixed in this commit:
> http://hg.libsdl.org/SDL/rev/b1124b32b07d
> 
> I'm not seeing the padding warnings here.  Do you have a specific alignment set
> in your compile environment?
> 
> Can you paste the full clang command line?
> 
> Thanks!
Comment 3 Sylvain 2016-11-13 17:40:57 UTC
Created attachment 2619 [details]
patch

Here's a patch to fix minor warnings around the whole code.
Comment 4 Sylvain 2016-11-13 17:50:23 UTC
Then, there are also a two "-Wassign-enum" warnings that might need attention: 


1) 

src/audio/pulseaudio/SDL_pulseaudio.c:706:84: warning: integer constant not in range of enumerated type 'pa_subscription_mask_t' (aka 'enum pa_subscription_mask')      [-Wassign-enum]
    o = PULSEAUDIO_pa_context_subscribe(hotplug_context, PA_SUBSCRIPTION_MASK_SINK | PA_SUBSCRIPTION_MASK_SOURCE, NULL, NULL);

The "PA_SUBSCRIPTION_MASK_SINK | PA_SUBSCRIPTION_MASK_SOURCE" might be incorrect. It cannot be "bitwised"



2)

src/video/SDL_blit_N.c:2474:40: warning: integer constant not in range of enumerated type 'enum (anonymous enum at src/video/SDL_blit_N.c:2469:5)' [-Wassign-enum]
   {0, 0, 0, 0, 0, 0, 0, 0, BlitNtoN, 0}

"struct blit_table" has a field "enum { NO_ALPHA = 1, SET_ALPHA = 2, COPY_ALPHA = 4 } alpha;"
it cannot be set to "0" neither set to a bitwise like "NO_ALPHA | COPY_ALPHA | SET_ALPHA".
Comment 5 Sam Lantinga 2016-11-14 07:01:54 UTC
The patch is in:
https://hg.libsdl.org/SDL/rev/d702ecbd8ba7

The patch needs to be reviewed and some of the constants should be removed instead of commented out, but thanks!
Comment 6 Sylvain 2016-11-14 08:03:18 UTC
Created attachment 2620 [details]
patch 2

Great! so here's the commented constants (and some macros) deleted.
Comment 7 Sylvain 2016-11-14 08:07:05 UTC
Created attachment 2621 [details]
list of remaining constants

Also, the list of others commented constant that I hadn't modified with the first patch.
Comment 8 Sylvain 2016-11-14 13:58:57 UTC
Created attachment 2625 [details]
patch 3

some more warnings removed (Including the previous reported (1)). The pulseaudio is also probably.
Comment 9 Sylvain 2016-11-14 13:59:24 UTC
The pulseaudio is also probably safe.
Comment 10 Sam Lantinga 2016-11-15 09:30:51 UTC
Thanks Sylvain!

Are the SDL API headers clean at this point?
Comment 11 Sylvain 2016-11-15 11:58:40 UTC
Almost done for the external headers.
(For the source code, it's nearly impossible to compile with -Weverything without warning :))

I will provide three patches.
Comment 12 Sylvain 2016-11-15 12:00:44 UTC
Created attachment 2628 [details]
patch warning documentation

A few warnings that shows up with -Wdocumentation and -Wdocumentation-unknown-command
Comment 13 Sylvain 2016-11-15 12:04:36 UTC
Created attachment 2629 [details]
patch warning padding

The patch for the padding warnings (-Wpadded). It explicitly sets the padding in the structure.

In "SDL_audio.h" there is a "packed" attribute that is used and could be done the same way (It would also remove some warnings).
Comment 14 Sylvain 2016-11-15 12:07:09 UTC
Created attachment 2630 [details]
patch warning reserved macro id

Renaming of guard header names to quiet -Wreserved-id-macro
Comment 15 Sylvain 2016-11-15 13:16:36 UTC
Actually, ignore the patch "patch for the padding warnings". 
It fixes only a few structures (the ones in my test program).

If done, it should be done for all structures, by:
- optionally, moving pointers to the beginning of the structure
- adding an explicit dummy padding.

Currently, it seems to me it would pollute the source code of the library.

Since almost all structure are not padded, It may not be necessary to pack "SDL_AudioCVT" structure in "SDL_audio.h". Or explicitly pad everything.

--- a/include/SDL_audio.h	Tue Nov 15 01:30:08 2016 -0800
+++ b/include/SDL_audio.h	Tue Nov 15 14:13:14 2016 +0100
@@ -186,17 +186,6 @@
 /**
  *  A structure to hold a set of audio conversion filters and buffers.
  */
-#ifdef __GNUC__
-/* This structure is 84 bytes on 32-bit architectures, make sure GCC doesn't
-   pad it out to 88 bytes to guarantee ABI compatibility between compilers.
-   vvv
-   The next time we rev the ABI, make sure to size the ints and add padding.
-*/
-#define SDL_AUDIOCVT_PACKED __attribute__((packed))
-#else
-#define SDL_AUDIOCVT_PACKED
-#endif
-/* */
 typedef struct SDL_AudioCVT
 {
     int needed;                 /**< Set to 1 if conversion possible */
@@ -210,7 +199,7 @@
     double len_ratio;           /**< Given len, final size is len*len_ratio */
     SDL_AudioFilter filters[10];        /**< Filter list */
     int filter_index;           /**< Current audio conversion function */
-} SDL_AUDIOCVT_PACKED SDL_AudioCVT;
+} SDL_AudioCVT;
Comment 16 Sylvain 2016-11-17 10:53:19 UTC
One minor warning patch of SDL2 compilation under mingw
(see https://buildbot.libsdl.org/builders/sdl-mingw-x86/builds/2967/steps/compile/logs/stdio)

diff -r d17dd08640a4 src/SDL_log.c
--- a/src/SDL_log.c	Tue Nov 15 01:30:08 2016 -0800
+++ b/src/SDL_log.c	Thu Nov 17 11:51:56 2016 +0100
@@ -305,11 +305,13 @@
 }
 
 #if defined(__WIN32__)
+# if !defined(HAVE_STDIO_H) && !defined(__WINRT__)
 /* Flag tracking the attachment of the console: 0=unattached, 1=attached, -1=error */
 static int consoleAttached = 0;
 
 /* Handle to stderr output of console. */
 static HANDLE stderrHandle = NULL;
+# endif
 #endif
 
 static void
Comment 17 Sylvain 2016-11-21 09:50:29 UTC
Some extra header renaming ...

--- a/SDL_image.h	Sun Nov 06 20:26:48 2016 -0800
+++ b/SDL_image.h	Mon Nov 21 10:46:48 2016 +0100
@@ -21,8 +21,8 @@
 
 /* A simple library to load images of various formats as SDL surfaces */
 
-#ifndef _SDL_IMAGE_H
-#define _SDL_IMAGE_H
+#ifndef SDL_IMAGE_H_
+#define SDL_IMAGE_H_
 
 #include "SDL.h"
 #include "SDL_version.h"
@@ -142,4 +142,4 @@
 #endif
 #include "close_code.h"
 
-#endif /* _SDL_IMAGE_H */
+#endif /* SDL_IMAGE_H_ */


--- a/SDL_mixer.h	Sun Nov 06 20:26:48 2016 -0800
+++ b/SDL_mixer.h	Mon Nov 21 10:47:10 2016 +0100
@@ -21,8 +21,8 @@
 
 /* $Id$ */
 
-#ifndef _SDL_MIXER_H
-#define _SDL_MIXER_H
+#ifndef SDL_MIXER_H_
+#define SDL_MIXER_H_
 
 #include "SDL_stdinc.h"
 #include "SDL_rwops.h"
@@ -631,4 +631,4 @@
 #endif
 #include "close_code.h"
 
-#endif /* _SDL_MIXER_H */
+#endif /* SDL_MIXER_H_ */


--- a/SDL_ttf.h	Sun Nov 06 20:26:48 2016 -0800
+++ b/SDL_ttf.h	Mon Nov 21 10:47:53 2016 +0100
@@ -28,8 +28,8 @@
    Unicode is hard, we learn as we go, and we apologize for adding to the
    confusion. */
 
-#ifndef _SDL_TTF_H
-#define _SDL_TTF_H
+#ifndef SDL_TTF_H_
+#define SDL_TTF_H_
 
 #include "SDL.h"
 #include "begin_code.h"
@@ -277,4 +277,4 @@
 #endif
 #include "close_code.h"
 
-#endif /* _SDL_TTF_H */
+#endif /* SDL_TTF_H_ */
Comment 18 Sylvain 2016-11-21 10:06:23 UTC
So currently, when compiling a source code, including SDL2 headers and -Weverything, 

There are only, at least, those warnings on linux:


SDL2/SDL_platform.h:60:8: warning: macro name is a reserved identifier [-Wreserved-id-macro]
#undef __LINUX__
#define __LINUX__   1


SDL2/SDL_audio.h:202:9: warning: packed attribute is unnecessary for 'needed' [-Wpacked]
    int needed;                 /**< Set to 1 if conversion possible */ 
    and so one for the "struct SDL_AudioCVT"


 include/SDL2/SDL_video.h:34:
../../../release/Release_SDL/build/include/SDL2/SDL_surface.h:72:22: warning: padding struct 'struct SDL_Surface' with 4 bytes to align 'format' [-Wpadded]
    SDL_PixelFormat *format;    /**< Read-only *


(The "-Wpacked" and "-Wpadded" appears when the struct explicitly used in the code.)
Comment 19 Sylvain 2016-11-21 20:25:14 UTC
Created attachment 2638 [details]
Android patch

Patch to fix missing prototypes on Android.
+ use the helper JNI macro from bug 2393
Comment 20 Sam Lantinga 2016-12-02 10:25:37 UTC
Got the Android patch, thanks!
https://hg.libsdl.org/SDL/rev/10fb0ebc4fbf
Comment 21 Sam Lantinga 2016-12-02 10:30:51 UTC
I'm going to go ahead and close this bug. Please open new bugs for additional patches.

Thanks!