On the dbus mailing list an error compiling dbus with cmake and msvc has been reported at http://lists.freedesktop.org/archives/dbus/2015-April/016656.html.
Created attachment 115179 [details] [review] cmake: Dump missing config header checks only if config.h.in is present.
Created attachment 115180 [details] [review] MSVC compile fix
Created attachment 115181 [details] [review] cmake: Add msvc support for sign-compare warnings.
Hi Ralf, your patches fixed the cmake build problem for the latest dbus version, thank you *very* much! ---rony
Just one remark, after having compiled the code for 64-bit Windows with the same MSVC compiler: there are a few warnings about possible loss of data converting from 'size_t' to 'int', e.g.: [ 36%] Building C object dbus/CMakeFiles/dbus-internal.dir/F_/work/git/dbus-win-20150418/dbus/dbus/dbus-auth-script.obj dbus-auth-script.c F:\work\git\dbus-win-20150418\dbus\dbus\dbus-auth-script.c(530) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data F:\work\git\dbus-win-20150418\dbus\dbus\dbus-auth-script.c(563) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data
(In reply to rony from comment #5) > Just one remark, after having compiled the code for 64-bit Windows with the > same MSVC compiler: there are a few warnings about possible loss of data > converting from 'size_t' to 'int', e.g.: > > [ 36%] Building C object > dbus/CMakeFiles/dbus-internal.dir/F_/work/git/dbus-win-20150418/dbus/dbus/ > dbus-auth-script.obj > dbus-auth-script.c > F:\work\git\dbus-win-20150418\dbus\dbus\dbus-auth-script.c(530) : warning > C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data > F:\work\git\dbus-win-20150418\dbus\dbus\dbus-auth-script.c(563) : warning > C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data https://msdn.microsoft.com/de-de/library/3b2e7499.aspx mentions that size_t is 64 on windows-64 bit while int is 32-bit.
(In reply to Ralf Habacker from comment #6) > (In reply to rony from comment #5) > > Just one remark, after having compiled the code for 64-bit Windows with the > > same MSVC compiler: there are a few warnings about possible loss of data > > converting from 'size_t' to 'int', e.g.: > > > > [ 36%] Building C object > > dbus/CMakeFiles/dbus-internal.dir/F_/work/git/dbus-win-20150418/dbus/dbus/ > > dbus-auth-script.obj > > dbus-auth-script.c > > F:\work\git\dbus-win-20150418\dbus\dbus\dbus-auth-script.c(530) : warning > > C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data > > F:\work\git\dbus-win-20150418\dbus\dbus\dbus-auth-script.c(563) : warning > > C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data > > https://msdn.microsoft.com/de-de/library/3b2e7499.aspx mentions that size_t > is 64 on windows-64 bit while int is 32-bit. the related code fragments are: 530: _dbus_string_delete (&to_send, where, strlen ("USERID_HEX")); 563: _dbus_string_delete (&to_send, where, strlen ("USERNAME_HEX")); the warning is caused by strlen() returning size_t while _dbus_string_delete() expects int as parameter 3. _dbus_string_delete (DBusString *str, int start, int len) @Simon: Would be a simple type cast 530: _dbus_string_delete (&to_send, where, (int)strlen ("USERNAME_HEX")); okay for you ? In dbus-string_util.c:_dbus_string_test() line 344 there is already a similar type cast 344: _dbus_assert (((int)strlen (s)) == i); Otherwise the third parameter of _dbus_string_delete() needs to be refactored.
Comment on attachment 115179 [details] [review] cmake: Dump missing config header checks only if config.h.in is present. Review of attachment 115179 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +26,5 @@ > autoversion(dbus) > + > +if(EXISTS ../config.h.in) > + autoheaderchecks(../config.h.in ConfigureChecks.cmake config.h.cmake) > +endif() Fine
Comment on attachment 115180 [details] [review] MSVC compile fix Review of attachment 115180 [details] [review]: ----------------------------------------------------------------- ::: tools/dbus-echo.c @@ +30,2 @@ > #include <unistd.h> > +#endif Fine @@ +233,4 @@ > if (noread) > { > while (1) > + _dbus_sleep_milliseconds (3600); Fine ::: tools/dbus-update-activation-environment.c @@ +63,3 @@ > /* apparently this is the portable way to get the entire environment... */ > extern char **environ; > +#endif Is dbus-update-activation-environment even useful on Windows? I thought I'd made it Unix-only... (If it "mostly works" on Windows, I'm pleasantly surprised - I hadn't really tried to make it portable.)
Comment on attachment 115181 [details] [review] cmake: Add msvc support for sign-compare warnings. Review of attachment 115181 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +207,5 @@ > + SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /w14018") > +else() > + SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wsign-compare") > + SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wsign-compare") > +endif() Fine
(In reply to Simon McVittie from comment #9) > ::: tools/dbus-update-activation-environment.c > @@ +63,3 @@ > > /* apparently this is the portable way to get the entire environment... */ > > extern char **environ; > > +#endif > > Is dbus-update-activation-environment even useful on Windows? I thought I'd > made it Unix-only... > > (If it "mostly works" on Windows, I'm pleasantly surprised - I hadn't really > tried to make it portable.) From https://msdn.microsoft.com/en-us/library/stxk41x1.aspx it looks as though a better way to get an equivalent of Unix environ might be to include stdlib.h (which that file does already) and #ifdef DBUS_WIN /* The Windows C runtime uses a different name */ #define environ _environ #else /* apparently this is the portable way to get the entire environment... * GNU platforms also put it in unistd.h but that's not portable */ extern char **environ; #endif or something similar. We can't use getenv/setenv/putenv or their Windows equivalents here, because we specifically want to iterate through the entire environment.
(In reply to Ralf Habacker from comment #7) > @Simon: Would be a simple type cast > > 530: _dbus_string_delete (&to_send, where, (int)strlen > ("USERNAME_HEX")); > > okay for you ? Yes: we know that casting strlen ("USERNAME_HEX") to int cannot actually overflow. The same reasoning is OK for any other uses of (int) strlen ("a small constant"). AIUI our preferred coding style is that casts look like "(int) foo" rather than "(int)foo" though. (I certainly prefer that way.) > Otherwise the third parameter of _dbus_string_delete() needs to be > refactored. It is deliberately an int rather than size_t. Havoc's reasoning was that if it overflows, we want the overflow to be to an obviously invalid value, rather than an incorrectly small value that could cause a buffer overflow. (AFAICS this is faulty reasoning, because integer overflow in C is undefined behaviour, so it could do anything... but it probably does what you'd expect in practice, and I'm not going to refactor all of DBusString right now.)
Created attachment 115214 [details] [review] Fix msvc sign-compare warning on Windows7 64bit by adding a type cast.
Comment on attachment 115179 [details] [review] cmake: Dump missing config header checks only if config.h.in is present. committed to master
Comment on attachment 115181 [details] [review] cmake: Add msvc support for sign-compare warnings. committed to master
Created attachment 115215 [details] [review] Fix msvc sign-compare warning on Windows7 64bit by adding a type cast (cleanup)
Comment on attachment 115215 [details] [review] Fix msvc sign-compare warning on Windows7 64bit by adding a type cast (cleanup) Review of attachment 115215 [details] [review]: ----------------------------------------------------------------- ship it
Comment on attachment 115215 [details] [review] Fix msvc sign-compare warning on Windows7 64bit by adding a type cast (cleanup) committed to master
(In reply to Simon McVittie from comment #9) > Comment on attachment 115180 [details] [review] [review] > MSVC compile fix > ::: tools/dbus-update-activation-environment.c > @@ +63,3 @@ > > /* apparently this is the portable way to get the entire environment... */ > > extern char **environ; > > +#endif > > Is dbus-update-activation-environment even useful on Windows? I thought I'd > made it Unix-only... > > (If it "mostly works" on Windows, I'm pleasantly surprised - I hadn't really > tried to make it portable.) How could I verify that it works ?
Created attachment 115216 [details] text file that lists cast warnings on MSC 64-bit Maybe that is too late, but just to make sure that a complete list of the files that issue cast warnings on MSC 64- bit is present, I attach this edited output for easier checking. Here the files that issue such warnings: dbus-auth-script.c dbus-connection.c dbus-internals.c dbus-marshal-basic.c dbus-memory.c dbus-mempool.c dbus-message.c dbus-monitor.c dbus-nonce.c dbus-object-tree.c dbus-sha.c dbus-spawn-win.c dbus-string-util.c dbus-string.c dbus-sysdeps-util-win.c dbus-sysdeps-win.c signals.c
(In reply to rony from comment #20) > Created attachment 115216 [details] > text file that lists cast warnings on MSC 64-bit > > Maybe that is too late, but just to make sure that a complete list of the > files that issue cast warnings on MSC 64- bit is present, I attach this > edited output for easier checking. > > Here the files that issue such warnings: > > dbus-auth-script.c > dbus-connection.c > dbus-internals.c > dbus-marshal-basic.c > dbus-memory.c > dbus-mempool.c > dbus-message.c > dbus-monitor.c > dbus-nonce.c > dbus-object-tree.c > dbus-sha.c > dbus-spawn-win.c > dbus-string-util.c > dbus-string.c > dbus-sysdeps-util-win.c > dbus-sysdeps-win.c > signals.c As simon stated (In reply to Simon McVittie from comment #12) > Yes: we know that casting strlen ("USERNAME_HEX") to int cannot actually > overflow. The same reasoning is OK for any other uses of (int) strlen ("a > small constant"). If there are any strlen cases, please fix them and append a related patch with git format-patch to this bug for reviewing. If there are other reasons please append a build log to this bug showing the related warnings.
Will do later today.
Starting to go after the MSC 64-bit warnings. However, almost at the end of the build the following error (missing 'unistd.h') occurs: [ 96%] Building C object tools/CMakeFiles/dbus-test-tool.dir/F_/work/git/dbus-win-20150420-master/dbus/tools/dbus-echo.obj dbus-echo.c F:\work\git\dbus-win-20150420-master\dbus\tools\dbus-echo.c(29) : fatal error C1083: Cannot open include file: 'unistd.h': No such file or directory NMAKE : fatal error U1077: 'E:\PROGRA~1\MICROS~2.0\VC\bin\X86_AM~1\cl.exe' : return code '0x2' --- the git log shows: F:\work\git\dbus-win-20150420-master\dbus>git log commit 449b5b9bfa34b49c1a5f8a4031687befc8bfac82 Author: Ralf Habacker <ralf.habacker@freenet.de> Date: Mon Apr 20 12:35:53 2015 +0200 Fix msvc sign-compare warning on Windows7 64bit by adding a type cast. The related warning is: C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=90089 Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> commit b43ad50be70c6ad6982b3560dd60b8b6411e1f57 Author: Ralf Habacker <ralf.habacker@freenet.de> Date: Sat Apr 18 21:54:05 2015 +0200 cmake: Add msvc support for sign-compare warnings. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=90089 Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Is it o.k. to also cast the result of string_array_length() to (int) like with strlen() in the same patch?
Created attachment 115228 [details] [review] diff (patch) for (int) casts Sorry, the first time I use git to create a patch (used 'git diff HEAD'). It seems that quite a few line-ends got changed/diffed by using the vs.exe (visual slick) editor, which usually identifies the end-of-lines correctly. So a little word of warning as the patch is therefore almost 200K large! --- This patch casts the results of strlen() to '(int)' to remove the warnings on 64-bit MSC. Also casts to '(int)' the results of string_array_length() where the length is assigned to an int variable (and therefore causes the warning). This refers to 'dbus/dbus-object-tree.c' only.
(In reply to rony from comment #24) > Is it o.k. to also cast the result of string_array_length() to (int) like > with strlen() in the same patch? The question to ask yourself is: is there any way that this length could possibly be outside the range of an int? If it's a compile-time constant length (e.g. it came from strlen() or sizeof(array)/sizeof(array[0]) or something) then it's OK. If in doubt, add a comment saying why it's OK. If it's a runtime-variable length, then it is not OK unless you have checked, before casting, that it is in range for the smaller type. For instance (this is pseudocode, obviously): void foo (size_t n) { ... if (n > INT_MAX) return_error ("n too large"); /* cast is safe: we explicitly range-checked n above */ some_api_that_expects_an_int (..., (int) n, ...); ... }
(In reply to rony from comment #23) > dbus-echo.c > F:\work\git\dbus-win-20150420-master\dbus\tools\dbus-echo.c(29) : fatal > error C1083: Cannot open include file: 'unistd.h': No > such file or directory > NMAKE : fatal error U1077: 'E:\PROGRA~1\MICROS~2.0\VC\bin\X86_AM~1\cl.exe' : > return code '0x2' > This is fixed with attachment 90089 [details] but isn't ready for applying.
Comment on attachment 115228 [details] [review] diff (patch) for (int) casts Review of attachment 115228 [details] [review]: ----------------------------------------------------------------- ::: bus/signals.c @@ +70,4 @@ > #ifndef DBUS_ENABLE_EMBEDDED_TESTS > _dbus_assert (rule->matches_go_to != NULL); > #endif > + Here and in mostly other places leading spaces has been removed which lead into the huge diff file.
Ralf, is there anything I can/should do about the removed leading whitespace or would it be ok the way it is?
Comment on attachment 115228 [details] [review] diff (patch) for (int) casts Review of attachment 115228 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +1734,4 @@ > /* use the GVariant bytestring convention for strings of unknown > * encoding: include the \0 in the payload, for zero-copy reading */ > if (!_dbus_asv_add_byte_array (&array_iter, "LinuxSecurityLabel", > + s, (int) strlen (s) + 1)) In principle we should do something like size_t len; len = strlen (s); if (len > (INT_MAX - 1)) { _dbus_verbose ("ignoring ridiculously long LinuxSecurityLabel"); dbus_free (s); } else if (!_dbus_asv_add_byte_array (..., 1 + (int) len)) { ... out of memory ... } (However, in practice I suspect Linux limits security labels to some sane length; and you can't control how long security labels are unless you're root, so this isn't useful as an attack vector.) ::: bus/signals.c @@ +70,4 @@ > #ifndef DBUS_ENABLE_EMBEDDED_TESTS > _dbus_assert (rule->matches_go_to != NULL); > #endif > + Please do not include whitespace changes in unrelated patches. You might find "git commit -i" or "git gui" useful. This was more time-consuming to review than it should have been, and I might well have missed some functional changes. If in doubt, review your own patch and ask yourself, if I was the maintainer, would I accept this? I'm almost tempted to do a scripted commit that deletes all the trailing whitespace (the only way it would be acceptable would be as a commit that does literally nothing else), but it would be pretty annoying for backports to stable-branches - we'd get a lot of spurious conflicts. The current approach is "only clean up trailing whitespace when you're modifying a very nearby line anyway, so you're not actually making merge conflicts any worse". @@ +848,4 @@ > > if (end != length) > { > + int len1 = (int) strlen ("path"); This is OK: "path" is a constant string. @@ +1940,4 @@ > if (!str_has_prefix (path, rule->path)) > return FALSE; > > + len = (int) strlen (rule->path); If whatever sets rule->path has verified that the length is bounded and less than INT_MAX, then a comment saying so here would be OK. @@ +1991,4 @@ > dbus_message_iter_get_basic (&iter, &actual_arg); > _dbus_assert (actual_arg != NULL); > > + actual_length = (int) strlen (actual_arg); This is probably OK, because the D-Bus Specification says messages cannot exceed 128M (therefore individual strings in a message cannot exceed 128M either, and 128M fits in an int); but it at least deserves a comment, and probably a _dbus_assert(). It's a bit disappointing that we don't have a dbus_message_iter_get_string_and_length (DBusMessageIter *, char **, int *) which works on strings, object paths and signatures, given that the length is already encoded in the message - we only actually need the strlen() here because the API is deficient. ::: dbus/dbus-connection.c @@ +5665,4 @@ > char **decomposed_path; > dbus_bool_t retval; > > + if (!_dbus_decompose_path (path, (int) strlen (path), &decomposed_path, NULL)) If this is going to assume that the path length is bounded, it should check it, or its callers should check it and pass in an int. _dbus_return_[val_]if_fail() is appropriate for diagnosing programmer error in public APIs. I'd be OK with considering object paths > INT_MAX to be programmer error even though the D-Bus Specification theoretically allows (2**32)-1 bytes, because object paths > 128M long cannot actually be used (that's the limit on the length of the message). I would also be OK with considering object paths > 128M long to be invalid, tbh. That's a truly ridiculous length already. @@ +5841,4 @@ > _dbus_return_val_if_fail (path != NULL, FALSE); > _dbus_return_val_if_fail (path[0] == '/', FALSE); > > + if (!_dbus_decompose_path (path, (int) strlen (path), &decomposed_path, NULL)) If this is going to assume that the path length is bounded, it should check it. @@ +5875,5 @@ > _dbus_return_val_if_fail (data_p != NULL, FALSE); > > *data_p = NULL; > + > + if (!_dbus_decompose_path (path, (int) strlen (path), &decomposed_path, NULL)) If this is going to assume that the path length is bounded, it should check it. @@ +5912,4 @@ > _dbus_return_val_if_fail (parent_path[0] == '/', FALSE); > _dbus_return_val_if_fail (child_entries != NULL, FALSE); > > + if (!_dbus_decompose_path (parent_path, (int) strlen (parent_path), &decomposed_path, NULL)) If this is going to assume that the path length is bounded, it should check it. ::: dbus/dbus-internals.c @@ +415,4 @@ > #endif > > /* Only print pid again if the next line is a new line */ > + len = (int) strlen (format); len could probably just be a size_t here, bypassing the whole issue? ::: dbus/dbus-marshal-basic.c @@ +757,4 @@ > { > return marshal_len_followed_by_bytes (MARSHAL_AS_STRING, > str, insert_at, value, > + (int) strlen (value), Messages are bounded at 128M by the wire protocol, so perhaps callers check this? If they don't, this function should. Passing in a too-long string here would be considered to be programmer error. @@ +769,4 @@ > { > return marshal_len_followed_by_bytes (MARSHAL_AS_SIGNATURE, > str, insert_at, value, > + (int) strlen (value), It is not obvious to me that this is valid. Signatures are bounded at 255 nonzero bytes + '\0' by the wire protocol, so perhaps callers check this? If they don't, this function should. Passing in > 255 nonzero bytes would be considered to be programmer error. ::: dbus/dbus-message.c @@ +3174,4 @@ > v = dbus_message_get_path (message); > if (v != NULL) > { > + if (!_dbus_decompose_path (v, (int) strlen (v), Deserves a comment explaining why it is valid, at least. ::: dbus/dbus-object-tree.c @@ +1110,4 @@ > > _dbus_assert (name != NULL); > > + len = (int) strlen (name); It is not obvious to me that this is valid. @@ +1630,4 @@ > int expected_len; > > if (!_dbus_decompose_path (decompose_tests[i].path, > + (int) strlen (decompose_tests[i].path), This is probably OK because this is test code, so we know that our paths are not overly long. @@ +1634,4 @@ > &result, &result_len)) > return FALSE; > > + expected_len = (int) string_array_length (decompose_tests[i].result); Ditto. Perhaps expected_len should have been a size_t, but it's hard to printf those in a portable way, because Windows doesn't support the ISO C99-mandated "%zu" and instead uses the Windows-specific "%Iu", so int is probably the better type here :-( @@ +1640,5 @@ > expected_len != result_len || > path_contains (decompose_tests[i].result, > (const char**) result) != STR_EQUAL) > { > + int real_len = (int) string_array_length ((const char**)result); Ditto @@ +1875,4 @@ > _dbus_object_tree_list_registered_unlocked (tree, path1, &child_entries); > if (child_entries != NULL) > { > + nb = (int) string_array_length ((const char**)child_entries); Ditto; or perhaps nb could be a size_t? @@ +1883,4 @@ > _dbus_object_tree_list_registered_unlocked (tree, path2, &child_entries); > if (child_entries != NULL) > { > + nb = (int) string_array_length ((const char**)child_entries); Ditto @@ +1891,4 @@ > _dbus_object_tree_list_registered_unlocked (tree, path8, &child_entries); > if (child_entries != NULL) > { > + nb = (int) string_array_length ((const char**)child_entries); Ditto @@ +1899,4 @@ > _dbus_object_tree_list_registered_unlocked (tree, root, &child_entries); > if (child_entries != NULL) > { > + nb = (int) string_array_length ((const char**)child_entries); Ditto ::: dbus/dbus-sha.c @@ +550,4 @@ > check_sha_str (const char *input, > const char *expected) > { > + return check_sha_binary (input, (int) strlen (input), expected); Is this just test code? I can't immediately tell... check_sha_binary() should probably take a size_t instead. ::: dbus/dbus-spawn-win.c @@ +507,4 @@ > if (!strings || !strings[0]) > return 0; > for (i = 0; strings[i]; i++) > + n += (int) strlen (strings[i]) + 1; Does anything guarantee that this is in-range, and that the addition doesn't overflow? (This is probably a case where the compiler warning has flagged up a real pre-existing bug: n could overflow and we would not malloc enough bytes. Luckily this is in Windows-specific code, where we do not support dbus-daemon being used as a security boundary.) ::: dbus/dbus-string-util.c @@ +58,1 @@ > if (((unsigned long)real_a->len) < c_str_len) Those two lines, together, look really suspicious: we're mixing up types. I suspect that c_str_len should be a size_t, and we should use ((size_t) real_a->len) (or assign it to a local variable of type size_t) throughout. DBUS_GENERIC_STRING_PREAMBLE will already have asserted that real_a->len is non-negative. @@ +129,4 @@ > int end; > > if (len < 0) > + len = (int) strlen (data); Maybe data is bounded in length because this is just test code? If in doubt, _dbus_assert(). ::: dbus/dbus-string.c @@ +195,2 @@ > _dbus_string_init_const_len (str, value, > + (int) strlen (value)); This looks suspicious: is the given value bounded? We should probably do the strlen(), then _dbus_assert() that it is in-range, and finally call _dbus_string_init_const_len(). @@ +940,4 @@ > DBUS_STRING_PREAMBLE (str); > _dbus_assert (buffer != NULL); > + > + buffer_len = (int) strlen (buffer); buffer_len should be of type size_t, and we should check whether buffer_len > (size_t) _DBUS_STRING_MAX_LENGTH. ::: dbus/dbus-sysdeps-util-win.c @@ +117,4 @@ > } > > total = 0; > + bytes_to_write = (int) strlen (pidstr);; Double semicolon considered useless; remove it while you're altering this line anyway. pidstr is a small stack buffer so this cast is OK, but it seems like it would be better if total and bytes_to_write were size_t instead. ::: dbus/dbus-sysdeps-win.c @@ +1568,4 @@ > } > _DBUS_ASSERT_ERROR_IS_CLEAR(error); > > + if (connect (fd, (struct sockaddr*) tmp->ai_addr, (int) tmp->ai_addrlen) == SOCKET_ERROR) Does connect() really take an argument type that differs from the type of (struct addrinfo *)->ai_addrlen? That seems rather ridiculous :'-( But presumably it's bounded, because we can trust the operating system not to have multi-gigabyte addresses; so that cast is OK. @@ +1720,4 @@ > } > _DBUS_ASSERT_ERROR_IS_CLEAR(error); > > + if (bind (fd, (struct sockaddr*) tmp->ai_addr, (int) tmp->ai_addrlen) == SOCKET_ERROR) Same as for connect() @@ +2966,4 @@ > > // create shm > hDBusSharedMem = CreateFileMappingA( INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, > + 0, (int) strlen( address ) + 1, _dbus_string_get_const_data(&shm_name) ); Is the length of address bounded? Does CreateFileMappingA really take an int? Ugh, looking at MSDN, it's worse: the size is split into two DWORDs for the high and low 32-bit parts of a potentially 64-bit length, or something. What's going on with that API? If I'm reading MSDN correctly, in principle we should pre-compute dbus_uint64_t len = strlen (address) + 1; (or __int64 if you prefer, since this is Windows-specific code already), and then pass (..., PAGE_READWRITE, (len >> 32), (len & 0xFFFFFFFF), ...) to CreateFileMappingA? Or something? ::: tools/dbus-monitor.c @@ +426,4 @@ > /* Prepend a rule (and a comma) to enable the monitor to eavesdrop. > * Prepending allows the user to add eavesdrop=false at command line > * in order to disable eavesdropping when needed */ > + filter_len = (int) strlen (EAVESDROPPING_RULE) + 1 + (int) strlen (arg) + 1; filter_len should probably just be a size_t which would sidestep the whole thing.
(In reply to Ralf Habacker from comment #28) > > error C1083: Cannot open include file: 'unistd.h' > > This is fixed with attachment 90089 [details] but isn't ready for applying. I think you mean Attachment #115180 [details] :-) (I keep mixing up bug numbers and attachment numbers, too.) That patch isn't ready to apply to git master, but it's good enough to use to continue testing.
Created attachment 115230 [details] [review] text file that lists cast warnings on MSC 64-bit without white-space changes Simon, sorry for the inconvenience. That is the reason why I wrote the first paragraph (it is the first time I use git to create a patch). Trying an "-i" to ignore whitespace did not work on my cygwin version of git. However I found "-b" which allowed me to create the "essence" diff/patch. Maybe it proofs nevertheless to be helpful. Still have to go through all of your other comments in the case I should/can change something (but really, Ralf is probably the one who is really in the know, I am just a "casual" walker-by).
(In reply to Simon McVittie from comment #31) > Comment on attachment 115228 [details] [review] [review] > Ugh, looking at MSDN, it's worse: the size is split into two DWORDs for the > high and low 32-bit parts of a potentially 64-bit length, or something. > What's going on with that API? This windows api function is designed to be callable from 32 bit clients supporting 64 bit file size types. > If I'm reading MSDN correctly, in principle we should pre-compute > > dbus_uint64_t len = strlen (address) + 1; Better size_t, which is the type strlen() returns. On msvc 64 bit size_t is a 64 bit type by default, on all other system 32bit, so right shifting afterwards works in any case. > then pass (..., PAGE_READWRITE, (len >> 32), (len & 0xFFFFFFFF), ...) to > CreateFileMappingA? yes
(In reply to rony from comment #33) > Trying an "-i" to ignore whitespace did not work on my cygwin version of git. Sorry, I meant "git commit --interactive". I thought that abbreviated to -i, but I was wrong. (In reply to Ralf Habacker from comment #34) > > Ugh, looking at MSDN, it's worse: the size is split into two DWORDs for the > > high and low 32-bit parts of a potentially 64-bit length, or something. > > What's going on with that API? > > This windows api function is designed to be callable from 32 bit clients > supporting 64 bit file size types. Right, but 32-bit CPUs can have 64-bit integers just fine... oh well, blame Microsoft. > > If I'm reading MSDN correctly, in principle we should pre-compute > > > > dbus_uint64_t len = strlen (address) + 1; > > Better size_t, which is the type strlen() returns. On msvc 64 bit size_t is > a 64 bit type by default, on all other system 32bit, so right shifting > afterwards works in any case. Right-shifting a 32-bit value by 32 bits is undefined behaviour, so we can't say (len >> 32) if len is only 32 bits long. ISO/IEC 9899:201x draft n1570 §6.5.7.3: "If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined". (Draft n1570 was the last freely available draft of ISO C11; I don't have a copy of the actual standards text, which costs money, but I believe n1570 is essentially equivalent to the actual standard.) I'd rather not get into writing horrible code like size_t len = ...; CreateFileMappingA (..., PAGE_READWRITE, #ifdef _WIN64 (len >> 32), (len & 0xFFFFFFFF), #else 0, len, #endif ...); if we don't really need to, and dbus already has a 64-bit type.
(In reply to rony from comment #33) > However I found "-b" which allowed me to create the "essence" diff/patch. > Maybe it proofs nevertheless to be helpful. Thanks, that's a lot easier to deal with. I assume it's functionally the same as the one with whitespace changes, which I already reviewed.
Created attachment 115407 [details] [review] MSVC compile fix (update)
Comment on attachment 115407 [details] [review] MSVC compile fix (update) Review of attachment 115407 [details] [review]: ----------------------------------------------------------------- The actual changes look fine. I'm not so happy about the commit message, which says very little. I was about to say that these look more like Windows fixes than MSVC fixes - but actually, mingw (at least mingw-w64) provides e.g. a unistd.h that declares sleep(). Maybe something like this: > tools: MSVC compile fixes > > unistd.h and sleep() are normally Unix-specific, although mingw also provides them. > The Windows C runtime documents _environ as its equivalent of Unix environ.
Created attachment 115412 [details] [review] tools: MSVC compile fixes (updated commit message)
Comment on attachment 115412 [details] [review] tools: MSVC compile fixes (updated commit message) Review of attachment 115412 [details] [review]: ----------------------------------------------------------------- Ideal, please merge. Thanks!
Comment on attachment 115412 [details] [review] tools: MSVC compile fixes (updated commit message) committed to master
(In reply to Simon McVittie from comment #35) > Right-shifting a 32-bit value by 32 bits is undefined behaviour, so we can't > say (len >> 32) if len is only 32 bits long. ISO/IEC 9899:201x draft n1570 > §6.5.7.3: "If the value of the right operand is negative or is greater than > or equal to the width of the promoted left operand, the behavior is > undefined". I believe that this statement belongs to the << operator, which is described in 6.5.7.4 "... If E1 has an unsigned type, the value of the result is E1 × 2 ^ E2, reduced modulo one more than the maximum value representable in the result type .... otherwise, the behavior is undefined" The right shift operator described in 6.5.7.5 "... If E1 has an unsigned type or if E1 has a signed type and a nonnegative value, the value of the result is the integral part of the quotient of E1 / 2 E2" so it looks valid also for unsigned int.
(In reply to Ralf Habacker from comment #42) > (In reply to Simon McVittie from comment #35) > so it looks valid also for unsigned int. Unfortunally gcc does not follow my observations :-( /home/xxx/src/dbus-1/dbus/dbus-sysdeps-win.c:2970:44: warning: right shift count >= width of type [-Wshift-count-overflow] len >> 32, len & 0xffffffff, _dbus_string_get_const_data(&shm_name) ); ^ so we need to have different implementation for _WIN64 and _WIN32
Created attachment 115455 [details] [review] dbus_daemon_publish_session_bus_address: Fix -Wsign-compare issue
Comment on attachment 115455 [details] [review] dbus_daemon_publish_session_bus_address: Fix -Wsign-compare issue Review of attachment 115455 [details] [review]: ----------------------------------------------------------------- I'd still prefer to make the type of @len be 64-bit (dbus_uint64_t or __uint64) unconditionally. That would allow using the ((len >> 32), (len & 0xffffffffu)) code path on both 32- and 64-bit Windows, without the #ifdef. (This is basically academic - addresses beyond 32-bit would be nonsensical.) ::: dbus/dbus-sysdeps-win.c @@ +2970,3 @@ > hDBusSharedMem = CreateFileMappingA( INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, > +#ifdef _WIN64 > + len >> 32, len & 0xffffffff, Strictly speaking I think this should be 0xffffffffu (an unsigned quantity). Or you could use _DBUS_UINT32_MAX.
Am I right in thinking that we've fixed the errors, and what's left is a bunch of -Wsign-compare warnings?
Created attachment 115746 [details] [review] dbus_daemon_publish_session_bus_address: Fix -Wsign-compare issue (update)
Created attachment 115748 [details] [review] dbus_daemon_publish_session_bus_address: Fix -Wsign-compare issue (update)
(In reply to Simon McVittie from comment #46) > Am I right in thinking that we've fixed the errors, and what's left is a > bunch of -Wsign-compare warnings? I do not have access to a msvc 64bit system, so it is up to rony to give an answer.
Comment on attachment 115748 [details] [review] dbus_daemon_publish_session_bus_address: Fix -Wsign-compare issue (update) Review of attachment 115748 [details] [review]: ----------------------------------------------------------------- Looks good. This is sufficiently niche that I'd prefer not to push it to 1.8.
Comment on attachment 115748 [details] [review] dbus_daemon_publish_session_bus_address: Fix -Wsign-compare issue (update) (In reply to Simon McVittie from comment #50) > Looks good. This is sufficiently niche that I'd prefer not to push it to 1.8. Applied this myself because I'd like to do a release soon. Fixed in git for 1.9.16
Was away, hence no answer about compiling in 64-bit mode. Maybe just a hint: it is possible with the 32-bit MSC compiler to compile for 64-bit, that is what I actually do. Here is what I do for that: - MSC (German installation, hence the German name "E:\Programme"): call "E:\Programme\Microsoft Visual Studio 9.0\VC\vcvarsall.bat" x86_amd64 - by default I also do a: call "E:\Programme\Microsoft Platform SDK for Windows Server 2003 R2\SetEnv.Cmd" /XP64 /RETAIL This is on a very old 32-bit Windows XP machine (not having had time to finally migrate it to a new PC). In the case that this does not work, please let me know and I will dig out everything again and do a 64-compilation.
NEEDINFO from MSVC users: what is left to do, and does the answer involve "fixing fatal errors" or just "fixing non-fatal warnings"?
Had some time so attempting to create a 32- and 64-bit Windows version of the release version of dbus 1.10.0 using the MS compiler. Received the following fatal error on either bitness: Attempt to create 32-bit version, fatal error information: --- cut --- F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2972) : error C2275: 'dbus_uint64_t' : illegal use of this type as an expression F:\work\git\dbus-win-20150929\dbus\dbus-build\dbus/dbus-arch-deps.h(37) : see declaration of 'dbus_uint64_t' F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2972) : error C2146: syntax error : missing ';' before identifier 'len' F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2972) : error C2065: 'len' : undeclared identifier F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2975) : error C2065: 'len' : undeclared identifier F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2975) : warning C4293: '>>' : shift count negative or too big, undefined behavior F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2975) : error C2065: 'len' : undeclared identifier NMAKE : fatal error U1077: 'E:\PROGRA~1\MICROS~2.0\VC\bin\cl.exe' : return code '0x2' Stop. NMAKE : fatal error U1077: '"E:\Programme\Microsoft Visual Studio 10.0\VC\BIN\nmake.exe"' : return code '0x2' Stop. NMAKE : fatal error U1077: '"E:\Programme\Microsoft Visual Studio 10.0\VC\BIN\nmake.exe"' : return code '0x2' Stop. --- cut --- Attempt to create *64*-bit version, fatal error information: --- cut --- [ 31%] Building C object dbus/CMakeFiles/dbus-1.dir/F_/work/git/dbus-win-20150929/dbus/dbus/dbus-sysdeps-win.obj dbus-sysdeps-win.c F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(1565) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(1718) : warning C4267: 'function' : conversion from 'size_t' to 'int', possible loss of data F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2972) : error C2275: 'dbus_uint64_t' : illegal use of this type as an expression F:\work\git\dbus-win-20150929\dbus\dbus-build64\dbus/dbus-arch-deps.h(37) : see declaration of 'dbus_uint64_t' F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2972) : error C2146: syntax error : missing ';' before identifier 'len' F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2972) : error C2065: 'len' : undeclared identifier F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2972) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2975) : error C2065: 'len' : undeclared identifier F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2975) : warning C4293: '>>' : shift count negative or too big, undefined behavior F:\work\git\dbus-win-20150929\dbus\dbus\dbus-sysdeps-win.c(2975) : error C2065: 'len' : undeclared identifier NMAKE : fatal error U1077: 'E:\PROGRA~1\MICROS~2.0\VC\bin\X86_AM~1\cl.exe' : return code '0x2' Stop. NMAKE : fatal error U1077: '"E:\Programme\Microsoft Visual Studio 10.0\VC\BIN\nmake.exe"' : return code '0x2' Stop. NMAKE : fatal error U1077: '"E:\Programme\Microsoft Visual Studio 10.0\VC\BIN\nmake.exe"' : return code '0x2' Stop. --- cut --- The 64-bit compile generates numerous warnings (as can be seen above), which I could try to tackle again, if desired. [These outputs stem from a German Windows XP SP 3 version, still no time to upgrade to a newer system. :( ]
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/126.
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.