* [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).