Bug 67072

Summary: gcc 4.8.1 fixes
Product: dbus Reporter: Ralf Habacker <ralf.habacker>
Component: coreAssignee: Havoc Pennington <hp>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: fix of gcc 4.8.1 warnings
gcc 4.8.1 cross compiling warning fixes
Fixed gcc-4.8.1-Wformat warnings on windows
Generate autotools provided defines using a cmake macro

Description Ralf Habacker 2013-07-19 09:29:44 UTC
Created attachment 82668 [details] [review]
fix of gcc 4.8.1 warnings

gcc 4.8.1 warning fixes
Comment 1 Ralf Habacker 2013-07-19 09:30:29 UTC
Created attachment 82669 [details] [review]
gcc 4.8.1 cross compiling warning fixes
Comment 2 Simon McVittie 2013-08-27 13:21:24 UTC
Comment on attachment 82668 [details] [review]
fix of gcc 4.8.1 warnings

Review of attachment 82668 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-win.c
@@ +173,4 @@
>  
>    if ((result = GetExtendedTcpTable (tcp_table, &size, TRUE, AF_INET, TCP_TABLE_OWNER_PID_ALL, 0)) != NO_ERROR)
>      {
> +      _dbus_verbose ("Error fetching tcp table %d\n", (int)result);

If result is a DWORD, I'd prefer "... %lu\n", (unsigned long) result).

Trivial: space after cast, please.

I was going to suggest using _dbus_win_error_string(), but it seems that does a malloc or equivalent, which seems too complicated for a _dbus_verbose().

@@ +187,4 @@
>          result = p->dwOwningPid;
>      }
>  
> +  _dbus_verbose ("got pid %d\n", (int)result);

Similarly, "got pid %lu\n", (unsigned long) result) seems better? (DWORD is always unsigned 32-bit, which is no larger than unsigned long, if I understand the Windows type system correctly.)

