[PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Stan C. Smith]

Hello,
  In osm_vendor_ibumad_sa.c the casting of a pointer to a long causes data truncation problems in 64-bit Windows where sizeof(long)
!= sizeof(void*).
'uintptr_t' is correct in both operating environments.

thank you,

stan.

signed-off-by: stan smith <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

diff --git a/opensm/libvendor/osm_vendor_ibumad_sa.c b/opensm/libvendor/osm_vendor_ibumad_sa.c
index 1fdcc47..9180972 100644
--- a/opensm/libvendor/osm_vendor_ibumad_sa.c
+++ b/opensm/libvendor/osm_vendor_ibumad_sa.c
@@ -85,7 +85,7 @@ __osmv_sa_mad_rcv_cb(IN osm_madw_t * p_madw,
 
 	/* obtain the sent context since we store it during send in the ni_ctx */
 	p_query_req_copy =
-	    (osmv_query_req_t *) (long *)(long)(p_req_madw->context.ni_context.
+	    (osmv_query_req_t *) (uintptr_t)(p_req_madw->context.ni_context.
 						node_guid);
 
 	/* provide the context of the original request in the result */
@@ -181,7 +181,7 @@ static void __osmv_sa_mad_err_cb(IN void *bind_context, IN osm_madw_t * p_madw)
 
 	/* Obtain the sent context etc */
 	p_query_req_copy =
-	    (osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context.
+	    (osmv_query_req_t *) (uintptr_t)(p_madw->context.ni_context.
 						node_guid);
 
 	/* provide the context of the original request in the result */
@@ -433,7 +433,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t * p_bind,
 	}
 	*p_query_req_copy = *p_query_req;
 	p_madw->context.ni_context.node_guid =
-	    (ib_net64_t) (long)p_query_req_copy;
+	    (ib_net64_t) (uintptr_t)p_query_req_copy;
 
 	/* we can support async as well as sync calls */
 	sync = ((p_query_req->flags & OSM_SA_FLAGS_SYNC) == OSM_SA_FLAGS_SYNC);

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Sasha Khapyorsky]

On 14:12 Thu 09 Sep     , Stan C. Smith wrote:
> 
> Hello,
>   In osm_vendor_ibumad_sa.c the casting of a pointer to a long causes data truncation problems in 64-bit Windows where sizeof(long)
> != sizeof(void*).
> 'uintptr_t' is correct in both operating environments.

Wouldn't it be better to remove those additional castings at all?
Like below?

> 
> thank you,
> 
> stan.
> 
> signed-off-by: stan smith <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> diff --git a/opensm/libvendor/osm_vendor_ibumad_sa.c b/opensm/libvendor/osm_vendor_ibumad_sa.c
> index 1fdcc47..9180972 100644
> --- a/opensm/libvendor/osm_vendor_ibumad_sa.c
> +++ b/opensm/libvendor/osm_vendor_ibumad_sa.c
> @@ -85,7 +85,7 @@ __osmv_sa_mad_rcv_cb(IN osm_madw_t * p_madw,
>  
>  	/* obtain the sent context since we store it during send in the ni_ctx */
>  	p_query_req_copy =
> -	    (osmv_query_req_t *) (long *)(long)(p_req_madw->context.ni_context.
> +	    (osmv_query_req_t *) (uintptr_t)(p_req_madw->context.ni_context.
>  						node_guid);

	p_query_req_copy = (osmv_query_req_t *) p_req_madw->context.ni_context.node_guid;

>  
>  	/* provide the context of the original request in the result */
> @@ -181,7 +181,7 @@ static void __osmv_sa_mad_err_cb(IN void *bind_context, IN osm_madw_t * p_madw)
>  
>  	/* Obtain the sent context etc */
>  	p_query_req_copy =
> -	    (osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context.
> +	    (osmv_query_req_t *) (uintptr_t)(p_madw->context.ni_context.
>  						node_guid);

 	p_query_req_copy = (osmv_query_req_t *) p_madw->context.ni_context.node_guid;

>  
>  	/* provide the context of the original request in the result */
> @@ -433,7 +433,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t * p_bind,
>  	}
>  	*p_query_req_copy = *p_query_req;
>  	p_madw->context.ni_context.node_guid =
> -	    (ib_net64_t) (long)p_query_req_copy;
> +	    (ib_net64_t) (uintptr_t)p_query_req_copy;

	p_madw->context.ni_context.node_guid = (ib_net64_t) p_query_req_copy;

?

Sasha

>  
>  	/* we can support async as well as sync calls */
>  	sync = ((p_query_req->flags & OSM_SA_FLAGS_SYNC) == OSM_SA_FLAGS_SYNC);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Hefty, Sean]

> Wouldn't it be better to remove those additional castings at all?
> Like below?
> 
..
> 	p_query_req_copy = (osmv_query_req_t *) p_req_madw->context.ni_context.node_guid;

I think the Windows compiler will complain about data loss on a 32-bit system.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Sasha Khapyorsky]

On 10:22 Tue 30 Nov     , Hefty, Sean wrote:
> > Wouldn't it be better to remove those additional castings at all?
> > Like below?
> > 
> ..
> > 	p_query_req_copy = (osmv_query_req_t *) p_req_madw->context.ni_context.node_guid;
> 
> I think the Windows compiler will complain about data loss on a 32-bit system.

p_req_madw->context.ni_context.node_guid has 64 bits type.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Jason Gunthorpe]

On Tue, Nov 30, 2010 at 08:57:54PM +0200, Sasha Khapyorsky wrote:
> On 10:22 Tue 30 Nov     , Hefty, Sean wrote:
> > > Wouldn't it be better to remove those additional castings at all?
> > > Like below?
> > > 
> > ..
> > > 	p_query_req_copy = (osmv_query_req_t *) p_req_madw->context.ni_context.node_guid;
> > 
> > I think the Windows compiler will complain about data loss on a 32-bit system.
> 
> p_req_madw->context.ni_context.node_guid has 64 bits type.

But the pointer has 32 bits, so the compiler warns you it chucked the
upper 32 bits of node_guid.

The correct cast in C99 is (osmv_query_req_t *)(unitptr_t)

But one should look upon all of these with great suspicion and take a
moment to figure out where the original non-pointer came from. It
better not be from network data or you have a security problem.

Just reading the line from here looks insane, casting a GUID to a
pointer? :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Smith, Stan]

Hefty, Sean wrote:
>> Wouldn't it be better to remove those additional castings at all?
>> Like below?
>>
> ..
>>      p_query_req_copy = (osmv_query_req_t *)
>> p_req_madw->context.ni_context.node_guid;
>
> I think the Windows compiler will complain about data loss on a
> 32-bit system.

Only the case @ line 433 compiler wise, requires the (uintptr_t) to eliminate the x86 compiler warning: conversion from ptr to ib_net64_t is sign-extended which may result in unexpected runtime behaviors.
Otherwise, x64 & x86 compilers digest the single cast approach.
The real question becomes, are the runtime truncations observed by Mellanox still present with the single cast?

> @@ -433,7 +433,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t * p_bind,
>       }
>       *p_query_req_copy = *p_query_req;
>       p_madw->context.ni_context.node_guid =
> -         (ib_net64_t) (long)p_query_req_copy;
> +         (ib_net64_t) (uintptr_t)p_query_req_copy;

        p_madw->context.ni_context.node_guid = (ib_net64_t) p_query_req_copy;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Smith, Stan]

Sasha Khapyorsky wrote:
> On 14:12 Thu 09 Sep     , Stan C. Smith wrote:
>>
>> Hello,
>>   In osm_vendor_ibumad_sa.c the casting of a pointer to a long
>> causes data truncation problems in 64-bit Windows where sizeof(long)
>> != sizeof(void*). 'uintptr_t' is correct in both operating
>> environments.
>
> Wouldn't it be better to remove those additional castings at all?
> Like below?

Hello,
  I assume the original casts were there for a reason hence the cumbersome double cast?
Mellanox had reported a truncation problem which was fixed with the (uintptr_t) replacement of (long*)(long).
It certainly make more sense to use the single cast if it works; no truncation.
I'll check with those who discovered the truncation and see if they tried the obvious single cast approach.

stan.

>
>>
>> thank you,
>>
>> stan.
>>
>> signed-off-by: stan smith <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> diff --git a/opensm/libvendor/osm_vendor_ibumad_sa.c
>> b/opensm/libvendor/osm_vendor_ibumad_sa.c
>> index 1fdcc47..9180972 100644
>> --- a/opensm/libvendor/osm_vendor_ibumad_sa.c
>> +++ b/opensm/libvendor/osm_vendor_ibumad_sa.c
>> @@ -85,7 +85,7 @@ __osmv_sa_mad_rcv_cb(IN osm_madw_t * p_madw,
>>
>>      /* obtain the sent context since we store it during send in the
>> ni_ctx */    p_query_req_copy = -        (osmv_query_req_t *) (long
>> *)(long)(p_req_madw->context.ni_context. +       (osmv_query_req_t *)
>>                                              (uintptr_t)(p_req_madw->context.ni_context. node_guid);
>
>       p_query_req_copy = (osmv_query_req_t *)
> p_req_madw->context.ni_context.node_guid;
>
>>
>>      /* provide the context of the original request in the result */
>> @@ -181,7 +181,7 @@ static void __osmv_sa_mad_err_cb(IN void
>> *bind_context, IN osm_madw_t * p_madw)
>>
>>      /* Obtain the sent context etc */
>>      p_query_req_copy =
>> -        (osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context.
>> +        (osmv_query_req_t *) (uintptr_t)(p_madw->context.ni_context.
>>                                              node_guid);
>
>       p_query_req_copy = (osmv_query_req_t *)
> p_madw->context.ni_context.node_guid;
>
>>
>>      /* provide the context of the original request in the result */
>> @@ -433,7 +433,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t *
>>      p_bind,         } *p_query_req_copy = *p_query_req;
>>      p_madw->context.ni_context.node_guid =
>> -        (ib_net64_t) (long)p_query_req_copy;
>> +        (ib_net64_t) (uintptr_t)p_query_req_copy;
>
>       p_madw->context.ni_context.node_guid = (ib_net64_t) p_query_req_copy;
>
> ?
>
> Sasha
>
>>
>>      /* we can support async as well as sync calls */
>>      sync = ((p_query_req->flags & OSM_SA_FLAGS_SYNC) ==
>> OSM_SA_FLAGS_SYNC);

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Stan C. Smith]

Hello,
  Your suggestion of removing extra casts does operate correctly under windows x64/x86 variants.
Windows compilers have improved.

thanks,

Stan.

Signed-off-by: stan smith <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

diff --git a/opensm/libvendor/osm_vendor_ibumad_sa.c b/opensm/libvendor/osm_vendor_ibumad_sa.c
index 1fdcc47..63728ad 100644
--- a/opensm/libvendor/osm_vendor_ibumad_sa.c
+++ b/opensm/libvendor/osm_vendor_ibumad_sa.c
@@ -85,8 +85,7 @@ __osmv_sa_mad_rcv_cb(IN osm_madw_t * p_madw,
 
 	/* obtain the sent context since we store it during send in the ni_ctx */
 	p_query_req_copy =
-	    (osmv_query_req_t *) (long *)(long)(p_req_madw->context.ni_context.
-						node_guid);
+	    (osmv_query_req_t *) p_req_madw->context.ni_context.node_guid;
 
 	/* provide the context of the original request in the result */
 	query_res.query_context = p_query_req_copy->query_context;
@@ -181,8 +180,7 @@ static void __osmv_sa_mad_err_cb(IN void *bind_context, IN osm_madw_t * p_madw)
 
 	/* Obtain the sent context etc */
 	p_query_req_copy =
-	    (osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context.
-						node_guid);
+	    (osmv_query_req_t *) p_madw->context.ni_context.node_guid;
 
 	/* provide the context of the original request in the result */
 	query_res.query_context = p_query_req_copy->query_context;
@@ -433,7 +431,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t * p_bind,
 	}
 	*p_query_req_copy = *p_query_req;
 	p_madw->context.ni_context.node_guid =
-	    (ib_net64_t) (long)p_query_req_copy;
+	    (ib_net64_t) (uintptr_t)p_query_req_copy;
 
 	/* we can support async as well as sync calls */
 	sync = ((p_query_req->flags & OSM_SA_FLAGS_SYNC) == OSM_SA_FLAGS_SYNC);

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Sasha Khapyorsky]

On 15:55 Mon 06 Dec     , Stan C. Smith wrote:
> 
> Hello,
>   Your suggestion of removing extra casts does operate correctly under windows x64/x86 variants.
> Windows compilers have improved.
> 
> thanks,
> 
> Stan.
> 
> Signed-off-by: stan smith <stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Applied. Thanks.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Hal Rosenstock]

