Opened 8 years ago

Closed 8 years ago

#7260 closed defect (fixed)

MinGW64 & -Werror -Werror

Reported by: yayooo Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.24-rc
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Patch1(MinGW64.patch) build on MinGW64(solved 2 errors):

ERROR1:
gcc -DHAVE_CONFIG_H -I. -I../.. -I../../src/common -ggdb3 -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wstack-protector -fwrapv --param ssp-buffer-size=1 -Wall -fno-strict-aliasing -MT procmon.o -MD -MP -MF .deps/procmon.Tpo -c -o procmon.o procmon.c
procmon.c:30:13: error: conflicting types for 'pid_t'
In file included from torint.h:20:0,

from compat.h:10,
from procmon.h:12,
from procmon.c:9:

C:\MinGW\bin\../lib/gcc/x86_64-w64-mingw32/4.7.0/../../../../x86_64-w64-mingw32/include/sys/types.h:68:16: note: previous declaration of 'pid_t' was here

TODO1:
Please Test on MSVC64.

ERROR2:
Oct 30 09:44:52.000 [err] tor_asprintf(): Bug: Internal error in asprintf
Oct 30 09:44:52.000 [err] tor_asprintf(): Bug: compat.c:405: tor_asprintf: Assertion 0 failed; aborting.
compat.c:405 tor_asprintf: Assertion 0 failed; aborting.

TODO2:
Please Test on MinGW32.

Patch2(Werror.patch) Eliminate ALL Warnings:
I have test Werror.patch on Linux_x86_64_2.6.18 & MinGW64 & OpenBSD_5.1_amd64.
configure is derive from configure.in, So not included in this patch, Please generate by yourself(run autoconf before your configure).

Child Tickets

Attachments (6)

MinGW64.patch (978 bytes) - added by yayooo 8 years ago.
Werror.patch (22.5 KB) - added by yayooo 8 years ago.
MinGW64.2.patch (1.9 KB) - added by yayooo 8 years ago.
Werror.2.patch (22.7 KB) - added by yayooo 8 years ago.
MinGW64.3.patch (1.9 KB) - added by yayooo 8 years ago.
Werror.3.patch (22.5 KB) - added by yayooo 8 years ago.

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by yayooo

Attachment: MinGW64.patch added

Changed 8 years ago by yayooo

Attachment: Werror.patch added

comment:1 Changed 8 years ago by rransom

Status: newneeds_revision

re MinGW64.patch (with SHA-256 4f142e2dbb33c786333e9b8e7936d181f1407c705a2a2046a128f964506e155c):

  • _MSC_VER should be replaced with an ‘are we using Microsoft's C runtime library on Windows’ macro, not with a list of three macros which happen to usually mean that Tor is being linked against that library.
  • Whether procmon.c typedefs pid_t or not should be determined by an autoconf test (like the way that src/common/util.c decides whether to define environ).

Werror.patch appears to contain several unrelated changes which should be split into separate patches.

comment:2 Changed 8 years ago by rransom

The changes to src/test/ in Werror.patch (SHA-256 7fe3a3e09fa98f5122de20a9a4b584e8155c052292faee8a509b7b765da563bd) require a very good explanation.

comment:3 Changed 8 years ago by yayooo

I am sure each line modification in Werror.patch is for the goal "-Wextra -Werror"
I have test this patch on Linux_x86_64_2.6.18 & MinGW64 & OpenBSD_5.1_amd64
and make sure with "-Wextra -Werror" or without "-Wextra -Werror" almost generate same config.log
and compiled successfully and ran successfully.

  • AC_COMPILE_IFELSE([AC_LANG_PROGRAM([$3],[void *ptr= $1 ;])],

+ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([$3],[void *ptr= $1 ;ptr=ptr;])],
AVOID:
error: unused variable 'ptr'

