Thread overview
GCC screws up pointer->ulong conversion on 32 bit systems
Nov 09, 2013
Johannes Pfau
Nov 09, 2013
Iain Buclaw
Nov 09, 2013
Johannes Pfau
Nov 24, 2013
Iain Buclaw
Dec 01, 2013
Johannes Pfau
Dec 01, 2013
Iain Buclaw
Nov 19, 2013
Kagamin
Nov 19, 2013
Iain Buclaw
November 09, 2013
----------------------------------------------
#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);
November 09, 2013
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';


November 09, 2013
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.
November 19, 2013
C first sign-extends then converts to unsigned. It's just weird it treats pointer as signed. Was there an option for it?
November 19, 2013
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';


November 24, 2013
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.
December 01, 2013
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? )
December 01, 2013
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.