www.digitalmars.com         C & C++   DMDScript  

D.gnu - GCC screws up pointer->ulong conversion on 32 bit systems

reply Johannes Pfau <nospam example.com> writes:
----------------------------------------------
#include <stdio.h>
#include <stdint.h>

void test(void* p)
{
    uint64_t pl = (uint64_t)p;
    uint64_t p2 = (uint64_t)(int)p;
    int tmp = (int)p;
    uint64_t p3 = (uint64_t)tmp;
    
    printf("%llx %llx %llx\n", pl, p2, p3);
}

void main()
{
    void* p = (void*)0xFFEECCAA;
    test(p);
}
-----------------------------------------------
Output is:
ffffffffffeeccaa ffffffffffeeccaa ffffffffffeeccaa

Expected:
ffeeccaa ffffffffffeeccaa ffffffffffeeccaa

Unfortunately this affects us as well (the bug is in
convert_to_integer). What do you think, is this a valid GCC bug which
should be filed?


I verified the C testcase on ARM and x86. I don't have a x86 gdc right
now, could you test whether the D testcase
http://dpaste.dzfl.pl/abb9a6971
also fails on x86?


The fix is simple:
in gcc/convert.c(convert_to_integer):

/* Convert to an unsigned integer of the correct width first, and
from there widen/truncate to the required type.  Some targets support
the coexistence of multiple valid pointer sizes, so fetch the one we
need from the type.  */
      expr = fold_build1 (CONVERT_EXPR,
			  lang_hooks.types.type_for_size
			    (TYPE_PRECISION (intype), 0),
			  expr);

should be

/* Convert to an unsigned integer of the correct width first, and
from there widen/truncate to the required type.  Some targets support
the coexistence of multiple valid pointer sizes, so fetch the one we
need from the type.  */
      expr = fold_build1 (CONVERT_EXPR,
			  lang_hooks.types.type_for_size
			    (TYPE_PRECISION (intype), 1),
			  expr);
Nov 09 2013
parent reply Iain Buclaw <ibuclaw ubuntu.com> writes:
On 9 November 2013 12:19, Johannes Pfau <nospam example.com> wrote:


 ----------------------------------------------
 #include <stdio.h>
 #include <stdint.h>

 void test(void* p)
 {
     uint64_t pl = (uint64_t)p;
     uint64_t p2 = (uint64_t)(int)p;
     int tmp = (int)p;
     uint64_t p3 = (uint64_t)tmp;

     printf("%llx %llx %llx\n", pl, p2, p3);
 }

 void main()
 {
     void* p = (void*)0xFFEECCAA;
     test(p);
 }
 -----------------------------------------------
 Output is:
 ffffffffffeeccaa ffffffffffeeccaa ffffffffffeeccaa

 Expected:
 ffeeccaa ffffffffffeeccaa ffffffffffeeccaa

 Unfortunately this affects us as well (the bug is in
 convert_to_integer). What do you think, is this a valid GCC bug which
 should be filed?


 I verified the C testcase on ARM and x86. I don't have a x86 gdc right
 now, could you test whether the D testcase
 http://dpaste.dzfl.pl/abb9a6971
 also fails on x86?


 The fix is simple:
 in gcc/convert.c(convert_to_integer):

 /* Convert to an unsigned integer of the correct width first, and
 from there widen/truncate to the required type.  Some targets support
 the coexistence of multiple valid pointer sizes, so fetch the one we
 need from the type.  */
       expr = fold_build1 (CONVERT_EXPR,
                           lang_hooks.types.type_for_size
                             (TYPE_PRECISION (intype), 0),
                           expr);


That code does seem to contradict what the comment is saying.  I'll raise
it in #gcc and send a patch  (unless you want to do it).


-- 
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
Nov 09 2013
next sibling parent reply Johannes Pfau <nospam example.com> writes:
Am Sat, 9 Nov 2013 17:01:43 +0000
schrieb Iain Buclaw <ibuclaw ubuntu.com>:


 On 9 November 2013 12:19, Johannes Pfau <nospam example.com> wrote:

 The fix is simple:
 in gcc/convert.c(convert_to_integer):

 /* Convert to an unsigned integer of the correct width first, and
 from there widen/truncate to the required type.  Some targets
 support the coexistence of multiple valid pointer sizes, so fetch
 the one we need from the type.  */
       expr = fold_build1 (CONVERT_EXPR,
                           lang_hooks.types.type_for_size
                             (TYPE_PRECISION (intype), 0),
                           expr);


 That code does seem to contradict what the comment is saying.  I'll
 raise it in #gcc and send a patch  (unless you want to do it).
 
 


It indeed seems like it's only a small oversight. It'd be
great if you could report it.
Nov 09 2013
parent reply "Iain Buclaw" <ibuclaw ubuntu.com> writes:
On Saturday, 9 November 2013 at 17:53:41 UTC, Johannes Pfau wrote:

 Am Sat, 9 Nov 2013 17:01:43 +0000
 schrieb Iain Buclaw <ibuclaw ubuntu.com>:


 On 9 November 2013 12:19, Johannes Pfau <nospam example.com> 
 wrote:

 The fix is simple:
 in gcc/convert.c(convert_to_integer):

 /* Convert to an unsigned integer of the correct width 
 first, and
 from there widen/truncate to the required type.  Some targets
 support the coexistence of multiple valid pointer sizes, so 
 fetch
 the one we need from the type.  */
       expr = fold_build1 (CONVERT_EXPR,
                           lang_hooks.types.type_for_size
                             (TYPE_PRECISION (intype), 0),
                           expr);


 That code does seem to contradict what the comment is saying.  
 I'll
 raise it in #gcc and send a patch  (unless you want to do it).
 
 


 It indeed seems like it's only a small oversight. It'd be
 great if you could report it.


Apparently it's working as per C semantics, which means the 
behavior is undefined.  I've mentioned that it makes no sense 
that documentation/code don't match comments. But all I've heard 
back is that you shouldn't use convert_to_integer in the front 
end (except maybe through convert() - which I am 90% certain is 
the case with us), and to implement our own semantics if we say 
that pointer to integer conversion is clearly defined in D.

Regards
Iain.
Nov 24 2013
parent reply Johannes Pfau <nospam example.com> writes:
Am Sun, 24 Nov 2013 12:50:43 +0100
schrieb "Iain Buclaw" <ibuclaw ubuntu.com>:


 
 Apparently it's working as per C semantics, which means the 
 behavior is undefined.  I've mentioned that it makes no sense 
 that documentation/code don't match comments. But all I've heard 
 back is that you shouldn't use convert_to_integer in the front 
 end (except maybe through convert() - which I am 90% certain is 
 the case with us), and to implement our own semantics if we say 
 that pointer to integer conversion is clearly defined in D.
 
 Regards
 Iain.


We actually use convert_to_integer directly, but anyway, here's the
fix:
https://github.com/jpf91/GDC/commit/c3e19f8e79c9b47bff29fd9a5be8de1c69af3ee5

Fixing it in the frontend has the benefit that we don't have to patch
gcc so maybe it's even better this way.

( I guess merging the ARM branch should probably wait till the 2.064
merge is finished, right? )
Dec 01 2013
parent Iain Buclaw <ibuclaw gdcproject.org> writes:
On 1 December 2013 13:15, Johannes Pfau <nospam example.com> wrote:

 Am Sun, 24 Nov 2013 12:50:43 +0100
 schrieb "Iain Buclaw" <ibuclaw ubuntu.com>:


 Apparently it's working as per C semantics, which means the
 behavior is undefined.  I've mentioned that it makes no sense
 that documentation/code don't match comments. But all I've heard
 back is that you shouldn't use convert_to_integer in the front
 end (except maybe through convert() - which I am 90% certain is
 the case with us), and to implement our own semantics if we say
 that pointer to integer conversion is clearly defined in D.

 Regards
 Iain.


 We actually use convert_to_integer directly


d_convert_basic is called as the default for convert() so the
statement is pretty much correct.  ;-)

Yeah, we just need to apply our own semantics before falling back to
convert_to_integer.
Dec 01 2013
prev sibling parent reply "Kagamin" <spam here.lot> writes:
C first sign-extends then converts to unsigned. It's just weird 
it treats pointer as signed. Was there an option for it?
Nov 18 2013
parent Iain Buclaw <ibuclaw ubuntu.com> writes:
On Nov 19, 2013 6:35 AM, "Kagamin" <spam here.lot> wrote:

 C first sign-extends then converts to unsigned. It's just weird it treats


pointer as signed. Was there an option for it?

No, it's a bug. The comment in the code says that it unsign-extends (if
that is even a proper word a to describe it) first before converting to
pointer.

Regards
-- 
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
Nov 18 2013