-int main(int c, char v) {
+int main() {
AVOID:
error: unused parameter 'c'
error: unused parameter 'v'

-AC_SEARCH_LIBS(pow, [m], , AC_MSG_ERROR([Could not find pow in libm or libc.]))
+AC_MSG_CHECKING(for library containing pow)
+LIBS=
+AC_LINK_IFELSE([AC_LANG_PROGRAM(double pow(double,double);,pow(2.7,3.1);)],ac_cv_search_pow="none required")

if test "$ac_cv_search_pow" != "none required"; then

+LIBS='-lm'
+AC_LINK_IFELSE([AC_LANG_PROGRAM(double pow(double,double);,pow(2.7,3.1);)],ac_cv_search_pow="$LIBS",AC_MSG_RESULT(no)
+AC_MSG_ERROR(Could not find pow in libm or libc.))

TOR_LIB_MATH="$ac_cv_search_pow"

+AC_LINK_IFELSE([AC_LANG_PROGRAM(long int lround(double);,lround(2.7);)],AC_DEFINE(HAVE_LROUND,1))
+AC_LINK_IFELSE([AC_LANG_PROGRAM(double rint(double);,rint(2.7);)],AC_DEFINE(HAVE_RINT,1))

fi

+AC_MSG_RESULT($ac_cv_search_pow)
AVOID:
error: conflicting types for built-in function 'lround'
error: conflicting types for built-in function 'rint'
error: conflicting types for built-in function 'pow'

  • [RAND_add((void*)0,0,0); exit(0);], [],

+ [RAND_add((void*)0,0,0);], [],
AVOID:
error: incompatible implicit declaration of built-in function 'exit'

+#ifndef _WIN32_WINNT

#define _WIN32_WINNT 0x0501

+#endif
AVOID:
error: "_WIN32_WINNT" redefined

-int main(int c, charv) { if (((time_t)-1)<0) return 1; else return 0; }])],
+int main() { if (((time_t)-1)<1) return 1; else return 0; }])],
AVOID:
error: comparison of unsigned expression < 0 is always false

  • "process %d.",
  • procmon->pid);

+ "process "I64_FORMAT".",
+ I64_PRINTF_ARG(procmon->pid));
AVOID:
error: format '%d' expects argument of type 'int', but argument 5 has type 'pid_t'

  • log_debug(LD_NET,"Cleaning up connection (fd %d).",conn->s);

+ log_debug(LD_NET,"Cleaning up connection (fd "TOR_SOCKET_T_FORMAT").",conn->s);
AVOID:
error: format '%d' expects argument of type 'int', but argument 5 has type 'intptr_t'

-launch_test_addresses(int fd, short event, void *args)
+launch_test_addresses(evutil_socket_t fd, short event, void *args)
AVOID:
error: passing argument 4 of 'event_new' from incompatible pointer type

  • tt_assert_test_type(a,b,#a" "#op" "#b,long,(val1_ op val2_), \
  • "%ld",TT_EXIT_TEST_FUNCTION)

+ tt_assert_test_type(a,b,#a" "#op" "#b,INTPTR_T_to_INT,(val1_ op val2_), \
+ ""INTPTR_T_FORMAT"",TT_EXIT_TEST_FUNCTION)
AVOID:
error: cast from pointer to integer of different size

Changed 8 years ago by yayooo

Attachment: MinGW64.2.patch added

Changed 8 years ago by yayooo

Attachment: Werror.2.patch added

comment:4 Changed 8 years ago by yayooo

Status: needs_revisionneeds_review

As follow your advises, I updated MinGW64.2.patch , detect _vscprintf() and pid_t in autoconf .
I also updated src/common/procmon.c in Werror.2.patch .
And tested on Linux_x86_64 MinGW64 OpenBSD_5.1_amd64 .

comment:5 Changed 8 years ago by nickm

Status: needs_reviewneeds_revision