On 12/6/2010 6:55 PM, Stan C. Smith wrote:
>
> Hello,
>    Your suggestion of removing extra casts does operate correctly under windows x64/x86 variants.
> Windows compilers have improved.
>
> thanks,
>
> Stan.
>
> Signed-off-by: stan smith<stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> diff --git a/opensm/libvendor/osm_vendor_ibumad_sa.c b/opensm/libvendor/osm_vendor_ibumad_sa.c
> index 1fdcc47..63728ad 100644
> --- a/opensm/libvendor/osm_vendor_ibumad_sa.c
> +++ b/opensm/libvendor/osm_vendor_ibumad_sa.c
> @@ -85,8 +85,7 @@ __osmv_sa_mad_rcv_cb(IN osm_madw_t * p_madw,
>
>   	/* obtain the sent context since we store it during send in the ni_ctx */
>   	p_query_req_copy =
> -	    (osmv_query_req_t *) (long *)(long)(p_req_madw->context.ni_context.
> -						node_guid);
> +	    (osmv_query_req_t *) p_req_madw->context.ni_context.node_guid;
>
>   	/* provide the context of the original request in the result */
>   	query_res.query_context = p_query_req_copy->query_context;
> @@ -181,8 +180,7 @@ static void __osmv_sa_mad_err_cb(IN void *bind_context, IN osm_madw_t * p_madw)
>
>   	/* Obtain the sent context etc */
>   	p_query_req_copy =
> -	    (osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context.
> -						node_guid);
> +	    (osmv_query_req_t *) p_madw->context.ni_context.node_guid;

On an x86 Linux machine, the above 2 instances changed now complain 
about warning: cast to pointer from integer of different size

so should those be cast to (osmv_query_req_t *) (long) or 
(osmv_query_req_t *) (uintptr_t) or something else ?

-- Hal
	
>
>   	/* provide the context of the original request in the result */
>   	query_res.query_context = p_query_req_copy->query_context;
> @@ -433,7 +431,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t * p_bind,
>   	}
>   	*p_query_req_copy = *p_query_req;
>   	p_madw->context.ni_context.node_guid =
> -	    (ib_net64_t) (long)p_query_req_copy;
> +	    (ib_net64_t) (uintptr_t)p_query_req_copy;
>
>   	/* we can support async as well as sync calls */
>   	sync = ((p_query_req->flags&  OSM_SA_FLAGS_SYNC) == OSM_SA_FLAGS_SYNC);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Smith, Stan]

Hal Rosenstock wrote:
> On 12/6/2010 6:55 PM, Stan C. Smith wrote:
>>
>> Hello,
>>    Your suggestion of removing extra casts does operate correctly
>> under windows x64/x86 variants. Windows compilers have improved.
>>
>> thanks,
>>
>> Stan.
>>
>> Signed-off-by: stan smith<stan.smith-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> diff --git a/opensm/libvendor/osm_vendor_ibumad_sa.c
>> b/opensm/libvendor/osm_vendor_ibumad_sa.c index 1fdcc47..63728ad
>> 100644 --- a/opensm/libvendor/osm_vendor_ibumad_sa.c +++
>> b/opensm/libvendor/osm_vendor_ibumad_sa.c @@ -85,8 +85,7 @@
>> __osmv_sa_mad_rcv_cb(IN osm_madw_t * p_madw,
>>
>>      /* obtain the sent context since we store it during send in the
>> ni_ctx */    p_query_req_copy =
>> -        (osmv_query_req_t *) (long
>> *)(long)(p_req_madw->context.ni_context.
>> -                                            node_guid);
>> +        (osmv_query_req_t *) p_req_madw->context.ni_context.node_guid;
>>
>>      /* provide the context of the original request in the result */
>>      query_res.query_context = p_query_req_copy->query_context;
>> @@ -181,8 +180,7 @@ static void __osmv_sa_mad_err_cb(IN void
>> *bind_context, IN osm_madw_t * p_madw)
>>
>>      /* Obtain the sent context etc */
>>      p_query_req_copy =
>> -        (osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context.
>> -                                            node_guid);
>> +        (osmv_query_req_t *) p_madw->context.ni_context.node_guid;
>
> On an x86 Linux machine, the above 2 instances changed now complain
> about warning: cast to pointer from integer of different size
>
> so should those be cast to (osmv_query_req_t *) (long) or
> (osmv_query_req_t *) (uintptr_t) or something else ?
>
> -- Hal


We should return to my original patch submission.
remove the (long*) (long) and replace with (uintptr_t)

>> -        (osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context.
>> -                                            node_guid);
>> +        (osmv_query_req_t *) (uintptr_t) p_madw->context.ni_context.node_guid;

Sasha, can you take care of this?

stan.

>
>>
>>      /* provide the context of the original request in the result */
>>      query_res.query_context = p_query_req_copy->query_context;
>> @@ -433,7 +431,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t *
>>      p_bind,         } *p_query_req_copy = *p_query_req;
>>      p_madw->context.ni_context.node_guid =
>> -        (ib_net64_t) (long)p_query_req_copy;
>> +        (ib_net64_t) (uintptr_t)p_query_req_copy;
>>
>>      /* we can support async as well as sync calls */
>>      sync = ((p_query_req->flags&  OSM_SA_FLAGS_SYNC) ==
>> OSM_SA_FLAGS_SYNC);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)

[Sasha Khapyorsky]

On 15:18 Tue 07 Dec     , Smith, Stan wrote:
> 
> We should return to my original patch submission.
> remove the (long*) (long) and replace with (uintptr_t)
> 
> >> -        (osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context.
> >> -                                            node_guid);
> >> +        (osmv_query_req_t *) (uintptr_t) p_madw->context.ni_context.node_guid;
> 
> Sasha, can you take care of this?

Done. Thanks.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html