You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jani Averbach <ja...@jaa.iki.fi> on 2003/11/28 14:11:57 UTC
[PATCH] Clean code to be 64bit clean
Hello,
I would like hear some comment before committing this, the first part
of patch should be ok, and current code is erroneus at the moment.
But what about second portion of patch
(subversion/libsvn_ra_dav/session.c), is it quaranteed that
SVN_AUT_SSL_* will never exceed 31 bytes, so is the conversion
int -> long safe, or is this used a (transparent) way that it doesnt matter?
Or should those bitfields be unsigned (the return value of
convert_neon_failures), so bitfield semantic will not change in the
conversion?
BR, Jani
Log:
Fix two pointer to int errors (in 64bit system), and one warning.
* subversion/libsvn_client/ssl_server_trust_providers.c
(ssl_server_trust_file_first_credentials): Changed variables
failures, last_failure to long. Conversio was
void * to int, now it is void * to long.
(ssl_server_trust_prompt_first_cred): Changed failures to long, ditto.
* subversion/libsvn_ra_dav/session.c
(server_ssl_callback): Conversio was int to void *, now it is
int to long to void *. This will prevent a compiler warning.
Index: subversion/libsvn_client/ssl_server_trust_providers.c
===================================================================
--- subversion/libsvn_client/ssl_server_trust_providers.c (revision 7859)
+++ subversion/libsvn_client/ssl_server_trust_providers.c (working copy)
@@ -56,7 +56,7 @@
apr_pool_t *pool)
{
ssl_server_trust_file_provider_baton_t *pb = provider_baton;
- int failures = (int) apr_hash_get (parameters,
+ long failures = (long) apr_hash_get (parameters,
SVN_AUTH_PARAM_SSL_SERVER_FAILURES,
APR_HASH_KEY_STRING);
const svn_auth_ssl_server_cert_info_t *cert_info =
@@ -84,7 +84,7 @@
if (!error && creds_hash)
{
svn_string_t *trusted_cert, *this_cert, *failstr;
- int last_failures;
+ long last_failures;
trusted_cert = apr_hash_get (creds_hash,
SVN_CLIENT__AUTHFILE_ASCII_CERT_KEY,
@@ -211,7 +211,7 @@
apr_pool_t *pool)
{
ssl_server_trust_prompt_provider_baton_t *pb = provider_baton;
- int failures = (int) apr_hash_get (parameters,
+ long failures = (long) apr_hash_get (parameters,
SVN_AUTH_PARAM_SSL_SERVER_FAILURES,
APR_HASH_KEY_STRING);
const svn_auth_ssl_server_cert_info_t *cert_info =
Index: subversion/libsvn_ra_dav/session.c
===================================================================
--- subversion/libsvn_ra_dav/session.c (revision 7859)
+++ subversion/libsvn_ra_dav/session.c (working copy)
@@ -169,7 +169,7 @@
svn_auth_set_parameter(ras->callbacks->auth_baton,
SVN_AUTH_PARAM_SSL_SERVER_FAILURES,
- (void*)convert_neon_failures(failures));
+ (void*)((long)convert_neon_failures(failures)));
/* Extract the info from the certificate */
cert_info.hostname = ne_ssl_cert_identity(cert);
--
Jani Averbach
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Clean code to be 64bit clean
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2003-11-28 at 09:11, Jani Averbach wrote:
> I would like hear some comment before committing this, the first part
> of patch should be ok, and current code is erroneus at the moment.
I think, for the sake of cleanliness, we should stop trying to stuff an
integral value into a void *, even if we can guarantee that it will
always work.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Clean code to be 64bit clean
Posted by Philip Martin <ph...@codematters.co.uk>.
"Jay 'Whip' Grizzard" <el...@lupine.org> writes:
>> It's a bit off-topic, but the ISO C 99 stdint.h header optionally
>> provides intptr_t and uintptr_t. When these types are provided it is
>> guaranteed that a void* can be cast to a [u]intptr_t and then cast
>> back to the original void* value.
>
> I'm not familiar with the C99 spec (someone feel free to buy me a copy! ;),
> but from the names of the types, intptr_t (etc) seem to be integer pointers.
They are integer types, not pointers. The C99 standard, unlike the
C89 standard, is available at a reasonable price online from ANSI (I
suppose technically one is supposed to be in the USA to take advantage
of their service). You want ISO/IEC 9899:1999.
> The subversion code being talked about doesn't convert int pointers back
> and forth from (void *) -- it converts (int) back and forth from (void *).
>
> This is not, has not been, and never will be either legal, or a good
> thing to do.
Agreed. The correct solution is to apr_palloc a char/int/long, put
the value in the allocated memory, and store the address in the void*.
On the other hand, there is a long history of C programmers converting
integers to and from pointers and getting away with it.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Clean code to be 64bit clean
Posted by Branko Čibej <br...@xbc.nu>.
Jay 'Whip' Grizzard wrote:
>>>Moreover, in general long is only
>>>type that is supposed (in practice) to hold void *, but this is not
>>>required by standard, so you should not count on it everywhere.
>>>
>>>
>>It's a bit off-topic, but the ISO C 99 stdint.h header optionally
>>provides intptr_t and uintptr_t. When these types are provided it is
>>guaranteed that a void* can be cast to a [u]intptr_t and then cast
>>back to the original void* value.
>>
>>
>
>I'm not familiar with the C99 spec (someone feel free to buy me a copy! ;),
>but from the names of the types, intptr_t (etc) seem to be integer pointers.
>
>
Well, I'm impressed. First you admit you don't know what you're talking
about, then you proceed at great length. :-)
intptr_t is an integer type that is wide enough to store a void* without
loss of precision, and a cast from void* to intptr_t and back is
guaranteed to preserve the pointer value.
>The subversion code being talked about doesn't convert int pointers back
>and forth from (void *) -- it converts (int) back and forth from (void *).
>
>This is not, has not been, and never will be either legal,
>
There is no legal code. There is only valid code (or better yet,
standard-conforming code).
And besides, C99 does specify the semantics of pointer-to-int
conversions, 6.3.2.3 paragraphs 5 and 6.
>or a good thing to do.
>
Depends on what you're doing, right?
> There only requirement with (void *) is that any type of
>pointer can be cast to (void *) and then back to its original type.
>
>
Nope. That guarantee only holds for object pointer types, not for
function pointers.
>There's no guarantee that any other type can be cast to/from (void *)
>without damage --
>
Nope again, casts to intptr_t are well-defined, see above.
>And, indeed, on some obscure platforms (admittedly, not
>any that subversion supports, I don't think), there actually aren't any
>integer datatypes capable of holding pointer datatypes without damage.
>
>
In that case, intptr_t won't be defined on that platform. Although note
that intptr_t doesn't have to be an alias for any other integer type, so
it's quite all right for it to be wider than "long long", for example.
>The right answer to this type of problem is almost always "write the code
>correctly" -- which is to say, don't treat (void *) and ([long] int) as
>interchangeable, because they aren't.
>
I can agree with that part, anyway. :-)
Although again, in this case, the void* cast to int is never interpreted
as a pointer.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Clean code to be 64bit clean
Posted by Jay 'Whip' Grizzard <el...@lupine.org>.
> > Moreover, in general long is only
> > type that is supposed (in practice) to hold void *, but this is not
> > required by standard, so you should not count on it everywhere.
>
> It's a bit off-topic, but the ISO C 99 stdint.h header optionally
> provides intptr_t and uintptr_t. When these types are provided it is
> guaranteed that a void* can be cast to a [u]intptr_t and then cast
> back to the original void* value.
I'm not familiar with the C99 spec (someone feel free to buy me a copy! ;),
but from the names of the types, intptr_t (etc) seem to be integer pointers.
The subversion code being talked about doesn't convert int pointers back
and forth from (void *) -- it converts (int) back and forth from (void *).
This is not, has not been, and never will be either legal, or a good
thing to do. There only requirement with (void *) is that any type of
pointer can be cast to (void *) and then back to its original type.
There's no guarantee that any other type can be cast to/from (void *)
without damage -- And, indeed, on some obscure platforms (admittedly, not
any that subversion supports, I don't think), there actually aren't any
integer datatypes capable of holding pointer datatypes without damage.
The right answer to this type of problem is almost always "write the code
correctly" -- which is to say, don't treat (void *) and ([long] int) as
interchangeable, because they aren't. Given the generally high quality
of the subversion code, I'm impressed that such poorly behaved code
could even get into the codebase...
Of course, I'm just a random pedant off in the wings somewhere... I'll be
happy to take my $0.02 and go home now... :)
-jay
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Clean code to be 64bit clean
Posted by Philip Martin <ph...@codematters.co.uk>.
Jani Averbach <ja...@jaa.iki.fi> writes:
> Moreover, in general long is only
> type that is supposed (in practice) to hold void *, but this is not
> required by standard, so you should not count on it everywhere.
It's a bit off-topic, but the ISO C 99 stdint.h header optionally
provides intptr_t and uintptr_t. When these types are provided it is
guaranteed that a void* can be cast to a [u]intptr_t and then cast
back to the original void* value.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Clean code to be 64bit clean
Posted by Jani Averbach <ja...@jaa.iki.fi>.
On 2003-11-30 21:58+0100, Tobias Ringstrom wrote:
> Jani Averbach wrote:
> >
> >This message is a little bit bogus, only problem seems to be that
> >there are tree compiler warnings of void * to integer conversion or
> >vice versa (when orginal data has been integer any way from the start).
>
> I'm not sure I understand what the problem is here. Do you really get
> a compiler warning if you cast a void* to an int?
Yep [1], this is a 64bit [2] system. It seems to be that in 32bit gcc
systems sizeof(int) == sizeof(long). Moreover, in general long is only
type that is supposed (in practice) to hold void *, but this is not
required by standard, so you should not count on it everywhere.
So in theory casting void * -> int is not a safe operation, but when
we are using void * just carrying data, in fact, it is.
BR, Jani
1)
../svn/trunk/subversion/libsvn_client/ssl_server_trust_providers.c:61:
warning: cast from pointer to integer of different size
...
2)
#include <stdio.h>
int
main()
{
printf("int %d\n", sizeof(int));
printf("long %d\n", sizeof(long));
printf("void * %d\n", sizeof(void *));
return 0;
}
./a.out
int 4
long 8
void * 8
--
Jani Averbach
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Clean code to be 64bit clean
Posted by Tobias Ringstrom <to...@ringstrom.mine.nu>.
Jani Averbach wrote:
> Hi,
>
> This message is a little bit bogus, only problem seems to be that
> there are tree compiler warnings of void * to integer conversion or
> vice versa (when orginal data has been integer any way from the start).
I'm not sure I understand what the problem is here. Do you really get
a compiler warning if you cast a void* to an int?
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Clean code to be 64bit clean
Posted by Jani Averbach <ja...@jaa.iki.fi>.
Hi,
This message is a little bit bogus, only problem seems to be that
there are tree compiler warnings of void * to integer conversion or
vice versa (when orginal data has been integer any way from the start).
Sorry about noise.
BR, Jani
On 2003-11-28 07:11-0700, Jani Averbach wrote:
>
> Hello,
>
> I would like hear some comment before committing this, the first part
> of patch should be ok, and current code is erroneus at the moment.
>
> But what about second portion of patch
> (subversion/libsvn_ra_dav/session.c), is it quaranteed that
> SVN_AUT_SSL_* will never exceed 31 bytes, so is the conversion
> int -> long safe, or is this used a (transparent) way that it doesnt matter?
> Or should those bitfields be unsigned (the return value of
> convert_neon_failures), so bitfield semantic will not change in the
> conversion?
>
> BR, Jani
>
> Log:
>
> Fix two pointer to int errors (in 64bit system), and one warning.
>
>
> * subversion/libsvn_client/ssl_server_trust_providers.c
> (ssl_server_trust_file_first_credentials): Changed variables
> failures, last_failure to long. Conversio was
> void * to int, now it is void * to long.
>
> (ssl_server_trust_prompt_first_cred): Changed failures to long, ditto.
>
> * subversion/libsvn_ra_dav/session.c
> (server_ssl_callback): Conversio was int to void *, now it is
> int to long to void *. This will prevent a compiler warning.
>
>
> Index: subversion/libsvn_client/ssl_server_trust_providers.c
> ===================================================================
> --- subversion/libsvn_client/ssl_server_trust_providers.c (revision 7859)
> +++ subversion/libsvn_client/ssl_server_trust_providers.c (working copy)
> @@ -56,7 +56,7 @@
> apr_pool_t *pool)
> {
> ssl_server_trust_file_provider_baton_t *pb = provider_baton;
> - int failures = (int) apr_hash_get (parameters,
> + long failures = (long) apr_hash_get (parameters,
> SVN_AUTH_PARAM_SSL_SERVER_FAILURES,
> APR_HASH_KEY_STRING);
> const svn_auth_ssl_server_cert_info_t *cert_info =
> @@ -84,7 +84,7 @@
> if (!error && creds_hash)
> {
> svn_string_t *trusted_cert, *this_cert, *failstr;
> - int last_failures;
> + long last_failures;
>
> trusted_cert = apr_hash_get (creds_hash,
> SVN_CLIENT__AUTHFILE_ASCII_CERT_KEY,
> @@ -211,7 +211,7 @@
> apr_pool_t *pool)
> {
> ssl_server_trust_prompt_provider_baton_t *pb = provider_baton;
> - int failures = (int) apr_hash_get (parameters,
> + long failures = (long) apr_hash_get (parameters,
> SVN_AUTH_PARAM_SSL_SERVER_FAILURES,
> APR_HASH_KEY_STRING);
> const svn_auth_ssl_server_cert_info_t *cert_info =
> Index: subversion/libsvn_ra_dav/session.c
> ===================================================================
> --- subversion/libsvn_ra_dav/session.c (revision 7859)
> +++ subversion/libsvn_ra_dav/session.c (working copy)
> @@ -169,7 +169,7 @@
>
> svn_auth_set_parameter(ras->callbacks->auth_baton,
> SVN_AUTH_PARAM_SSL_SERVER_FAILURES,
> - (void*)convert_neon_failures(failures));
> + (void*)((long)convert_neon_failures(failures)));
>
> /* Extract the info from the certificate */
> cert_info.hostname = ne_ssl_cert_identity(cert);
>
>
>
> --
> Jani Averbach
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
Jani Averbach jaa@iki.fi +1-303-413-1047
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org