inet data type regression test fails

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?

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, Pennsylvania 19026

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, Pennsylvania 19026

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, Pennsylvania 19026

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, Pennsylvania 19026

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, Pennsylvania 19026

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

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.

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