[ECOS] Re: CGY_SWAP16() seems broken to me.

[Grant Edwards <grante@visi.com>]

On 2007-09-22, Sergei Gavrikov <w3sg@SoftHome.net> wrote:
> On Sat, Sep 22, 2007 at 02:56:33AM +0000, Grant Edwards wrote:
>> unsigned u = CYG_SWAP16(0x1234);
>> 
>> What would a reasonable person (who hasn't looked at
>> hal_endian.h) expect u to be?
>> 
>> I certainly expected it to be 0x3412. 
>> 
>> But it's not.  It's 0x123412.  
>
> That is exists. I got same result
>
> #include <cyg/hal/hal_endian.h>
> #include <sys/param.h>
> #include <sys/endian.h>
>
> #include <stdio.h>
>
> int
> main (void)
> {
>     printf ("HAL swap16(0x1234) = 0x%x\n", CYG_SWAP16 (0x1234));
>     printf ("NET swap16(0x1234) = 0x%x\n", swap16 (0x1234));
>     return 0;
> }
>
> HAL swap16(0x1234) = 0x123412
> NET swap16(0x1234) = 0x3412
>
>> IMO, that's just plain broken.
>
> ... I don't know. What do eCos experts think about?
>
>> Here's the definition from hal_endian.h:
>> 
>> # define CYG_SWAP16(_x_)                                        \
>>     ({ cyg_uint16 _x = (_x_); ((_x << 8) | (_x >> 8)); })
>> 
>> Why isn't it this?
>> 
>> # define CYG_SWAP16(_x_)                                        \
>>     ({ cyg_uint16 _x = (_x_); (cyg_uint16)((_x << 8) | (_x >> 8)); })
>> 
>> Why would anybody expect a macro named CYG_SWAP16() to return a
>> "24-bit value" when passed a "16-bit value"?
>
> Usually, they use yet masking in same macros. Good byteorder examples
> are
>
> net/bsd_tcpip/current/include/sys/endian.h
> net/tcpip/current/include/sys/endian.h
>
> It seems, that eCos HAL's CYG_SWAP16() either assumes what it will be to
> live in "16-bit world" or it assumes that will be a developers's care.

I think both assumptions are invalid.

> I did find only 3 places where CYG_SWAP16() macro is used. For
> example, CYG_SWAP16() macro uses itself in the eCos drivers
> for 16-bit Ethernet controllers (cs8900a, ec555) "16-bit
> world". And I found that RedBoot's conf_endian_fixup()
> (fconfig.c) uses eCos HAL's CYG_SWAP16() "as is", because that
> was a "developer's care", there is there
>
>                 u16 = CYG_SWAP16(u16);
>
> but, not :-)
>
> 		unsigned u = CYG_SWAP16(0x1234);

Right.  The existing macro works correctly if the result is
being assigned to a 16-bit lvalue, but it doesn't work if it's
used without a typecase or mask in an arithmetic expression or
if it's being assied to a 32-bit lvalue.

My fix above should be completely transparent. if the value is
being assigned to a 16 bit lvalue, the compiler will generate
exactly the same code as when using the existing "broken"
macro. If the result from CYG_SWAP16() is being masked by the
user, the fixed macro will generate exactly the same code as
the existing macro.

The only argument against my fix would be to claim that
CYG_SWAP16 is expected to return 24 bits in wider contexts, and
returning only 16 bits is going to break code or surprise a
user.

-- 
Grant Edwards                   grante             Yow!  .. I must be a
                                  at               VETERINARIAN...
                               visi.com            


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss