From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9913 invoked by alias); 22 Sep 2007 14:56:42 -0000 Received: (qmail 9901 invoked by uid 22791); 22 Sep 2007 14:56:41 -0000 X-Spam-Check-By: sourceware.org Received: from main.gmane.org (HELO ciao.gmane.org) (80.91.229.2) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 22 Sep 2007 14:56:38 +0000 Received: from list by ciao.gmane.org with local (Exim 4.43) id 1IZ6OW-0008Va-Ec for ecos-discuss@sources.redhat.com; Sat, 22 Sep 2007 14:55:44 +0000 Received: from grante.dsl.visi.com ([208.42.141.248]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Sat, 22 Sep 2007 14:55:44 +0000 Received: from grante by grante.dsl.visi.com with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Sat, 22 Sep 2007 14:55:44 +0000 To: ecos-discuss@sources.redhat.com From: Grant Edwards Date: Sat, 22 Sep 2007 14:56:00 -0000 Message-ID: References: <20070922122026.GA5875@ubuntu> User-Agent: slrn/0.9.8.1 (Linux) X-IsSubscribed: yes Mailing-List: contact ecos-discuss-help@ecos.sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: ecos-discuss-owner@ecos.sourceware.org Subject: [ECOS] Re: CGY_SWAP16() seems broken to me. X-SW-Source: 2007-09/txt/msg00123.txt.bz2 On 2007-09-22, Sergei Gavrikov 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 > #include > #include > > #include > > 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