New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gtls: fix build when sizeof(long) < sizeof(void *) #1617

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@dscho
Contributor

dscho commented Jun 26, 2017

This is the case most notably on Windows.

Signed-off-by: Johannes Schindelin johannes.schindelin@gmx.de

@mention-bot

This comment has been minimized.

mention-bot commented Jun 26, 2017

@dscho, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @bagder and @rousskov to be potential reviewers.

@dscho

This comment has been minimized.

Contributor

dscho commented Jun 26, 2017

This issue came up while working on #1601.

@coveralls

This comment has been minimized.

coveralls commented Jun 26, 2017

Coverage Status

Coverage increased (+0.01%) to 73.823% when pulling 8a561d1 on dscho:fix-gnutls-build into 922f800 on curl:master.

@jay jay added the build label Jun 27, 2017

@jay

This comment has been minimized.

Member

jay commented Jun 27, 2017

unfortunately we don't have intptr_t in c89. we could use ptrdiff_t which is not technically correct since it's only meant to hold the difference between two members of the same array. it seems like casting between (gnutls_transport_ptr) would make more sense but it looks like those macros were added to fix the warning that comes from doing that.

/cc @gknauf

@dscho dscho force-pushed the dscho:fix-gnutls-build branch from 8a561d1 to d98824e Jun 27, 2017

@dscho

This comment has been minimized.

Contributor

dscho commented Jun 27, 2017

unfortunately we don't have intptr_t in c89.

Good point.

we could use ptrdiff_t which is not technically correct since it's only meant to hold the difference between two members of the same array.

I took this idea a little further: if we pretend that we have a hypothetical character array starting at NULL, we can convert an integer i to a pointer by taking the address of the ith element. Converting back is simply a matter of casting the pointer difference with NULL back to an int.

The patch was updated accordingly.

I verified that this fixes the compiler warning on Windows.

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2017

Coverage Status

Coverage increased (+0.008%) to 73.819% when pulling d98824e on dscho:fix-gnutls-build into 922f800 on curl:master.

@dscho

This comment has been minimized.

Contributor

dscho commented Jun 29, 2017

@jay @gknauf any chance you can have a look at this rather tiny PR? 😄

@bagder

bagder approved these changes Jun 30, 2017 edited

It is a bit funny-looking, but should not cause any troubles as long as the result actually fits in 32 bits... And also, it was made funny before this patch.

@bagder

This comment has been minimized.

Member

bagder commented Jun 30, 2017

@jay any objection?

jay added a commit to jay/curl that referenced this pull request Jun 30, 2017

gtls: fix build when sizeof(long) < sizeof(void *)
- Change gnutls pointer/int macros to pointer/curl_socket_t.
  Prior to this change they used long type as well.

The size of the `long` data type can be shorter than that of pointer
types. This is the case most notably on Windows.

If C99 were acceptable, we could simply use `intptr_t` here. But we
want to retain C89 compatibility.

Simply use the trick of performing pointer arithmetic with the NULL
pointer: to convert an integer `i` to a pointer, simply take the
address of the `i`th element of a hypothetical character array
starting at address NULL. To convert back, simply cast the pointer
difference.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Closes curl#1617
@jay

This comment has been minimized.

Member

jay commented Jun 30, 2017

@jay any objection?

No. It seems like it's UB either way. To touch on what you said about 32 bits I think it could be improved if we rename them socket macros and cast to curl_socket_t since that's the type. Barring input from @gknauf how about these modifications to @dscho's work https://github.com/curl/curl/compare/master...jay:pr_1617_amended?expand=1 . I'm not sure if it would resurrect the warnings though

@bagder

This comment has been minimized.

Member

bagder commented Jul 2, 2017

could be improved if we rename them socket macros and cast to curl_socket_t since that's the type

I like that. Also, the underlying socket type on win64 (SOCKET) is larger than int/long, so I think that makes a lot of sense. What do you think @dscho ?

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 3, 2017

@jay sadly, this does not work:

vtls/gtls.c: In function 'gtls_connect_step1':
vtls/gtls.c:69:27: error: invalid operands to binary + (have 'char *' and 'char *')
   ((void *) ((char *)NULL + (char *)(s)))
                           ^ ~~~~~~~~~~~
vtls/gtls.c:852:21: note: in expansion of macro 'GNUTLS_SOCKET_TO_POINTER_CAST'
     transport_ptr = GNUTLS_SOCKET_TO_POINTER_CAST(conn->sock[sockindex]);
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gtls: fix build when sizeof(long) < sizeof(void *)
- Change gnutls pointer/int macros to pointer/curl_socket_t.
  Prior to this change they used long type as well.

The size of the `long` data type can be shorter than that of pointer
types. This is the case most notably on Windows.

If C99 were acceptable, we could simply use `intptr_t` here. But we
want to retain C89 compatibility.

Simply use the trick of performing pointer arithmetic with the NULL
pointer: to convert an integer `i` to a pointer, simply take the
address of the `i`th element of a hypothetical character array
starting at address NULL. To convert back, simply cast the pointer
difference.

Thanks to Jay Satiro for the initial modification to use curl_socket_t
instead of int/long.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

@dscho dscho force-pushed the dscho:fix-gnutls-build branch from d98824e to 8c88e88 Jul 3, 2017

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 3, 2017

I fixed that build error and amended jay@537b26c accordingly, force-pushing to the branch (and hence updating this PR).

Good to go?

@bagder

This comment has been minimized.

Member

bagder commented Jul 3, 2017

👍 let's just first see that the CI doesn't find something unexpected

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 3, 2017

let's just fist see that the CI doesn't find something unexpected

Of course! ;-)

@coveralls

This comment has been minimized.

coveralls commented Jul 3, 2017

Coverage Status

Coverage increased (+0.008%) to 74.011% when pulling 8c88e88 on dscho:fix-gnutls-build into 3a48a13 on curl:master.

@bagder bagder closed this in c0cdc68 Jul 3, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jul 3, 2017

Thanks!

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 3, 2017

Everything seems to pass ;-)

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 3, 2017

Oh, I missed that you closed this already. Sorry!

@dscho dscho deleted the dscho:fix-gnutls-build branch Jul 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment