* [ECOS] CGY_SWAP16() seems broken to me.
@ 2007-09-22 2:56 Grant Edwards
2007-09-22 12:22 ` Sergei Gavrikov
0 siblings, 1 reply; 5+ messages in thread
From: Grant Edwards @ 2007-09-22 2:56 UTC (permalink / raw)
To: ecos-discuss
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.
IMO, that's just plain broken.
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"?
--
Grant Edwards grante Yow! Ha ha Ha ha Ha ha
at Ha Ha Ha Ha -- When will I
visi.com EVER stop HAVING FUN?!!
--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [ECOS] CGY_SWAP16() seems broken to me.
2007-09-22 2:56 [ECOS] CGY_SWAP16() seems broken to me Grant Edwards
@ 2007-09-22 12:22 ` Sergei Gavrikov
2007-09-22 14:56 ` [ECOS] " Grant Edwards
0 siblings, 1 reply; 5+ messages in thread
From: Sergei Gavrikov @ 2007-09-22 12:22 UTC (permalink / raw)
To: Grant Edwards; +Cc: ecos-discuss
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
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);
In any case, thank you for your report, we will be to know about.
Sergei
--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ECOS] Re: CGY_SWAP16() seems broken to me.
2007-09-22 12:22 ` Sergei Gavrikov
@ 2007-09-22 14:56 ` Grant Edwards
2007-09-22 15:23 ` Grant Edwards
0 siblings, 1 reply; 5+ messages in thread
From: Grant Edwards @ 2007-09-22 14:56 UTC (permalink / raw)
To: ecos-discuss
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ECOS] Re: CGY_SWAP16() seems broken to me.
2007-09-22 14:56 ` [ECOS] " Grant Edwards
@ 2007-09-22 15:23 ` Grant Edwards
2007-09-22 17:17 ` Sergei Gavrikov
0 siblings, 1 reply; 5+ messages in thread
From: Grant Edwards @ 2007-09-22 15:23 UTC (permalink / raw)
To: ecos-discuss
On 2007-09-22, Grant Edwards <grante@visi.com> wrote:
> 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.
I've examined all uses of SWAP16 and the LE16/BE16 macros in my
source tree (which is a few months out of date), and it doesn't
look like any existing code depends on the fact that SWAP16()
produces 24 bits instead of 16.
--
Grant Edwards grante Yow! An INK-LING? Sure --
at TAKE one!! Did you BUY any
visi.com COMMUNIST UNIFORMS??
--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [ECOS] Re: CGY_SWAP16() seems broken to me.
2007-09-22 15:23 ` Grant Edwards
@ 2007-09-22 17:17 ` Sergei Gavrikov
0 siblings, 0 replies; 5+ messages in thread
From: Sergei Gavrikov @ 2007-09-22 17:17 UTC (permalink / raw)
To: Grant Edwards; +Cc: ecos-discuss
On Sat, Sep 22, 2007 at 03:20:51PM +0000, Grant Edwards wrote:
> On 2007-09-22, Grant Edwards <grante@visi.com> wrote:
>
> > 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.
>
> I've examined all uses of SWAP16 and the LE16/BE16 macros in my
> source tree (which is a few months out of date), and it doesn't
> look like any existing code depends on the fact that SWAP16()
> produces 24 bits instead of 16.
Today's score for CYG_SWAP16() is
CYG_SWAP16() : 3 sources
CYG_CPU_TO_LE16() : 4 sources
As you could see, ntohs(), htons() = swap16() in the little-endian world
(net/tcpip/current/include/sys/endian.h). There are tons of C lines are
using these net macros in the net stuff and at first, I just did think
about same strong rules for the HAL CYG_SWAP16() too, i.e. about type
casting (compiler care) + bit masking (CPU workaround).
I do see no criminal if you will provide a patch, which will set the
first barier - type casting. I vote for your patch. I did dislike that
0x123412.
Sergei
--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-09-22 17:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-09-22 2:56 [ECOS] CGY_SWAP16() seems broken to me Grant Edwards
2007-09-22 12:22 ` Sergei Gavrikov
2007-09-22 14:56 ` [ECOS] " Grant Edwards
2007-09-22 15:23 ` Grant Edwards
2007-09-22 17:17 ` Sergei Gavrikov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).