The MinGW64.2.patch one looks almost okay. It's not necessary to modify orconfig.h.in, though, since it should get automatically generated by autoheader. But we probably *do* need corresponding changes in src/win32/orconfig.h, since the MSVC build (which uses it) doesn't use autoconf. These problems seem like they could be good to fix in 0.2.3, ... I could use a second opinion from Roger/Weasel/Erinn/Andrea/everybody else on that one, though. If not, alpha releases _are_ where we usually add support for more platforms.

On the Werror.2.patch:

  • This one is complicated enough that it probably makes more sense for 0.2.4... but hm. Some of the warnings that it fixes -- notably the socket printf one -- look as though they might mean invoking sprintf with too many arguments on Win64. That is probably not safe.
  • I don't understand what the point is to making the autoconf tests all build with -Werror; autoconf shouldn't be passing any -W flags to the compiler while it's building. In particular, it seems like a step backwards to have to clone AC_SEARCH_LIBS. Do you know about --enable-gcc-warnings ? You can invoke ./configure as "./configure --enable-gcc-warnings" and get Tor built with -Wextra -Werror and many more warnings.
  • INTPTR_T_to_INT should probably use the same naming convention as the other printf stuff: look at how U64_PRINTF_ARG() works. If we're calling it INTPTR_T_TO_INT, it needs to be a function or a macro that converts. If it has to be a type, then it should probably be INTPTR_PRINTF_TYPE or something.
  • In test_util.c , 1000000 is entirely too many bytes to be stack-allocating.
  • In src/test/tinytest_macros.h, intptr_t doesn't make much sense as a type. int64_t would make more sense; so would intmax_t if we had it.

comment:6 Changed 8 years ago by yayooo

Status: needs_revisionneeds_review

I am a newbie, thank you for your advices, I did some updates followed your advices , and tested on Linux_x86_64 MinGW64 OpenBSD_5.1_amd64 .

  1. remove orconfig.h.in from changed list, so please autoheader before configure .
  2. updated src/win32/orconfig.h , but I got no MSVC installed, so should somebody help me to test it on MSVC32 & MSVC64 ?
  3. updated src/common/procmon.c in Werror.3.patch ,
  4. rename INTPTR_T_to_INT to INTEGER_TYPE_AS_INTPTR_T

+ char s1[1000000], s2[10], s3[10], ch;
is for the OpenBSD platform:
cc1: warnings being treated as errors
test_util.c: In function 'test_util_sscanf':
test_util.c:1488: warning: Array size (20) smaller than format string size (1000000) (arg 3)
test_util.c:1593: warning: Array size (20) smaller than format string size (1000) (arg 3)
Anyone have good suggestion to solve this ?

my goal is export CFLAGS='-Wextra -Werror' before configure , and everything still goes well.

Changed 8 years ago by yayooo

Attachment: MinGW64.3.patch added

Changed 8 years ago by yayooo

Attachment: Werror.3.patch added

comment:7 Changed 8 years ago by nickm

Okay; I've split these up, forward-ported to 0.2.4, and taken a different approach to the unit test compilation issues. (The failing cases should have been using test_eq_ptr, rather than test_eq).

Please review branch "win64-7260" in my public repository (the top five commits at https://gitweb.torproject.org/nickm/tor.git/shortlog/refs/heads/win64-7260 currently).

I don't see the advantage to "CFLAGS='-Wextra -Werror'" over "./configure --enable-gcc-warnings" -- it gives you fewer warnings, *and* forces us not to use standard autoconf macros.

The "char s1[10000000]" fix feels wrong; the point of the first test case is to make sure that tor_sscanf rejects scanf widths over 9999. I wonder if you can get around those warnings by changing the argument of tor_sscanf from s1 to s_ptr, where s_ptr is declared as "char *s_ptr = s1;"

comment:8 Changed 8 years ago by nickm

Keywords: tor-relay added
Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

(Ping? If nobody's going to review this, it makes it harder to merge.)

comment:9 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

It still looks good to me. Please reopen if there are remaining issues other than the configure ones.

Note: See TracTickets for help on using tickets.