Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1668188

Re: endian bitshift defects [ was: staging: fusb302: don't bitshift __le16 type ]

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

On Fri, 16 Jun 2017, Joe Perches wrote:

> On Sat, 2017-06-17 at 07:23 +0200, Julia Lawall wrote:
> > On Fri, 16 Jun 2017, Joe Perches wrote:
> > > On Fri, 2017-06-16 at 19:45 +0200, Frans Klaver wrote:
> > > > The header field in struct pd_message is declared as an __le16 type. The
> > > > data in the message is supposed to be little endian. This means we don't
> > > > have to go and shift the individual bytes into position when we're
> > > > filling the buffer, we can just copy the contents right away. As an
> > > > added benefit we don't get fishy results on big endian systems anymore.
> > >
> > > Thanks for pointing this out.
> > >
> > > There are several instances of this class of error.
> > >
> > > Here's a cocci script to find them.
> > >
> > > This is best used with cocci's --all-includes option like:
> > >
> > > $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci .
> > > [ many defects...]
>
> Probably would have been better as [ many possible defects... ]

OK

> > > $ cat lebe_bitshifts.cocci
> > > @@
> > > typedef __le16, __le32, __le64,  __be16, __be32, __be64;
> > > { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> > > expression b;
> > > @@
> > >
> > > *	a << b
>
> [etc...]
>
> > Is this always a problem?
>
> No, not always.
>
> If the CPU is the equivalent endian, the bitshift is fine.
> It can't be known if the code is only compiled on a
> single cpu type.  It is rather odd though to use endian
> notation if the code is compiled for a single cpu type.

Is there some way to know from the code if it is compiled for a single cou
type?

> > Would it be useful to add this to the scripts
> > in the kernel?
>
> Maybe.

If there are a lot of false positives, it could be a nuisance...

> btw: is there a way for the operators to be surrounded by
> some \( \| \) or some other bracket style so it could
> be written with a single test?
>
> Something like:
>
> @@
> typedef __le16, __le32, __le64,  __be16, __be32, __be64;
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> *	a [<<|<<=|>>|>>=] b

Partly.  You can define

binary operator bop = {<<,>>};

or

assignment operator aop = {<<=,>>=};

to make two rules instead of four.

julia

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] staging: fusb302: don't bitshift __le16 type Frans Klaver <fransklaver@gmail.com> - 2017-06-16 19:40 +0200
  Re: [PATCH] staging: fusb302: don't bitshift __le16 type Guenter Roeck <linux@roeck-us.net> - 2017-06-16 20:20 +0200
  endian bitshift defects [ was: staging: fusb302: don't bitshift  __le16 type ] Joe Perches <joe@perches.com> - 2017-06-17 00:50 +0200
    Re: endian bitshift defects [ was: staging: fusb302: don't bitshift  __le16 type ] Julia Lawall <julia.lawall@lip6.fr> - 2017-06-17 07:30 +0200
      Re: endian bitshift defects [ was: staging: fusb302: don't bitshift  __le16 type ] Joe Perches <joe@perches.com> - 2017-06-17 08:00 +0200
        Re: endian bitshift defects [ was: staging: fusb302: don't bitshift  __le16 type ] Julia Lawall <julia.lawall@lip6.fr> - 2017-06-17 08:10 +0200
          Re: endian bitshift defects [ was: staging: fusb302: don't bitshift  __le16 type ] Joe Perches <joe@perches.com> - 2017-06-17 08:30 +0200
            Re: endian bitshift defects [ was: staging: fusb302: don't bitshift  __le16 type ] Julia Lawall <julia.lawall@lip6.fr> - 2017-06-17 08:30 +0200

csiph-web