tatsuhiro-t / spdylay

The experimental SPDY protocol version 2, 3 and 3.1 implementation in C
http://tatsuhiro-t.github.io/spdylay/
MIT License
603 stars 102 forks source link

Incorrect test for event.udata being intptr_t #33

Closed sludin closed 12 years ago

sludin commented 12 years ago

On OS X with gcc == i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1 the following code check for event.udata being an intptr_t:


#include <sys/types.h>
#include <sys/event.h>
#include <sys/time.h>

int
main ()
{
  struct kevent event;
  event.udata = (intptr_t*)(&event);
  ;
  return 0;
}

I believe the proper check should be:

event.udata = (intptr_t)(&event);

Or something similar. The current check as written passes when event.udata us a void*.

In EventPoll_kqueue.cc the following #ifdef block is used:

#ifdef KEVENT_UDATA_INTPTR_T
# define PTR_TO_UDATA(X) (reinterpret_cast<intptr_t>(X))
#else // !KEVENT_UDATA_INTPTR_T
# define PTR_TO_UDATA(X) (X)
#endif // !KEVENT_UDATA_INTPTR_T

Since KEVENT_UDATA_INTPTR_T is set to 1 by the configure check, the:

PTR_TO_UDATA(user_data)

in update_event becomes:

reinterpret_cast<intptr_t>(user_data)

And results in the following error:

error: invalid conversion from ‘intptr_t’ to ‘void*’

tatsuhiro-t commented 12 years ago

Thank you for pointing out. Fixed in the master branch.

sludin commented 12 years ago

Fix is in, but the onion continues to peel back. The problem now comes from the difference between the C cast rules and the C++ cast rules. When compiled with gcc the following succeeds with a warning:

event.udata = (intptr_t)(&event);

Under g++ it gets an error. Should the test be run instead with ac_fn_cxx_try_compile? I am not sure the current test is getting the information you want.

sludin commented 12 years ago

The following diff on configure.ac might do it:


sludin@sfo-mppcg ~/src/spdylay]$ git diff
diff --git a/configure.ac b/configure.ac
index d4bac5a..ee1b176 100644
--- a/configure.ac
+++ b/configure.ac
@@ -169,6 +169,7 @@ AM_CONDITIONAL([HAVE_EPOLL], [ test "x${have_epoll}" = "xyes" ])
 AC_CHECK_FUNCS([kqueue], [have_kqueue=yes])
 AM_CONDITIONAL([HAVE_KQUEUE], [test "x${have_kqueue}" = "xyes"])

+AC_LANG_PUSH(C++)
 AC_MSG_CHECKING([whether struct kevent.udata is intptr_t])
 AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
 #include <sys/types.h>
@@ -185,6 +186,7 @@ if test "x$kevent_udata_intptr_t" = "xyes"; then
   AC_DEFINE([KEVENT_UDATA_INTPTR_T], [1],
             [Define to 1 if struct kevent.udata is intptr_t])
 fi
+AC_LANG_POP()

 if test "x$maintainer_mode" != "xno"; then
     CFLAGS="$CFLAGS -Wall -Wextra -Werror"
tatsuhiro-t commented 12 years ago

Thank you! Merged and pushed.