@@ +882,4 @@
>  
>    if (!OpenProcessToken (process_handle, TOKEN_QUERY, &process_token))
>      {
> +      _dbus_verbose("OpenProcessToken failed with error %d", (int)GetLastError());

Same here

@@ +890,4 @@
>             || (token_user = alloca (n)) == NULL
>             || !GetTokenInformation (process_token, TokenUser, token_user, n, &n))
>      {
> +      _dbus_verbose("GetTokenInformation failed with error %d", (int)GetLastError ());

Same here

@@ +2131,4 @@
>    if (!CryptAcquireContext (&hprov, NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT))
>      return FALSE;
>  
> +  if (!CryptGenRandom (hprov, n_bytes, (BYTE *)p))

OK, fine.

(Trivial: space after cast please: (BYTE *) p)

@@ +2152,4 @@
>  _dbus_get_tmpdir(void)
>  {
>    static const char* tmpdir = NULL;
> +  static unsigned char buf[1000];

OK, but don't you need "tmpdir = (char *) buf" instead of "tmpdir = buf" now?

I notice this function isn't thread-safe... but that's something for another bug.

@@ +2156,4 @@
>  
>    if (tmpdir == NULL)
>      {
> +      unsigned char *last_slash;

OK.
Comment 3 Simon McVittie 2013-08-27 13:22:08 UTC
Comment on attachment 82669 [details] [review]
gcc 4.8.1 cross compiling warning fixes

Review of attachment 82669 [details] [review]:
-----------------------------------------------------------------

::: cmake/config.h.cmake
@@ -19,5 @@
>  #cmakedefine DBUS_MACHINE_UUID_FILE "@DBUS_MACHINE_UUID_FILE@"
>  #cmakedefine DBUS_DAEMONDIR "@DBUS_DAEMONDIR@"
>  #cmakedefine PACKAGE "@PACKAGE@"
> -/* Version number of package */
> -#cmakedefine DBUS_MAJOR_VERSION @DBUS_MAJOR_VERSION@

Where do these get defined, if not here?
Comment 4 Ralf Habacker 2013-08-27 13:36:48 UTC
(In reply to comment #3)
> Comment on attachment 82669 [details] [review] [review]
> gcc 4.8.1 cross compiling warning fixes
> 
> Review of attachment 82669 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Where do these get defined, if not here?

> >  #cmakedefine DBUS_MACHINE_UUID_FILE "@DBUS_MACHINE_UUID_FILE@"
tools dbus-launch.c -> unix only 
need to exclude from patch, otherwise it will break cmake linux build

> >  #cmakedefine DBUS_DAEMONDIR "@DBUS_DAEMONDIR@"
not used by any c file 

> >  #cmakedefine PACKAGE "@PACKAGE@"
not used by any c file

> > -/* Version number of package */
> > -#cmakedefine DBUS_MAJOR_VERSION @DBUS_MAJOR_VERSION@
is defined in dbus/dbus-sysdeps.h.in
Comment 5 Ralf Habacker 2013-08-27 13:44:01 UTC
(In reply to comment #2)
> Comment on attachment 82668 [details] [review] [review]
> fix of gcc 4.8.1 warnings
> 
> Review of attachment 82668 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-sysdeps-win.c
> @@ +173,4 @@
> >  
> >    if ((result = GetExtendedTcpTable (tcp_table, &size, TRUE, AF_INET, TCP_TABLE_OWNER_PID_ALL, 0)) != NO_ERROR)
> >      {
> > +      _dbus_verbose ("Error fetching tcp table %d\n", (int)result);
> 
> If result is a DWORD, I'd prefer "... %lu\n", (unsigned long) result).
> 
Because DWORD is already defined unsigned long 

/usr/i686-w64-mingw32/sys-root/mingw/include/mqoai.h:371:  typedef unsigned __LONG32 DWORD;
/usr/i686-w64-mingw32/sys-root/mingw/include/mapidbg.h:36:typedef unsigned __LONG32 DWORD;
/usr/i686-w64-mingw32/sys-root/mingw/include/mapinls.h:27:  typedef unsigned __LONG32 DWORD;
/usr/i686-w64-mingw32/sys-root/mingw/include/winsmcrd.h:29:  typedef ULONG DWORD;
/usr/i686-w64-mingw32/sys-root/mingw/include/ddk/ntstrsafe.h:32:typedef ULONG DWORD;
/usr/i686-w64-mingw32/sys-root/mingw/include/wtypes.h:74:typedef ULONG DWORD;
/usr/i686-w64-mingw32/sys-root/mingw/include/windef.h:117:typedef unsigned __LONG32 DWORD;

it would be enough to use  prefer "... %lu\n", result)  i guess ?
Comment 6 Ralf Habacker 2013-08-27 13:51:34 UTC
(In reply to comment #2)
> Comment on attachment 82668 [details] [review] [review]
> fix of gcc 4.8.1 warnings
> 
> Review of attachment 82668 [details] [review] [review]:
> -----------------------------------------------------------------

> @@ +2152,4 @@
> >  _dbus_get_tmpdir(void)
> >  {
> >    static const char* tmpdir = NULL;
> > +  static unsigned char buf[1000];
> 
> OK, but don't you need "tmpdir = (char *) buf" instead of "tmpdir = buf" now?
> 
not sure, I only changed the types as the compiler suggested.

> I notice this function isn't thread-safe... but that's something for another
> bug.
Which will probably refactor this lines too, so i will remove them from this patch.
Comment 7 Ralf Habacker 2013-08-27 13:59:57 UTC
(In reply to comment #2)
> Comment on attachment 82668 [details] [review] [review]
> fix of gcc 4.8.1 warnings
> 
> Review of attachment 82668 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I was going to suggest using _dbus_win_error_string(), but it seems that
> does a malloc or equivalent, which seems too complicated for a
> _dbus_verbose().
_dbus_win_warn_win_error requires a type cast too, 

void _dbus_win_warn_win_error (const char *message, int code)

I suggest to add a generic printf style like _dbus_error() and/or dbus_warning() function. May also help on other places
Comment 8 Ralf Habacker 2013-08-27 14:02:53 UTC
(In reply to comment #7)
> I suggest to add a generic printf style like _dbus_error() and/or
> dbus_warning() function. May also help on other places

Update: dbus_warn() is also present, dbus_error() not.
Comment 9 Simon McVittie 2013-08-27 14:06:44 UTC
(In reply to comment #5)
> > If result is a DWORD, I'd prefer "... %lu\n", (unsigned long) result).
> > 
> Because DWORD is already defined unsigned long 
...
> it would be enough to use  prefer "... %lu\n", result)  i guess ?

I thought you said on another bug that __LONG32 is sometimes int?

If DWORD is always *the same size as* unsigned long (as opposed to *no larger than* unsigned long), then I'm fine with printf'ing it as a %lu with no cast.

If DWORD could have a size differing from unsigned long, then you still need to cast it. I doubt Microsoft is particularly likely to release a Windows version that's LP64 (32-bit int, 64-bit long and pointer - like x86-64 Linux) since they seem to have settled on LLP64 (32-bit int and long, 64-bit "long long" and pointer) instead, but you never know.
Comment 10 Simon McVittie 2013-08-27 14:07:58 UTC
(In reply to comment #7)
> _dbus_win_warn_win_error requires a type cast too, 
> 
> void _dbus_win_warn_win_error (const char *message, int code)

Yeah, that second parameter should be DWORD, if the intention is "takes a Windows system error code, as if from GetLastError()".
Comment 11 Ralf Habacker 2013-08-27 14:09:19 UTC
(In reply to comment #10)
> (In reply to comment #7)
> > _dbus_win_warn_win_error requires a type cast too, 
> > 
> > void _dbus_win_warn_win_error (const char *message, int code)
> 
> Yeah, that second parameter should be DWORD, if the intention is "takes a
> Windows system error code, as if from GetLastError()".

and it does not support printf style parameter list. :-(
Comment 12 Ralf Habacker 2013-08-27 14:18:28 UTC
(In reply to comment #9)
> (In reply to comment #5)
> > > If result is a DWORD, I'd prefer "... %lu\n", (unsigned long) result).
> > > 
> > Because DWORD is already defined unsigned long 
> ...
> > it would be enough to use  prefer "... %lu\n", result)  i guess ?
> 
> I thought you said on another bug that __LONG32 is sometimes int?
> 
> If DWORD is always *the same size as* unsigned long (as opposed to *no
> larger than* unsigned long), then I'm fine with printf'ing it as a %lu with
> no cast.

At https://bugs.freedesktop.org/show_bug.cgi?id=61874#c5 there is mentioned that on 64 bit cygwin systems long is 64 bit, so int is taken which is 32bit on that platform, but DWORD is always unsigned "32bits"

> 
> If DWORD could have a size differing from unsigned long, then you still need
> to cast it. 
sure, but there is not GetLastWin64Error or similar. 

>I doubt Microsoft is particularly likely to release a Windows
> version that's LP64 (32-bit int, 64-bit long and pointer - like x86-64
> Linux) 

or cygwin 64bit as mentioned above
Comment 13 Simon McVittie 2013-08-27 14:22:45 UTC
(In reply to comment #12)
> At https://bugs.freedesktop.org/show_bug.cgi?id=61874#c5 there is mentioned
> that on 64 bit cygwin systems long is 64 bit, so int is taken which is 32bit
> on that platform, but DWORD is always unsigned "32bits"

Ah, I had half-remembered that there was some sort of counter-example.

If we use dbus-sysdeps-win on Cygwin (do we? it isn't clear to me...) then yes I'm afraid it will need the (unsigned long) cast.
Comment 14 Ralf Habacker 2013-08-27 15:34:53 UTC
Created attachment 84720 [details] [review]
Fixed gcc-4.8.1-Wformat warnings on windows
Comment 15 Ralf Habacker 2013-08-27 15:36:03 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > At https://bugs.freedesktop.org/show_bug.cgi?id=61874#c5 there is mentioned
> > that on 64 bit cygwin systems long is 64 bit, so int is taken which is 32bit
> > on that platform, but DWORD is always unsigned "32bits"
> 
> Ah, I had half-remembered that there was some sort of counter-example.
> 
> If we use dbus-sysdeps-win on Cygwin (do we? it isn't clear to me...) 
no, cygwin is unix like.
Comment 16 Simon McVittie 2013-08-27 15:42:48 UTC
Comment on attachment 84720 [details] [review]
Fixed gcc-4.8.1-Wformat warnings on windows

Review of attachment 84720 [details] [review]:
-----------------------------------------------------------------

Sure, looks good, since you said Cygwin doesn't use these files.
Comment 17 Ralf Habacker 2013-08-28 08:52:34 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 82669 [details] [review] [review] [review]
> > gcc 4.8.1 cross compiling warning fixes
> > 
> > Review of attachment 82669 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > Where do these get defined, if not here?
> 
> > >  #cmakedefine DBUS_MACHINE_UUID_FILE "@DBUS_MACHINE_UUID_FILE@"
> tools dbus-launch.c -> unix only 
> need to exclude from patch, otherwise it will break cmake linux build
> 
> > >  #cmakedefine DBUS_DAEMONDIR "@DBUS_DAEMONDIR@"
> not used by any c file 
> 
> > >  #cmakedefine PACKAGE "@PACKAGE@"
> not used by any c file
> 
> > > -/* Version number of package */
> > > -#cmakedefine DBUS_MAJOR_VERSION @DBUS_MAJOR_VERSION@
> is defined in dbus/dbus-sysdeps.h.in

forget the comment about the first three entries 

>  #cmakedefine DBUS_MACHINE_UUID_FILE "@DBUS_MACHINE_UUID_FILE@"
>  #cmakedefine DBUS_DAEMONDIR "@DBUS_DAEMONDIR@"
>  #cmakedefine PACKAGE "@PACKAGE@"

they are not changed. Only the version related defines has been changed  

-/* Version number of package */
-#cmakedefine DBUS_MAJOR_VERSION @DBUS_MAJOR_VERSION@
-#cmakedefine DBUS_MINOR_VERSION @DBUS_MINOR_VERSION@
-#cmakedefine DBUS_MICRO_VERSION @DBUS_MICRO_VERSION@
-#cmakedefine DBUS_VERSION ((@DBUS_MAJOR_VERSION@ << 16) | (@DBUS_MINOR_VERSION@ << 8) | (@DBUS_MICRO_VERSION@))
-#cmakedefine DBUS_VERSION_STRING "@DBUS_VERSION_STRING@"
-#cmakedefine DBUS_ENABLE_STATS
 
-#define VERSION DBUS_VERSION_STRING
+#cmakedefine DBUS_ENABLE_STATS


which are defined in dbus/dbus-sysdeps.h.in. Any more problems ?
Comment 18 Ralf Habacker 2013-08-28 08:53:02 UTC
Comment on attachment 84720 [details] [review]
Fixed gcc-4.8.1-Wformat warnings on windows

applied to master
Comment 19 Simon McVittie 2013-08-28 09:16:21 UTC
(In reply to comment #17)
> which are defined in dbus/dbus-sysdeps.h.in

Where do you get that file? I don't see one in git master.
Comment 20 Ralf Habacker 2013-08-28 09:27:23 UTC
(In reply to comment #19)
> (In reply to comment #17)
> > which are defined in dbus/dbus-sysdeps.h.in
> 
> Where do you get that file? I don't see one in git master.

s/sys/arch-/g
Comment 21 Simon McVittie 2013-08-28 09:51:54 UTC
Comment on attachment 82669 [details] [review]
gcc 4.8.1 cross compiling warning fixes

Review of attachment 82669 [details] [review]:
-----------------------------------------------------------------

(In reply to comment #20)
> s/sys/arch-/g

Ah, that makes more sense :-)

The DBUS_*VERSION* stuff can be deleted, but you need to either keep VERSION expanding to DBUS_VERSION_STRING, or replace all its uses (which seem to be in tools/*.c - use git grep for '\<VERSION\>').

As a general principle, things should be (conditionally) defined in config.h.cmake if and only if they are (conditionally) AC_DEFINE'd in configure.ac (which results in them going in config.h.in and hence config.h). However, the Autotools do define a few things automatically, such as PACKAGE and VERSION, so not everything is immediately visible.
Comment 22 Simon McVittie 2013-08-28 09:53:38 UTC
(In reply to comment #21)
> either keep VERSION
> expanding to DBUS_VERSION_STRING, or replace all its uses

... or having VERSION expand to "@DBUS_VERSION_STRING@" directly would be fine too - that's the closest equivalent of what the Autotools do (VERSION expands to "1.7.5" or whatever, without going via an intermediate macro).
Comment 23 Ralf Habacker 2013-08-28 12:21:29 UTC
Created attachment 84788 [details] [review]
Generate autotools provided defines using a cmake macro

I was inpirated by your last comments to refactor generating of VERSION and PACKAGE related macros.
Comment 24 Simon McVittie 2013-08-28 12:46:47 UTC
Comment on attachment 84788 [details] [review]
Generate autotools provided defines using a cmake macro

Review of attachment 84788 [details] [review]:
-----------------------------------------------------------------

Nice! OK, please commit.
Comment 25 Ralf Habacker 2013-08-28 12:51:51 UTC
Comment on attachment 84788 [details] [review]
Generate autotools provided defines using a cmake macro

applied to master
Comment 26 Ralf Habacker 2013-08-28 12:52:07 UTC
Comment on attachment 82669 [details] [review]
gcc 4.8.1 cross compiling warning fixes

applied to master
Comment 27 Simon McVittie 2013-08-29 11:58:25 UTC
All fixed in git for 1.7.6, I think.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.