Обсуждение: inet data type regression test fails
Hi all, The inet regression test has been failed on my LinuxPPC. While investigating the reason, I found a code that doesn't work on LinuxPPC. From network_broadcast() in utils/adt/network.c: int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); Here ip_bits() returns from (unsigned char)0 to 32. My question is: what is the correct result of (0xffffffff >> ip_bits())? 1. 0x0 2. 0xffffffff (actually does nothing) LinuxPPC is 1. FreeBSD and Solaris are 2. network_broadcast() seems to expect 2. My guess is shifting over 32bit against a 32bit integer is not permitted and the result is platform depedent. If this would true, it could be said that network_broadcast() has a portabilty problem. Comments? --- Tatsuo Ishii
> The inet regression test has been failed on my LinuxPPC. While > investigating the reason, I found a code that doesn't work on > LinuxPPC. From network_broadcast() in utils/adt/network.c: > > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); > > Here ip_bits() returns from (unsigned char)0 to 32. My question is: > what is the correct result of (0xffffffff >> ip_bits())? I should have said that: what is the correct result of (0xffffffff >> ip_bits()) if ip_bits() == 32? > 1. 0x0 > 2. 0xffffffff (actually does nothing) > > LinuxPPC is 1. FreeBSD and Solaris are 2. network_broadcast() seems to > expect 2. My guess is shifting over 32bit against a 32bit integer is > not permitted and the result is platform depedent. If this would true, > it could be said that network_broadcast() has a portabilty > problem. Comments? > --- > Tatsuo Ishii >
> what is the correct result of
> (0xffffffff >> ip_bits()) if ip_bits() == 32?
> > 1. 0x0
> > 2. 0xffffffff (actually does nothing)
In both cases, it does something. I haven't looked it up, but I suspect
that this is an implementation-defined result, since you are seeing the
results of right-shifting the sign bit *or* the high bit downward. On
some systems it does not propagate, and on others it does.
Have you tried coercing 0xffffffff to be a signed char? The better
solution is probably to mask the result before comparing, or handling
shifts greater than 31 as a special case. For example,
/* It's an IP V4 address: */ int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip)));
becomes
/* It's an IP V4 address: */ int addr = htonl(ntohl(ip_v4addr(ip)); if (ip_bits(ip) < sizeof(addr)) addr |=
(0xffffffff>> ip_bits(ip)));
or something like that...
- Tom
>> what is the correct result of
>> (0xffffffff >> ip_bits()) if ip_bits() == 32?
>> > 1. 0x0
>> > 2. 0xffffffff (actually does nothing)
>
>In both cases, it does something. I haven't looked it up, but I suspect
>that this is an implementation-defined result, since you are seeing the
>results of right-shifting the sign bit *or* the high bit downward. On
>some systems it does not propagate, and on others it does.
>
>Have you tried coercing 0xffffffff to be a signed char? The better
>solution is probably to mask the result before comparing, or handling
>shifts greater than 31 as a special case. For example,
>
> /* It's an IP V4 address: */
> int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip)));
>
>becomes
>
> /* It's an IP V4 address: */
> int addr = htonl(ntohl(ip_v4addr(ip));
> if (ip_bits(ip) < sizeof(addr))
> addr |= (0xffffffff >> ip_bits(ip)));
>
>or something like that...
Thank you for the advice. I concluded that current inet code has a
portability problem. Included patches should be applied to both
current and 6.4 tree. I have tested on LinuxPPC, FreeBSD and Solaris
2.6. Now the inet regression tests on these platforms are all happy.
---
Tatsuo Ishii
------------------------------------------------------------------------
*** pgsql/src/backend/utils/adt/network.c.orig Fri Jan 1 13:17:13 1999
--- pgsql/src/backend/utils/adt/network.c Tue Feb 23 21:31:41 1999
***************
*** 356,362 **** if (ip_family(ip) == AF_INET) { /* It's an IP V4 address: */
! int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); if (inet_net_ntop(AF_INET,
&addr,32, tmp, sizeof(tmp)) == NULL) {
--- 356,367 ---- if (ip_family(ip) == AF_INET) { /* It's an IP V4 address: */
! int addr;
! unsigned long mask = 0xffffffff;
!
! if (ip_bits(ip) < 32)
! mask >>= ip_bits(ip);
! addr = htonl(ntohl(ip_v4addr(ip)) | mask); if (inet_net_ntop(AF_INET, &addr, 32, tmp, sizeof(tmp))
==NULL) {
Applied.
> >> what is the correct result of
> >> (0xffffffff >> ip_bits()) if ip_bits() == 32?
> >> > 1. 0x0
> >> > 2. 0xffffffff (actually does nothing)
> >
> >In both cases, it does something. I haven't looked it up, but I suspect
> >that this is an implementation-defined result, since you are seeing the
> >results of right-shifting the sign bit *or* the high bit downward. On
> >some systems it does not propagate, and on others it does.
> >
> >Have you tried coercing 0xffffffff to be a signed char? The better
> >solution is probably to mask the result before comparing, or handling
> >shifts greater than 31 as a special case. For example,
> >
> > /* It's an IP V4 address: */
> > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip)));
> >
> >becomes
> >
> > /* It's an IP V4 address: */
> > int addr = htonl(ntohl(ip_v4addr(ip));
> > if (ip_bits(ip) < sizeof(addr))
> > addr |= (0xffffffff >> ip_bits(ip)));
> >
> >or something like that...
>
> Thank you for the advice. I concluded that current inet code has a
> portability problem. Included patches should be applied to both
> current and 6.4 tree. I have tested on LinuxPPC, FreeBSD and Solaris
> 2.6. Now the inet regression tests on these platforms are all happy.
> ---
> Tatsuo Ishii
> ------------------------------------------------------------------------
> *** pgsql/src/backend/utils/adt/network.c.orig Fri Jan 1 13:17:13 1999
> --- pgsql/src/backend/utils/adt/network.c Tue Feb 23 21:31:41 1999
> ***************
> *** 356,362 ****
> if (ip_family(ip) == AF_INET)
> {
> /* It's an IP V4 address: */
> ! int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip)));
>
> if (inet_net_ntop(AF_INET, &addr, 32, tmp, sizeof(tmp)) == NULL)
> {
> --- 356,367 ----
> if (ip_family(ip) == AF_INET)
> {
> /* It's an IP V4 address: */
> ! int addr;
> ! unsigned long mask = 0xffffffff;
> !
> ! if (ip_bits(ip) < 32)
> ! mask >>= ip_bits(ip);
> ! addr = htonl(ntohl(ip_v4addr(ip)) | mask);
>
> if (inet_net_ntop(AF_INET, &addr, 32, tmp, sizeof(tmp)) == NULL)
> {
>
>
-- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
> Hi all, > > The inet regression test has been failed on my LinuxPPC. While > investigating the reason, I found a code that doesn't work on > LinuxPPC. From network_broadcast() in utils/adt/network.c: > > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); > > Here ip_bits() returns from (unsigned char)0 to 32. My question is: > what is the correct result of (0xffffffff >> ip_bits())? > > 1. 0x0 > 2. 0xffffffff (actually does nothing) > > LinuxPPC is 1. FreeBSD and Solaris are 2. network_broadcast() seems to > expect 2. My guess is shifting over 32bit against a 32bit integer is > not permitted and the result is platform depedent. If this would true, > it could be said that network_broadcast() has a portabilty > problem. Comments? If 0xffffff is unsigned, it should allow the right shift. When you say 1 or 2, how do you get those values? -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> > The inet regression test has been failed on my LinuxPPC. While
> > investigating the reason, I found a code that doesn't work on
> > LinuxPPC. From network_broadcast() in utils/adt/network.c:
> >
> > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip)));
> >
> > Here ip_bits() returns from (unsigned char)0 to 32. My question is:
> > what is the correct result of (0xffffffff >> ip_bits())?
> >
> > 1. 0x0
> > 2. 0xffffffff (actually does nothing)
> >
> > LinuxPPC is 1. FreeBSD and Solaris are 2. network_broadcast() seems to
> > expect 2. My guess is shifting over 32bit against a 32bit integer is
> > not permitted and the result is platform depedent. If this would true,
> > it could be said that network_broadcast() has a portabilty
> > problem. Comments?
>
> If 0xffffff is unsigned, it should allow the right shift.
No. it does not depend on if 0xffffffff is signed or not. Suppose a
is signed and b is unsigned. In "a >> b", before doing an actual
shifting operation, a is "upgraded" to unsigned by the compiler.
>When you say
> 1 or 2, how do you get those values?
You could observe the "32 bit shift efect" I mentioned in the previous
mail by running following small program.
main()
{ unsigned char c; for (c = 0;c <=32;c++) { printf("shift: %d result: 0x%08x\n",c,0xffffffff >> c); }
}
---
Tatsuo Ishii
Can someone comment on this one? Is it fixed? > Hi all, > > The inet regression test has been failed on my LinuxPPC. While > investigating the reason, I found a code that doesn't work on > LinuxPPC. From network_broadcast() in utils/adt/network.c: > > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); > > Here ip_bits() returns from (unsigned char)0 to 32. My question is: > what is the correct result of (0xffffffff >> ip_bits())? > > 1. 0x0 > 2. 0xffffffff (actually does nothing) > > LinuxPPC is 1. FreeBSD and Solaris are 2. network_broadcast() seems to > expect 2. My guess is shifting over 32bit against a 32bit integer is > not permitted and the result is platform depedent. If this would true, > it could be said that network_broadcast() has a portabilty > problem. Comments? > --- > Tatsuo Ishii > > -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> Can someone comment on this one? Is it fixed?
> > The inet regression test has been failed on my LinuxPPC.
> > My guess is shifting over 32bit against a 32bit integer is
> > not permitted and the result is platform depedent.
Yes, it is fixed. You applied the patches :)
backend/utils/adt/network.c:
revision 1.6
date: 1999/02/24 03:17:05; author: momjian; state: Exp; lines: +7
-2
Thank you for the advice. I concluded that current inet code has a
portability problem. Included patches should be applied to both
current and 6.4 tree. I have tested on LinuxPPC, FreeBSD and Solaris
2.6. Now the inet regression tests on these platforms are all happy.
- Thomas
--
Thomas Lockhart lockhart@alumni.caltech.edu
South Pasadena, California
On Sun, 9 May 1999, Bruce Momjian wrote: > > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); There needs to be a UL on the end of that constant. Otherwise it depends on whether or not the compiler chooses to make it signed or unsigned. Not only that, but shifting by >=32 is undefined... Intel chipsets will go mod 32 and change 32 to 0. Taral
> On Sun, 9 May 1999, Bruce Momjian wrote: > > > > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); > > There needs to be a UL on the end of that constant. Otherwise it depends > on whether or not the compiler chooses to make it signed or unsigned. Not > only that, but shifting by >=32 is undefined... Intel chipsets will go mod > 32 and change 32 to 0. > Anyone want to supply a patch? -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> > > > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip)));
> >
> > There needs to be a UL on the end of that constant. Otherwise it depends
> > on whether or not the compiler chooses to make it signed or unsigned. Not
> > only that, but shifting by >=32 is undefined... Intel chipsets will go mod
> > 32 and change 32 to 0.
> >
>
> Anyone want to supply a patch?
This has been already fixed. Now it looks like:
unsigned long mask = 0xffffffff;
if (ip_bits(ip) < 32) mask >>= ip_bits(ip); addr = htonl(ntohl(ip_v4addr(ip)) | mask);
---
Tatsuo Ishii
> > > > > int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); > > > > > > There needs to be a UL on the end of that constant. Otherwise it depends > > > on whether or not the compiler chooses to make it signed or unsigned. Not > > > only that, but shifting by >=32 is undefined... Intel chipsets will go mod > > > 32 and change 32 to 0. > > > > > > > Anyone want to supply a patch? > > This has been already fixed. Now it looks like: > > unsigned long mask = 0xffffffff; > > if (ip_bits(ip) < 32) > mask >>= ip_bits(ip); > addr = htonl(ntohl(ip_v4addr(ip)) | mask); Oh. Very nice. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
On Tue, 11 May 1999, Tatsuo Ishii wrote: > unsigned long mask = 0xffffffff; > > if (ip_bits(ip) < 32) > mask >>= ip_bits(ip); > addr = htonl(ntohl(ip_v4addr(ip)) | mask); That's wrong too. There needs to be: elsemask = 0; Taral
>On Tue, 11 May 1999, Tatsuo Ishii wrote: > >> unsigned long mask = 0xffffffff; >> >> if (ip_bits(ip) < 32) >> mask >>= ip_bits(ip); >> addr = htonl(ntohl(ip_v4addr(ip)) | mask); > >That's wrong too. There needs to be: > >else > mask = 0; > >Taral No. it is expected addr == 0xffffffff if ip_bits() returns >= 32. This is how the function (network_broadcast()) is made. See included posting. >From: Tatsuo Ishii <t-ishii@sra.co.jp> >To: hackers@postgreSQL.org >Subject: [HACKERS] inet data type regression test fails >Date: Mon, 22 Feb 1999 11:54:39 +0900 > >Hi all, > >The inet regression test has been failed on my LinuxPPC. While >investigating the reason, I found a code that doesn't work on >LinuxPPC. From network_broadcast() in utils/adt/network.c: > >int addr = htonl(ntohl(ip_v4addr(ip)) | (0xffffffff >> ip_bits(ip))); > >Here ip_bits() returns from (unsigned char)0 to 32. My question is: >what is the correct result of (0xffffffff >> ip_bits())? > >1. 0x0 >2. 0xffffffff (actually does nothing) > >LinuxPPC is 1. FreeBSD and Solaris are 2. network_broadcast() seems to >expect 2. My guess is shifting over 32bit against a 32bit integer is >not permitted and the result is platform depedent. If this would true, >it could be said that network_broadcast() has a portabilty >problem. Comments? >--- >Tatsuo Ishii > >
On Tue, 11 May 1999, Tatsuo Ishii wrote: > >On Tue, 11 May 1999, Tatsuo Ishii wrote: > > > >> unsigned long mask = 0xffffffff; > >> > >> if (ip_bits(ip) < 32) > >> mask >>= ip_bits(ip); > >> addr = htonl(ntohl(ip_v4addr(ip)) | mask); > No. it is expected addr == 0xffffffff if ip_bits() returns >= 32. This > is how the function (network_broadcast()) is made. > See included posting. ip_bits(ip) = 0 => mask = 0xffffffff ip_bits(ip) = 31 => mask = 1 ip_bits(ip) = 32 => mask = 0xffffffff You sure? Taral
>> >> unsigned long mask = 0xffffffff; >> >> >> >> if (ip_bits(ip) < 32) >> >> mask >>= ip_bits(ip); >> >> addr = htonl(ntohl(ip_v4addr(ip)) | mask); > >> No. it is expected addr == 0xffffffff if ip_bits() returns >= 32. This >> is how the function (network_broadcast()) is made. >> See included posting. > >ip_bits(ip) = 0 => mask = 0xffffffff >ip_bits(ip) = 31 => mask = 1 >ip_bits(ip) = 32 => mask = 0xffffffff > >You sure? Yes. That's exactly what I expected. --- Tatsuo Ishii