Fix win32 building with C99 mode

PATCH Superseded Gert Doering 3 participants 12 messages Sep 17, 2016 – Oct 2, 2016 1 message not in the archive

Share & tools

Gert Doering

author

2016-09-17 13:20 UTC #0 lore raw

PATCH 1/1 Fix win32 building with C99 mode

In -std=c99 mode, WIN32 is not defined to be "1" anymore, but just "#define WIN32" - so the "#if WIN32" breaks, needs to be "#ifdef WIN32"

Signed-off-by Gert Doering <[email protected]...>

---
 src/openvpn/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

src/openvpn/misc.c +1 -1

index 0991d79..2982cd0 100644
@@ -977,7 +977,7 @@ hostname_randomize(const char *hostname, struct gc_arena *gc)
 const char *
 gen_path (const char *directory, const char *filename, struct gc_arena *gc)
 {
-#if WIN32
+#ifdef WIN32
   const int CC_PATH_RESERVED = CC_LESS_THAN|CC_GREATER_THAN|CC_COLON|
     CC_DOUBLE_QUOTE|CC_SLASH|CC_BACKSLASH|CC_PIPE|CC_QUESTION_MARK|CC_ASTERISK;
 #else

Signature · 1 line

1.9.1

A message that is not in the archive (sent off-list or not yet received): [email protected]...

Илья Шипицин

2016-09-17 13:40 UTC #2 lore raw

Should we add mingw compiler to travis-ci matrix?

17 сент. 2016 г. 6:34 PM пользователь "Gert Doering" <[email protected]...> написал:

In -std=c99 mode, WIN32 is not defined to be "1" anymore, but just "#define WIN32" - so the "#if WIN32" breaks, needs to be "#ifdef WIN32"

Signed-off-by Gert Doering <[email protected]...>

---
 src/openvpn/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

src/openvpn/misc.c +0 -0

index 0991d79..2982cd0 100644
@@ -977,7 +977,7 @@ hostname_randomize(const char *hostname, struct

gc_arena *gc)
const char *
gen_path (const char *directory, const char *filename, struct gc_arena *gc) {
-#if WIN32
+#ifdef WIN32
const int CC_PATH_RESERVED = CC_LESS_THAN|CC_GREATER_THAN|CC_COLON|
CC_DOUBLE_QUOTE|CC_SLASH|CC_BACKSLASH|CC_PIPE|CC_QUESTION_
MARK|CC_ASTERISK;
#else
--
1.9.1

------------------------------------------------------------
------------------
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Gert Doering

author

2016-09-17 13:58 UTC #3 lore raw

Hi,

Quoted 1 line · ???????? ??????????????

Should we add mingw compiler to travis-ci matrix?

If travis can do mingw builds, that would be good.

But it is complicated.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Илья Шипицин

2016-09-18 11:49 UTC #4 lore raw

we already run openvpn/openvpn-build in travis.
build only is not complicated.

there are minor issues with 14.04 mingw gcc, which is too old for openvpn master. I'll make patch after I will resolve it.

2016-09-17 18:58 GMT+05:00 Gert Doering <[email protected]...>:

Quoted 18 lines

Hi,
Quoted 1 line · ???????? ??????????????
Should we add mingw compiler to travis-ci matrix?
If travis can do mingw builds, that would be good.
But it is complicated.
gert
--
USENET is *not* the non-clickable part of WWW!
                                                           //
www.muc.de/~gert/
Gert Doering - Munich, Germany
[email protected]...
fax: +49-89-35655025 [email protected]...
muenchen.de

Selva Nair

2016-09-17 23:11 UTC #5 lore raw

In reply to Gert Doering

Hi,

Quoted 3 lines · Gert Doering

In -std=c99 mode, WIN32 is not defined to be "1" anymore, but just "#define WIN32" - so the "#if WIN32" breaks, needs to be "#ifdef WIN32"

Indeed...

To depend on the compiler or system headers to define WIN32 was not a great idea anyway. Using _WIN32 or __WIN32 is probably more reliable. Or we should define WIN32 in config.h. With std=c99, it seems WIN32 gets defined by chance through some header pulled-in by syshead.h -- not something one can rely on.

Compiler-defined macros with std=c99:

$ x86_64-w64-mingw32-gcc -std=c99 -E -dM - </dev/null | grep WIN32
#define _WIN32 1
#define __WIN32 1
#define __WIN32__ 1

As for a quick fix, the patch takes care of misc.c, but there is an issue with block_dns.c as well. It does not include the required config.h and syshead.h at the top before #ifdef WIN32, so now the whole file gets skipped -- mea culpa, mea culpa...

I think we can just remove #ifdef WIN32 from block_dns.c (its compiled only on Windows), and add config.h and syshead.h headers at the top. Then we also need -I$(top_srcdir)/src/compat in src/openvpnsev/Makefile.am as compat.h is included by syshead.h.

Thanks,

Selva

Gert Doering

author

2016-09-18 06:59 UTC #6 lore raw

Hi,

Quoted 5 lines · Selva Nair

Quoted 2 lines · Gert Doering
In -std=c99 mode, WIN32 is not defined to be "1" anymore, but just "#define WIN32" - so the "#if WIN32" breaks, needs to be "#ifdef WIN32"

Quoted 7 lines

Indeed...
To depend on the compiler or system headers to define WIN32 was not a great idea anyway. Using _WIN32 or __WIN32 is probably more reliable. Or we should define WIN32 in config.h. With std=c99, it seems WIN32 gets defined by chance through some header pulled-in by syshead.h -- not something one can rely on.

Heiko says that "WIN32 is what everybody is testing for on Windows", so that seems to be an official API define...

With c99, on mingw, the #define WIN32 comes from "minwindef.h", which is included from "windef.h", which is included from "windows.h" -- but I wonder how we ever include that in the first place, given this code in syshead.h

#ifdef WIN32
#include <windows.h>
#include <winsock2.h>
#define sleep(x) Sleep((x)*1000)
#define random rand
#define srandom srand
#endif

... so maybe it is actually *never* set, even with my patch, and we just do not know it's breaking because it's not linking...?

Ah. Here we go... trying to redefine WIN32 at the end of syshead.h shows what is happening:

In file included from /usr/share/mingw-w64/include/windef.h:8:0,

                 from /usr/share/mingw-w64/include/windows.h:69,
                 from /usr/share/mingw-w64/include/winsock2.h:23,
                 from ../../src/compat/compat.h:29,
                 from syshead.h:28,

so compat.h is pulling it via

#ifdef HAVE_WINSOCK2_H
#include <winsock2.h>
#endif

whee...

Quoted 11 lines

Compiler-defined macros with std=c99:
$ x86_64-w64-mingw32-gcc -std=c99 -E -dM - </dev/null | grep WIN32
#define _WIN32 1
#define __WIN32 1
#define __WIN32__ 1
As for a quick fix, the patch takes care of misc.c, but there is an issue with block_dns.c as well. It does not include the required config.h and syshead.h at the top before #ifdef WIN32, so now the whole file gets skipped -- mea culpa, mea culpa...

Quoted 4 lines

I think we can just remove #ifdef WIN32 from block_dns.c (its compiled only on Windows), and add config.h and syshead.h headers at the top. Then we also need -I$(top_srcdir)/src/compat in src/openvpnsev/Makefile.am as compat.h is included by syshead.h.

Do you have time to cook up & test such a patch? Then I'll go and focus a bit more on my windows testing environment in the meantime, and merge the patch later today...

thanks,

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Selva Nair

2016-09-18 14:20 UTC #7 lore raw

Quoted 9 lines · Gert Doering

Ah. Here we go... trying to redefine WIN32 at the end of syshead.h shows what is happening:
In file included from /usr/share/mingw-w64/include/windef.h:8:0,
                 from /usr/share/mingw-w64/include/windows.h:69,
                 from /usr/share/mingw-w64/include/winsock2.h:23,
                 from ../../src/compat/compat.h:29,
                 from syshead.h:28,

Quoted 8 lines

so compat.h is pulling it via
#ifdef HAVE_WINSOCK2_H
#include <winsock2.h>
#endif
whee...

Yeah, to me that only confirms my feeling that #ifdef WIN32 is not reliable. Much better to use _WIN32 which appears to be defined by all (?) C compilers that support windows and doesn't require any particular header to be included.
http://nadeausoftware.com/articles/2012/01/c_c_tip_how_
use_compiler_predefined_macros_detect_operating_system# WindowsCygwinnonPOSIXandMinGW

Anyway, right now the priority being un-break building on windows, your v2 looks good. I'll test it and report back.

Thanks,

Selva

Gert Doering

author

2016-09-18 20:15 UTC #8 lore raw

Hi,

Quoted 10 lines · Selva Nair

Yeah, to me that only confirms my feeling that #ifdef WIN32 is not reliable. Much better to use _WIN32 which appears to be defined by all (?) C compilers that support windows and doesn't require any particular header to be included.
http://nadeausoftware.com/articles/2012/01/c_c_tip_how_
use_compiler_predefined_macros_detect_operating_system# WindowsCygwinnonPOSIXandMinGW
Anyway, right now the priority being un-break building on windows, your v2 looks good. I'll test it and report back.

Thanks for testing and ACKing. Merged.

I think I'll open a trac ticket for the next step, so it isn't lost - we intend to do a major round of formatting cleanup before 2.4, and changing WIN32 to _WIN32 in the process would nicely fit :-)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Gert Doering

author

2016-10-02 17:48 UTC #9 lore raw

Hi,

Quoted 8 lines · Gert Doering

Quoted 7 lines · Selva Nair
Yeah, to me that only confirms my feeling that #ifdef WIN32 is not reliable. Much better to use _WIN32 which appears to be defined by all (?) C compilers that support windows and doesn't require any particular header to be included.
http://nadeausoftware.com/articles/2012/01/c_c_tip_how_
use_compiler_predefined_macros_detect_operating_system# WindowsCygwinnonPOSIXandMinGW

[..]

trac #746

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Gert Doering

author

2016-09-18 12:14 UTC #10 lore raw

PATCH v2 1/1 Fix win32 building with C99 mode

In reply to Gert Doering

In -std=c99 mode, WIN32 is not defined to be "1" anymore, but just "#define WIN32" - so the "#if WIN32" breaks, needs to be "#ifdef WIN32"

v2: also fix block_dns.c (include config.h + compat.h) (Selva Nair)

Signed-off-by Gert Doering <[email protected]...>

---
 src/openvpn/block_dns.c     | 11 +++++++++++
 src/openvpn/misc.c          |  2 +-
 src/openvpnserv/Makefile.am |  2 +-
 3 files changed, 13 insertions(+), 2 deletions(-)

src/openvpn/block_dns.c +11 -0

index af2db18..ee20c68 100644
@@ -24,6 +24,17 @@
  *  59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+#ifdef HAVE_CONFIG_VERSION_H
+#include "config-version.h"
+#endif
+
+#include "syshead.h"
+
 #ifdef WIN32
 
 #include <fwpmu.h>

src/openvpn/misc.c +1 -1

index 0991d79..2982cd0 100644
@@ -977,7 +977,7 @@ hostname_randomize(const char *hostname, struct gc_arena *gc)
 const char *
 gen_path (const char *directory, const char *filename, struct gc_arena *gc)
 {
-#if WIN32
+#ifdef WIN32
   const int CC_PATH_RESERVED = CC_LESS_THAN|CC_GREATER_THAN|CC_COLON|
     CC_DOUBLE_QUOTE|CC_SLASH|CC_BACKSLASH|CC_PIPE|CC_QUESTION_MARK|CC_ASTERISK;
 #else

src/openvpnserv/Makefile.am +1 -1

index 3c757d6..3521a34 100644
@@ -18,7 +18,7 @@ EXTRA_DIST = \
 	openvpnserv.vcxproj.filters
 
 AM_CPPFLAGS = \
-	-I$(top_srcdir)/include -I$(top_srcdir)/src/openvpn
+	-I$(top_srcdir)/include -I$(top_srcdir)/src/openvpn -I$(top_srcdir)/src/compat
 
 if WIN32
 sbin_PROGRAMS = openvpnserv

Signature · 1 line

1.9.1

Selva Nair

2016-09-18 16:38 UTC #11 lore raw

Hi,

Quoted 12 lines · Gert Doering

In -std=c99 mode, WIN32 is not defined to be "1" anymore, but just "#define WIN32" - so the "#if WIN32" breaks, needs to be "#ifdef WIN32"
v2: also fix block_dns.c (include config.h + compat.h) (Selva Nair)
Signed-off-by Gert Doering <[email protected]...>
---
 src/openvpn/block_dns.c     | 11 +++++++++++
 src/openvpn/misc.c          |  2 +-
 src/openvpnserv/Makefile.am |  2 +-
 3 files changed, 13 insertions(+), 2 deletions(-)

The patch looks good, fixes windows cross-compile and appears to work fine (as per a quick run on Win 7). Thanks.

ACK from me.

Selva

Gert Doering

author

2016-09-18 20:10 UTC #12 lore raw

In reply to Gert Doering

Patch has been applied to the master branch.

commit 6cd7e08d89cc9c39d00989866fb4782d5e7041dc

Author: Gert Doering
Date:   Sun Sep 18 14:14:23 2016 +0200

     Fix win32 building with C99 mode

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering