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

[Julia Lawall]

On Mon, 26 Jun 2017, Frans Klaver wrote:

> On Fri, Jun 23, 2017 at 07:37:28PM -0400, Julia Lawall wrote:
> >
> >
> > On Sat, 24 Jun 2017, Frans Klaver wrote:
> >
> > > Hm. For some reason the great mail filtering scheme decided to push
> > > this past my inbox :-/
> > >
> > > On Sat, Jun 17, 2017 at 12:44 AM, 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.
> > >
> > > There are other smells around __(le|be) types that show up in staging
> > > that might be worth checking in the rest of the kernel as well. e.g.
> > > converting to cpu and storing it back into itself (possibly with its
> > > bytes reversed), direct assignments without conversion and what else
> > > you might have. sparse obviously already flags anything fishy going on
> > > with these types, but cannot distinguish between the classes of
> > > errors. I'll need to acquaint myself with spatch a bit more to be able
> > > to track that down.
> >
> > If you have concrete code examples, even fake ones, illustrating a class
> > of problem, then that would be great.
>
> Alright, I'll describe two fairly simple cases for starters.
>
> One class of issue that I have on top of mind is simply
>
>   __le16 val;
>
>   val = le16_to_cpu(val);
>
> The problem there obviously being that val is supposed to be guaranteed
> little endian. Sparse will throw a warning at this. It may also appear
> as (or be 'fixed' as)
>
>   __le16 val;
>
>   le16_to_cpus(val);
>
> Sparse doesn't flag this second version as an issue, while it causes the
> same problem. It is especially a potential problem when the value is
> stored in driver data.
>
> Another smell that is prevalent, at least in staging, is
>
>   u16 in;
>   u16 out;
>
>   out = cpu_to_le16(in);
>
> or in one instance (drivers/staging/fbtft/fbtft-io.c) I saw
>
>   u64 tmp;
>
>   *(u64*)dst = cpu_to_be64(tmp);
>
> Now these aren't necessarily problematic. Usually this typo of code is
> preparing the data to be sent out in a specific byte ordering, but again
> issues may arise if this specifically ordered data is stored somewhere.
>
> I'll leave it at that for now.

OK, thanks!

julia

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

[Julia Lawall]

On Mon, 26 Jun 2017, Frans Klaver wrote:

> On Fri, Jun 23, 2017 at 07:37:28PM -0400, Julia Lawall wrote:
> >
> >
> > On Sat, 24 Jun 2017, Frans Klaver wrote:
> >
> > > Hm. For some reason the great mail filtering scheme decided to push
> > > this past my inbox :-/
> > >
> > > On Sat, Jun 17, 2017 at 12:44 AM, 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.
> > >
> > > There are other smells around __(le|be) types that show up in staging
> > > that might be worth checking in the rest of the kernel as well. e.g.
> > > converting to cpu and storing it back into itself (possibly with its
> > > bytes reversed), direct assignments without conversion and what else
> > > you might have. sparse obviously already flags anything fishy going on
> > > with these types, but cannot distinguish between the classes of
> > > errors. I'll need to acquaint myself with spatch a bit more to be able
> > > to track that down.
> >
> > If you have concrete code examples, even fake ones, illustrating a class
> > of problem, then that would be great.
>
> Alright, I'll describe two fairly simple cases for starters.
>
> One class of issue that I have on top of mind is simply
>
>   __le16 val;
>
>   val = le16_to_cpu(val);
>
> The problem there obviously being that val is supposed to be guaranteed
> little endian. Sparse will throw a warning at this. It may also appear
> as (or be 'fixed' as)
>
>   __le16 val;
>
>   le16_to_cpus(val);
>
> Sparse doesn't flag this second version as an issue, while it causes the
> same problem. It is especially a potential problem when the value is
> stored in driver data.
>
> Another smell that is prevalent, at least in staging, is
>
>   u16 in;
>   u16 out;
>
>   out = cpu_to_le16(in);
>
> or in one instance (drivers/staging/fbtft/fbtft-io.c) I saw
>
>   u64 tmp;
>
>   *(u64*)dst = cpu_to_be64(tmp);
>
> Now these aren't necessarily problematic. Usually this typo of code is
> preparing the data to be sent out in a specific byte ordering, but again
> issues may arise if this specifically ordered data is stored somewhere.
>
> I'll leave it at that for now.

OK, thanks!

julia

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

[Frans Klaver]

On Fri, Jun 23, 2017 at 07:37:28PM -0400, Julia Lawall wrote:
> 
> 
> On Sat, 24 Jun 2017, Frans Klaver wrote:
> 
> > Hm. For some reason the great mail filtering scheme decided to push
> > this past my inbox :-/
> >
> > On Sat, Jun 17, 2017 at 12:44 AM, 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.
> >
> > There are other smells around __(le|be) types that show up in staging
> > that might be worth checking in the rest of the kernel as well. e.g.
> > converting to cpu and storing it back into itself (possibly with its
> > bytes reversed), direct assignments without conversion and what else
> > you might have. sparse obviously already flags anything fishy going on
> > with these types, but cannot distinguish between the classes of
> > errors. I'll need to acquaint myself with spatch a bit more to be able
> > to track that down.
> 
> If you have concrete code examples, even fake ones, illustrating a class
> of problem, then that would be great.

Alright, I'll describe two fairly simple cases for starters.

One class of issue that I have on top of mind is simply

__le16 val;

val = le16_to_cpu(val);

The problem there obviously being that val is supposed to be guaranteed
little endian. Sparse will throw a warning at this. It may also appear
as (or be 'fixed' as)

__le16 val;

le16_to_cpus(val);

Sparse doesn't flag this second version as an issue, while it causes the
same problem. It is especially a potential problem when the value is
stored in driver data.

Another smell that is prevalent, at least in staging, is

u16 in;
u16 out;

out = cpu_to_le16(in);

or in one instance (drivers/staging/fbtft/fbtft-io.c) I saw

u64 tmp;

*(u64*)dst = cpu_to_be64(tmp);

Now these aren't necessarily problematic. Usually this typo of code is
preparing the data to be sent out in a specific byte ordering, but again
issues may arise if this specifically ordered data is stored somewhere.

I'll leave it at that for now. 

Frans

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

[Frans Klaver]

On Fri, Jun 23, 2017 at 07:37:28PM -0400, Julia Lawall wrote:
> 
> 
> On Sat, 24 Jun 2017, Frans Klaver wrote:
> 
> > Hm. For some reason the great mail filtering scheme decided to push
> > this past my inbox :-/
> >
> > On Sat, Jun 17, 2017 at 12:44 AM, 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.
> >
> > There are other smells around __(le|be) types that show up in staging
> > that might be worth checking in the rest of the kernel as well. e.g.
> > converting to cpu and storing it back into itself (possibly with its
> > bytes reversed), direct assignments without conversion and what else
> > you might have. sparse obviously already flags anything fishy going on
> > with these types, but cannot distinguish between the classes of
> > errors. I'll need to acquaint myself with spatch a bit more to be able
> > to track that down.
> 
> If you have concrete code examples, even fake ones, illustrating a class
> of problem, then that would be great.

Alright, I'll describe two fairly simple cases for starters.

One class of issue that I have on top of mind is simply

__le16 val;

val = le16_to_cpu(val);

The problem there obviously being that val is supposed to be guaranteed
little endian. Sparse will throw a warning at this. It may also appear
as (or be 'fixed' as)

__le16 val;

le16_to_cpus(val);

Sparse doesn't flag this second version as an issue, while it causes the
same problem. It is especially a potential problem when the value is
stored in driver data.

Another smell that is prevalent, at least in staging, is

u16 in;
u16 out;

out = cpu_to_le16(in);

or in one instance (drivers/staging/fbtft/fbtft-io.c) I saw

u64 tmp;

*(u64*)dst = cpu_to_be64(tmp);

Now these aren't necessarily problematic. Usually this typo of code is
preparing the data to be sent out in a specific byte ordering, but again
issues may arise if this specifically ordered data is stored somewhere.

I'll leave it at that for now. 

Frans

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

[Julia Lawall]

On Mon, 26 Jun 2017, Frans Klaver wrote:

> On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall  wrote:
> >
> >
> > On Sat, 24 Jun 2017, Frans Klaver wrote:
> >
> >> Hm. For some reason the great mail filtering scheme decided to push
> >> this past my inbox :-/
> >>
> >> On Sat, Jun 17, 2017 at 12:44 AM, 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.
> >>
> >> There are other smells around __(le|be) types that show up in staging
> >> that might be worth checking in the rest of the kernel as well. e.g.
> >> converting to cpu and storing it back into itself (possibly with its
> >> bytes reversed), direct assignments without conversion and what else
> >> you might have. sparse obviously already flags anything fishy going on
> >> with these types, but cannot distinguish between the classes of
> >> errors. I'll need to acquaint myself with spatch a bit more to be able
> >> to track that down.
> >
> > If you have concrete code examples, even fake ones, illustrating a class
> > of problem, then that would be great.
>
> I'll see if I can produce some somewhere this week.

Thanks.

julia

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

[Julia Lawall]

On Mon, 26 Jun 2017, Frans Klaver wrote:

> On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall  wrote:
> >
> >
> > On Sat, 24 Jun 2017, Frans Klaver wrote:
> >
> >> Hm. For some reason the great mail filtering scheme decided to push
> >> this past my inbox :-/
> >>
> >> On Sat, Jun 17, 2017 at 12:44 AM, 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.
> >>
> >> There are other smells around __(le|be) types that show up in staging
> >> that might be worth checking in the rest of the kernel as well. e.g.
> >> converting to cpu and storing it back into itself (possibly with its
> >> bytes reversed), direct assignments without conversion and what else
> >> you might have. sparse obviously already flags anything fishy going on
> >> with these types, but cannot distinguish between the classes of
> >> errors. I'll need to acquaint myself with spatch a bit more to be able
> >> to track that down.
> >
> > If you have concrete code examples, even fake ones, illustrating a class
> > of problem, then that would be great.
>
> I'll see if I can produce some somewhere this week.

Thanks.

julia

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

[Frans Klaver]

On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall  wrote:
>
>
> On Sat, 24 Jun 2017, Frans Klaver wrote:
>
>> Hm. For some reason the great mail filtering scheme decided to push
>> this past my inbox :-/
>>
>> On Sat, Jun 17, 2017 at 12:44 AM, 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.
>>
>> There are other smells around __(le|be) types that show up in staging
>> that might be worth checking in the rest of the kernel as well. e.g.
>> converting to cpu and storing it back into itself (possibly with its
>> bytes reversed), direct assignments without conversion and what else
>> you might have. sparse obviously already flags anything fishy going on
>> with these types, but cannot distinguish between the classes of
>> errors. I'll need to acquaint myself with spatch a bit more to be able
>> to track that down.
>
> If you have concrete code examples, even fake ones, illustrating a class
> of problem, then that would be great.

I'll see if I can produce some somewhere this week.

Thanks,
Frans

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

[Frans Klaver]

On Sat, Jun 24, 2017 at 1:37 AM, Julia Lawall  wrote:
>
>
> On Sat, 24 Jun 2017, Frans Klaver wrote:
>
>> Hm. For some reason the great mail filtering scheme decided to push
>> this past my inbox :-/
>>
>> On Sat, Jun 17, 2017 at 12:44 AM, 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.
>>
>> There are other smells around __(le|be) types that show up in staging
>> that might be worth checking in the rest of the kernel as well. e.g.
>> converting to cpu and storing it back into itself (possibly with its
>> bytes reversed), direct assignments without conversion and what else
>> you might have. sparse obviously already flags anything fishy going on
>> with these types, but cannot distinguish between the classes of
>> errors. I'll need to acquaint myself with spatch a bit more to be able
>> to track that down.
>
> If you have concrete code examples, even fake ones, illustrating a class
> of problem, then that would be great.

I'll see if I can produce some somewhere this week.

Thanks,
Frans

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

[Julia Lawall]

On Sat, 24 Jun 2017, Frans Klaver wrote:

> Hm. For some reason the great mail filtering scheme decided to push
> this past my inbox :-/
>
> On Sat, Jun 17, 2017 at 12:44 AM, 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.
>
> There are other smells around __(le|be) types that show up in staging
> that might be worth checking in the rest of the kernel as well. e.g.
> converting to cpu and storing it back into itself (possibly with its
> bytes reversed), direct assignments without conversion and what else
> you might have. sparse obviously already flags anything fishy going on
> with these types, but cannot distinguish between the classes of
> errors. I'll need to acquaint myself with spatch a bit more to be able
> to track that down.

If you have concrete code examples, even fake ones, illustrating a class
of problem, then that would be great.

thanks,
julia

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

[Julia Lawall]

On Sat, 24 Jun 2017, Frans Klaver wrote:

> Hm. For some reason the great mail filtering scheme decided to push
> this past my inbox :-/
>
> On Sat, Jun 17, 2017 at 12:44 AM, 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.
>
> There are other smells around __(le|be) types that show up in staging
> that might be worth checking in the rest of the kernel as well. e.g.
> converting to cpu and storing it back into itself (possibly with its
> bytes reversed), direct assignments without conversion and what else
> you might have. sparse obviously already flags anything fishy going on
> with these types, but cannot distinguish between the classes of
> errors. I'll need to acquaint myself with spatch a bit more to be able
> to track that down.

If you have concrete code examples, even fake ones, illustrating a class
of problem, then that would be great.

thanks,
julia

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

[Frans Klaver]

Hm. For some reason the great mail filtering scheme decided to push
this past my inbox :-/

On Sat, Jun 17, 2017 at 12:44 AM, 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.

There are other smells around __(le|be) types that show up in staging
that might be worth checking in the rest of the kernel as well. e.g.
converting to cpu and storing it back into itself (possibly with its
bytes reversed), direct assignments without conversion and what else
you might have. sparse obviously already flags anything fishy going on
with these types, but cannot distinguish between the classes of
errors. I'll need to acquaint myself with spatch a bit more to be able
to track that down.

Thanks,
Frans

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

[Frans Klaver]

Hm. For some reason the great mail filtering scheme decided to push
this past my inbox :-/

On Sat, Jun 17, 2017 at 12:44 AM, 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.

There are other smells around __(le|be) types that show up in staging
that might be worth checking in the rest of the kernel as well. e.g.
converting to cpu and storing it back into itself (possibly with its
bytes reversed), direct assignments without conversion and what else
you might have. sparse obviously already flags anything fishy going on
with these types, but cannot distinguish between the classes of
errors. I'll need to acquaint myself with spatch a bit more to be able
to track that down.

Thanks,
Frans

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

[Julia Lawall]

On Fri, 16 Jun 2017, Joe Perches wrote:

> On Sat, 2017-06-17 at 08:00 +0200, Julia Lawall wrote:
> > 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?
>
> No easy way as far as I can tell.
> I believe it'd require parsing Kconfig.
>
> > > > 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...
>
> I believe the most likely false positives would be in arch/ code
>
> > > 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 = {<<,>>};
>
> thanks.
>
> btw:  After a couple hours with this script, I got a segmentation fault
>
> Here's the output I got running
>
> $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci .

Weird.  I will try it.

thanks,
julia

