All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>,
	Justin Stitt <jstitt007@gmail.com>,
	Nathan Chancellor <nathan@kernel.org>, Tom Rix <trix@redhat.com>,
	linux-kernel@vger.kernel.org, llvm@lists.linux.dev,
	Richard Smith <richardsmith@google.com>
Subject: Re: [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional
Date: Wed, 8 Jun 2022 04:54:29 +0000	[thread overview]
Message-ID: <YqArhaiEu+6YWZfg@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <20220607162128.b5d4aa70f4a8a7610ce29250@linux-foundation.org>

On Tue, Jun 07, 2022 at 04:21:28PM -0700, Andrew Morton wrote:

> > 6.5.15/5
> > >> If both the second and third operands have arithmetic type, the result type that would be determined by the usual arithmetic conversions, were they applied to those two operands, is the type of the result.
> > 6.3.1.8/1
> > >> Otherwise, the integer promotions are performed on both operands.
> > 6.3.1.1/2
> > >> If an int can represent all values of the original type (as restricted by the width, for a bit-field), the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions.
> 
> Geeze.  Can we please turn this into English and add it to the changelog?
> 
> Is it saying that an expression
> 
> 	int ? u16 : u16
> 
> has type int?  Or something else?  What did we do wrong here and is it
> possible to correct our types rather than adding a cast?

Not quite.  Same rules as u16 + u16 - on architectures where int is wider
than 16 bits it's (int)u16 + (int)u16 and yields int, on 16bit ones it's
(unsigned int)u16 + (unsigned int)u16 and yields unsigned int.

You *can't* get smaller-than-int out of ? :, same as you can't get it
out of addition, etc.

__builtin_choose_expr() would do it, but I would take a cast over that
ugliness.

FWIW, it might make sense for clang to keep track of the following
property: expression has the same value as it would if integer promotions
in it had been replaced with integer promotion of result.

Example: with
	unsigned short x, y, mask;

expresion "x & y" is interpreted as and_int((int)x, (int)y), which is equal
to (int)and_u16(x, y), so that expression has the property in question.
"x != 12 ? x : y" has the same property.  "x + y", OTOH, doesn't - if x and y
are both 32768, x + y is add_int((int)x, (int)y), i.e. 65536, while
(int)add_u16(x, y) would be 0.

For a somewhat more subtle example,
	(x & ~mask) | (y & mask)
is interpreted as
	or_int(and_int((int)x, not_int((int)mask)), and_int((int)y, (int)mask))
which is equal to
	(int)or_u16(and_u16(x,not_u16(mask)), and_u16(y, mask))
IOW, the property in question holds for that one, despite having a subexpression
(~mask) that does *NOT* have that property.  (int)not_u16(0) is 0xffff and
not_int((int)0) is (assuming 32bit int) 0xffffffff.  Upper 16 bits get fouled;
applying & with known-16bit launders them off...

That predicate is behind the handling of small bitwise types in sparse;
otherwise all operations on __be16 would trigger warnings due to promotions
from __be16 to int.  And aforementioned subtle example is common enough, so we
had to deal with it.  See commit d24967cb847b "[PATCH] handle fouled-bitwise"
in sparse git...

  parent reply	other threads:[~2022-06-08  5:18 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-07 22:20 [PATCH] include/uapi/linux/swab.h: add __u16 cast to __swab16 conditional Justin Stitt
2022-06-07 22:27 ` Andrew Morton
2022-06-07 22:42   ` Nick Desaulniers
2022-06-07 23:21     ` Andrew Morton
2022-06-07 23:43       ` Nick Desaulniers
2022-06-08  4:54       ` Al Viro [this message]
2022-06-08 19:35         ` Nick Desaulniers
2022-06-10  7:19           ` David Laight
2022-06-07 23:20 ` Nick Desaulniers
2022-06-08  0:14   ` [PATCH v2] include/uapi/linux/swab.h: move explicit cast outside ternary Justin Stitt
2022-06-08 20:09     ` Andrew Morton
2022-06-08 21:16       ` Nick Desaulniers
2022-06-08 21:19     ` Nick Desaulniers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YqArhaiEu+6YWZfg@zeniv-ca.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=akpm@linux-foundation.org \
    --cc=jstitt007@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=llvm@lists.linux.dev \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=richardsmith@google.com \
    --cc=trix@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.