| 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 |
||
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! 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!
Created attachment 2619 [details]
patch
Here's a patch to fix minor warnings around the whole code.
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".
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! Created attachment 2620 [details]
patch 2
Great! so here's the commented constants (and some macros) deleted.
Created attachment 2621 [details]
list of remaining constants
Also, the list of others commented constant that I hadn't modified with the first patch.
Created attachment 2625 [details]
patch 3
some more warnings removed (Including the previous reported (1)). The pulseaudio is also probably.
The pulseaudio is also probably safe. Thanks Sylvain! Are the SDL API headers clean at this point? 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. Created attachment 2628 [details]
patch warning documentation
A few warnings that shows up with -Wdocumentation and -Wdocumentation-unknown-command
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).
Created attachment 2630 [details]
patch warning reserved macro id
Renaming of guard header names to quiet -Wreserved-id-macro
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;
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 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_ */
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.)
Created attachment 2638 [details] Android patch Patch to fix missing prototypes on Android. + use the helper JNI macro from bug 2393 Got the Android patch, thanks! https://hg.libsdl.org/SDL/rev/10fb0ebc4fbf I'm going to go ahead and close this bug. Please open new bugs for additional patches. Thanks! |
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.