> diff -u -p ./net/dsa/tag_qca.c /tmp/nothing/net/dsa/tag_qca.c
> --- ./net/dsa/tag_qca.c
> +++ /tmp/nothing/net/dsa/tag_qca.c
> @@ -84,7 +84,6 @@ static struct sk_buff *qca_tag_rcv(struc
>   hdr = ntohs(*phdr);
>
>   /* Make sure the version is correct */
> - ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S;
>   if (unlikely(ver != QCA_HDR_VERSION))
>   return NULL;
>
> diff -u -p ./arch/mips/pci/pci-lantiq.c 
> /tmp/nothing/arch/mips/pci/pci-lantiq.c
> --- ./arch/mips/pci/pci-lantiq.c
> +++ /tmp/nothing/arch/mips/pci/pci-lantiq.c
> @@ -151,7 +151,6 @@ static int ltq_pci_startup(struct platfo
>   /* setup the request mask */
>   req_mask = of_get_property(node, "req-mask", NULL);
>   if (req_mask)
> - temp_buffer &= ~((*req_mask & 0xf) << 16);
>   else
>   temp_buffer &= ~0xf;
>   /* enable internal arbiter */
> diff -u -p ./arch/powerpc/platforms/powernv/opal-lpc.c 
> /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c
> --- ./arch/powerpc/platforms/powernv/opal-lpc.c
> +++ /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c
> @@ -44,7 +44,6 @@ static __le16 __opal_lpc_inw(unsigned lo
>   if (opal_lpc_chip_id < 0 || port > 0xfffe)
>   return 0x;
>   if (port & 1)
> - return (__le16)opal_lpc_inb(port) << 8 | opal_lpc_inb(port + 1);
>   rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, , 2);
>   return rc ? 0x : be32_to_cpu(data);
>  }
> @@ -61,9 +60,6 @@ static __le32 __opal_lpc_inl(unsigned lo
>   if (opal_lpc_chip_id < 0 || port > 0xfffc)
>   return 0x;
>   if (port & 3)
> - return (__le32)opal_lpc_inb(port) << 24 |
> -(__le32)opal_lpc_inb(port + 1) << 16 |
> -(__le32)opal_lpc_inb(port + 2) <<  8 |
>  opal_lpc_inb(port + 3);
>   rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, , 4);
>   return rc ? 0x : be32_to_cpu(data);
> @@ 

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

[Julia Lawall]

On Fri, 16 Jun 2017, Joe Perches wrote:

> On Sat, 2017-06-17 at 08:00 +0200, Julia Lawall wrote:
> > 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?
>
> No easy way as far as I can tell.
> I believe it'd require parsing Kconfig.
>
> > > > 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...
>
> I believe the most likely false positives would be in arch/ code
>
> > > 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 = {<<,>>};
>
> thanks.
>
> btw:  After a couple hours with this script, I got a segmentation fault
>
> Here's the output I got running
>
> $ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci .

Weird.  I will try it.

thanks,
julia

> diff -u -p ./net/dsa/tag_qca.c /tmp/nothing/net/dsa/tag_qca.c
> --- ./net/dsa/tag_qca.c
> +++ /tmp/nothing/net/dsa/tag_qca.c
> @@ -84,7 +84,6 @@ static struct sk_buff *qca_tag_rcv(struc
>   hdr = ntohs(*phdr);
>
>   /* Make sure the version is correct */
> - ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S;
>   if (unlikely(ver != QCA_HDR_VERSION))
>   return NULL;
>
> diff -u -p ./arch/mips/pci/pci-lantiq.c 
> /tmp/nothing/arch/mips/pci/pci-lantiq.c
> --- ./arch/mips/pci/pci-lantiq.c
> +++ /tmp/nothing/arch/mips/pci/pci-lantiq.c
> @@ -151,7 +151,6 @@ static int ltq_pci_startup(struct platfo
>   /* setup the request mask */
>   req_mask = of_get_property(node, "req-mask", NULL);
>   if (req_mask)
> - temp_buffer &= ~((*req_mask & 0xf) << 16);
>   else
>   temp_buffer &= ~0xf;
>   /* enable internal arbiter */
> diff -u -p ./arch/powerpc/platforms/powernv/opal-lpc.c 
> /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c
> --- ./arch/powerpc/platforms/powernv/opal-lpc.c
> +++ /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c
> @@ -44,7 +44,6 @@ static __le16 __opal_lpc_inw(unsigned lo
>   if (opal_lpc_chip_id < 0 || port > 0xfffe)
>   return 0x;
>   if (port & 1)
> - return (__le16)opal_lpc_inb(port) << 8 | opal_lpc_inb(port + 1);
>   rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, , 2);
>   return rc ? 0x : be32_to_cpu(data);
>  }
> @@ -61,9 +60,6 @@ static __le32 __opal_lpc_inl(unsigned lo
>   if (opal_lpc_chip_id < 0 || port > 0xfffc)
>   return 0x;
>   if (port & 3)
> - return (__le32)opal_lpc_inb(port) << 24 |
> -(__le32)opal_lpc_inb(port + 1) << 16 |
> -(__le32)opal_lpc_inb(port + 2) <<  8 |
>  opal_lpc_inb(port + 3);
>   rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, , 4);
>   return rc ? 0x : be32_to_cpu(data);
> @@ 

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

[Joe Perches]

On Sat, 2017-06-17 at 08:00 +0200, Julia Lawall wrote:
> 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?

No easy way as far as I can tell.
I believe it'd require parsing Kconfig.

> > > 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...

I believe the most likely false positives would be in arch/ code

> > 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 = {<<,>>};

thanks.

btw:  After a couple hours with this script, I got a segmentation fault

Here's the output I got running

$ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci .
diff -u -p ./net/dsa/tag_qca.c /tmp/nothing/net/dsa/tag_qca.c
--- ./net/dsa/tag_qca.c
+++ /tmp/nothing/net/dsa/tag_qca.c
@@ -84,7 +84,6 @@ static struct sk_buff *qca_tag_rcv(struc
hdr = ntohs(*phdr);
 
/* Make sure the version is correct */
-   ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S;
if (unlikely(ver != QCA_HDR_VERSION))
return NULL;
 
diff -u -p ./arch/mips/pci/pci-lantiq.c /tmp/nothing/arch/mips/pci/pci-lantiq.c
--- ./arch/mips/pci/pci-lantiq.c
+++ /tmp/nothing/arch/mips/pci/pci-lantiq.c
@@ -151,7 +151,6 @@ static int ltq_pci_startup(struct platfo
/* setup the request mask */
req_mask = of_get_property(node, "req-mask", NULL);
if (req_mask)
-   temp_buffer &= ~((*req_mask & 0xf) << 16);
else
temp_buffer &= ~0xf;
/* enable internal arbiter */
diff -u -p ./arch/powerpc/platforms/powernv/opal-lpc.c 
/tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c
--- ./arch/powerpc/platforms/powernv/opal-lpc.c
+++ /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c
@@ -44,7 +44,6 @@ static __le16 __opal_lpc_inw(unsigned lo
if (opal_lpc_chip_id < 0 || port > 0xfffe)
return 0x;
if (port & 1)
-   return (__le16)opal_lpc_inb(port) << 8 | opal_lpc_inb(port + 1);
rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, , 2);
return rc ? 0x : be32_to_cpu(data);
 }
@@ -61,9 +60,6 @@ static __le32 __opal_lpc_inl(unsigned lo
if (opal_lpc_chip_id < 0 || port > 0xfffc)
return 0x;
if (port & 3)
-   return (__le32)opal_lpc_inb(port) << 24 |
-  (__le32)opal_lpc_inb(port + 1) << 16 |
-  (__le32)opal_lpc_inb(port + 2) <<  8 |
   opal_lpc_inb(port + 3);
rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, , 4);
return rc ? 0x : be32_to_cpu(data);
@@ -86,7 +82,6 @@ static void __opal_lpc_outw(__le16 val,
if (opal_lpc_chip_id < 0 || port > 0xfffe)
return;
if (port & 1) {
-   opal_lpc_outb(val >> 8, port);
opal_lpc_outb(val , port + 1);

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

[Joe Perches]

On Sat, 2017-06-17 at 08:00 +0200, Julia Lawall wrote:
> 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?

No easy way as far as I can tell.
I believe it'd require parsing Kconfig.

> > > 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...

I believe the most likely false positives would be in arch/ code

> > 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 = {<<,>>};

thanks.

btw:  After a couple hours with this script, I got a segmentation fault

Here's the output I got running

$ spatch --all-includes --very-quiet --sp-file lebe_bitshifts.cocci .
diff -u -p ./net/dsa/tag_qca.c /tmp/nothing/net/dsa/tag_qca.c
--- ./net/dsa/tag_qca.c
+++ /tmp/nothing/net/dsa/tag_qca.c
@@ -84,7 +84,6 @@ static struct sk_buff *qca_tag_rcv(struc
hdr = ntohs(*phdr);
 
/* Make sure the version is correct */
-   ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S;
if (unlikely(ver != QCA_HDR_VERSION))
return NULL;
 
diff -u -p ./arch/mips/pci/pci-lantiq.c /tmp/nothing/arch/mips/pci/pci-lantiq.c
--- ./arch/mips/pci/pci-lantiq.c
+++ /tmp/nothing/arch/mips/pci/pci-lantiq.c
@@ -151,7 +151,6 @@ static int ltq_pci_startup(struct platfo
/* setup the request mask */
req_mask = of_get_property(node, "req-mask", NULL);
if (req_mask)
-   temp_buffer &= ~((*req_mask & 0xf) << 16);
else
temp_buffer &= ~0xf;
/* enable internal arbiter */
diff -u -p ./arch/powerpc/platforms/powernv/opal-lpc.c 
/tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c
--- ./arch/powerpc/platforms/powernv/opal-lpc.c
+++ /tmp/nothing/arch/powerpc/platforms/powernv/opal-lpc.c
@@ -44,7 +44,6 @@ static __le16 __opal_lpc_inw(unsigned lo
if (opal_lpc_chip_id < 0 || port > 0xfffe)
return 0x;
if (port & 1)
-   return (__le16)opal_lpc_inb(port) << 8 | opal_lpc_inb(port + 1);
rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, , 2);
return rc ? 0x : be32_to_cpu(data);
 }
@@ -61,9 +60,6 @@ static __le32 __opal_lpc_inl(unsigned lo
if (opal_lpc_chip_id < 0 || port > 0xfffc)
return 0x;
if (port & 3)
-   return (__le32)opal_lpc_inb(port) << 24 |
-  (__le32)opal_lpc_inb(port + 1) << 16 |
-  (__le32)opal_lpc_inb(port + 2) <<  8 |
   opal_lpc_inb(port + 3);
rc = opal_lpc_read(opal_lpc_chip_id, OPAL_LPC_IO, port, , 4);
return rc ? 0x : be32_to_cpu(data);
@@ -86,7 +82,6 @@ static void __opal_lpc_outw(__le16 val,
if (opal_lpc_chip_id < 0 || port > 0xfffe)
return;
if (port & 1) {
-   opal_lpc_outb(val >> 8, port);
opal_lpc_outb(val , port + 1);

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

[Julia Lawall]

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

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

[Julia Lawall]

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

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

[Joe Perches]

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... ]

> > $ 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.

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

Maybe.

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

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

[Joe Perches]

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... ]

> > $ 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.

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

Maybe.

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

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

[Julia Lawall]

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...]
>
> $ cat lebe_bitshifts.cocci
> @@
> typedef __le16, __le32, __le64,  __be16, __be32, __be64;
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> * a << b
>
> @@
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> * a <<= b
>
> @@
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> * a >> b
>
> @@
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> * a >>= b

Is this always a problem?  Would it be useful to add this to the scripts
in the kernel?

julia

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

[Julia Lawall]

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...]
>
> $ cat lebe_bitshifts.cocci
> @@
> typedef __le16, __le32, __le64,  __be16, __be32, __be64;
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> * a << b
>
> @@
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> * a <<= b
>
> @@
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> * a >> b
>
> @@
> { __le16, __le32, __le64,  __be16, __be32, __be64 } a;
> expression b;
> @@
>
> * a >>= b

Is this always a problem?  Would it be useful to add this to the scripts
in the kernel?

julia

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

[Joe Perches]

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...]

$ cat lebe_bitshifts.cocci
@@
typedef __le16, __le32, __le64,  __be16, __be32, __be64;
{ __le16, __le32, __le64,  __be16, __be32, __be64 } a;
expression b;
@@

*   a << b

@@
{ __le16, __le32, __le64,  __be16, __be32, __be64 } a;
expression b;
@@

*   a <<= b

@@
{ __le16, __le32, __le64,  __be16, __be32, __be64 } a;
expression b;
@@

*   a >> b

@@
{ __le16, __le32, __le64,  __be16, __be32, __be64 } a;
expression b;
@@

*   a >>= b
$

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

[Joe Perches]

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...]

$ cat lebe_bitshifts.cocci
@@
typedef __le16, __le32, __le64,  __be16, __be32, __be64;
{ __le16, __le32, __le64,  __be16, __be32, __be64 } a;
expression b;
@@

*   a << b

@@
{ __le16, __le32, __le64,  __be16, __be32, __be64 } a;
expression b;
@@

*   a <<= b

@@
{ __le16, __le32, __le64,  __be16, __be32, __be64 } a;
expression b;
@@

*   a >> b

@@
{ __le16, __le32, __le64,  __be16, __be32, __be64 } a;
expression b;
@@

*   a >>= b
$