[-mm] -funsigned-char, x86: make struct p4_event_bind::cntr signed array

  struct p4_event_bind::cntr[][] should be signed because of
the following code:

	int i, j;
        for (i = 0; i < P4_CNTR_LIMIT; i++) {
          ===>  j = bind->cntr[thread][i];
                if (j != -1 && !test_bit(j, used_mask))
                        return j;
        }

Making this member unsigned will make "j" 255 and fail "j != -1"
comparison.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 arch/x86/events/intel/p4.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Thu, Oct 20, 2022 at 3:49 AM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> struct p4_event_bind::cntr[][] should be signed because of
> the following code:
>
>         int i, j;
>         for (i = 0; i < P4_CNTR_LIMIT; i++) {
>           ===>  j = bind->cntr[thread][i];
>                 if (j != -1 && !test_bit(j, used_mask))
>                         return j;
>         }
>
> Making this member unsigned will make "j" 255 and fail "j != -1"
> comparison.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

Nice catch.

Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>

>  arch/x86/events/intel/p4.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/arch/x86/events/intel/p4.c
> +++ b/arch/x86/events/intel/p4.c
> @@ -24,7 +24,7 @@ struct p4_event_bind {
>         unsigned int escr_msr[2];               /* ESCR MSR for this event */
>         unsigned int escr_emask;                /* valid ESCR EventMask bits */
>         unsigned int shared;                    /* event is shared across threads */
> -       char cntr[2][P4_CNTR_LIMIT];            /* counter index (offset), -1 on absence */
> +       signed char cntr[2][P4_CNTR_LIMIT];     /* counter index (offset), -1 on absence */
>  };

This is fine, but I wonder if this is a good occasion to use `s8` instead?

Jason
  

On Thu, Oct 20, 2022 at 9:28 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Nice catch.
>
> Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>

Can we please try to collect these all in one place?

I see that Andrew picked up the original one for -mm, but I think it
would be better if we had one specific place for all of this (one
branch) to collect it all.

I'm actually trying to do a "make allyesconfig" build on x86-64 with
both signed and unsigned char, and trying to see if I can script
something sane to show differences.

Doing the build is easy, but the differences end up being huge just
due to silly constants (ie the whole "one small difference ends up
changing layout, which then causes hundreds of megs of diff just due
to hex constants in the disassembly changing".

I think the problem is that I tried to do the vmlinux file after
linking to make it easier. Doing the individual object files
separately would probably have been a better idea, just to avoid this
kind of relocation offset issues.

There's not a huge amount of pull requests (the week after -rc1 tends
to be quiet for me), so I'll continue to waste my time on this.

                 Linus
  

Hi Linus,

On Thu, Oct 20, 2022 at 11:15 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Can we please try to collect these all in one place?
>
> I see that Andrew picked up the original one for -mm, but I think it
> would be better if we had one specific place for all of this (one
> branch) to collect it all.

Sure. Andrew can drop it from -mm, and I'll collect everything in:

https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/log/?h=unsigned-char&r

And I'll ask Stephen to add that branch to -next.

> I'm actually trying to do a "make allyesconfig" build on x86-64 with
> both signed and unsigned char, and trying to see if I can script
> something sane to show differences.
>
> Doing the build is easy, but the differences end up being huge just
> due to silly constants (ie the whole "one small difference ends up
> changing layout, which then causes hundreds of megs of diff just due
> to hex constants in the disassembly changing".

If you can get a copy of IDA Pro, diaphora is quite nice:
https://github.com/joxeankoret/diaphora

Or sometimes with objdump, I've had more success by keeping debug
symbols, and then trimming offsets from jmps.

Jason
  

On Thu, Oct 20, 2022 at 10:33 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Or sometimes with objdump, I've had more success by keeping debug
> symbols, and then trimming offsets from jmps.

objdump is what I'm using, and it actually seems ok on individual object files.

Now I just need to script the "do all the object files" and see how
massive the end result is.

                Linus
  

On Thu, Oct 20, 2022 at 10:42:25AM -0700, Linus Torvalds wrote:
> On Thu, Oct 20, 2022 at 10:33 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Or sometimes with objdump, I've had more success by keeping debug
> > symbols, and then trimming offsets from jmps.
> 
> objdump is what I'm using, and it actually seems ok on individual object files.
> 
> Now I just need to script the "do all the object files" and see how
> massive the end result is.

For the a/b build, I start with all*config, then:

# Stop painful noise
CONFIG_KCOV=n
CONFIG_GCOV_KERNEL=n
CONFIG_GCC_PLUGINS=n
CONFIG_IKHEADERS=n
CONFIG_KASAN=n
CONFIG_UBSAN=n
CONFIG_KCSAN=n
CONFIG_KMSAN=n
# Get us source/line details
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y
CONFIG_DEBUG_INFO_REDUCED=n CONFIG_DEBUG_INFO_COMPRESSED=n
CONFIG_DEBUG_INFO_SPLIT=n

And to keep other build-time junk stabilized[1], I build with these make
options:

KBUILD_BUILD_TIMESTAMP=1970-01-01
KBUILD_BUILD_USER=user
KBUILD_BUILD_HOST=host
KBUILD_BUILD_VERSION=1

For the code diff, I use:

objdump --disassemble --demangle --no-show-raw-insn --no-addresses

and when doing the manual examination:

objdump --disassemble --demangle --reloc --source -l --no-show-raw-insn


My not-great way to filter out the movsbl/movzbl, I added this to diff:
	-I '\bmov[sz]bl\b'
  

On Thu, Oct 20, 2022 at 11:57 AM Kees Cook <keescook@chromium.org> wrote:
>
> For the a/b build, I start with all*config, then:

Yes, I have that part all figured out.

> For the code diff, I use:
>
> objdump --disassemble --demangle --no-show-raw-insn --no-addresses

This part I still hate.

Have you figured out any way to get objdump to actually show the
relocations in-place in the assembly?

Ie, instead of

        call   <will_become_orphaned_pgrp+0xbf>
                        R_X86_64_PLT32  debug_lockdep_rcu_enabled-0x4

just show it as

        call   debug_lockdep_rcu_enabled

to make the diff - when it exists - hugely more legible?

Because now any code changes will not just show the code changes, but
end up showing a lot of silly changes because the "+0xbf" changes.

I guess I'll just have to remove all of those hex constants anyway,
because they also show up for any jumps inside the functions.

I also explored trying to compare just the generates *.s files, but
that has its own set of problems, notably with gcc label numbering.
Plus they are harder to generate for the full tree with our standard
build rules (maybe there's some trick I haven't thought of to make gcc
keep the '*.s' files as it generates the '*.o' ones).

I do have something that "works", but it turns out to be very noisy,
because while gcc *often* generates almost identical code, then when
it doesn't it can be quite nasty.

When there is a *real* difference, having a nasty diff is fine. For
example, the arch/x86/events/intel/p4.c issue that Alexey found
generates huge differences, because gcc can just see that "ok, that's
never negative", and generates completely different code.

That's good.

But when there's some small change that just changes the offset, it's
just annoying, even with --no-addresses. The hex numbers can be edited
out, but then you have the nop padding changes etc etc.

So getting rid of that kind of pointless noise is just about all the
effort here.

                  Linus
  

On Thu, Oct 20, 2022 at 12:39 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So getting rid of that kind of pointless noise is just about all the
> effort here.

Current status: of 22.5k object files, 971 have differences.

In many cases, the differences are small and trivial. Example:

 - fscrypt_show_test_dummy_encryption() does that same "print a char with %c"

        seq_printf(seq, "%ctest_dummy_encryption=v%d", sep, vers);

which is entirely harmless and exactly the same as that  (but I most
certainly haven't figured out how to automatically script away that
"oh, %c is fine).

And in other cases, there's no actual difference at all, just
different register usage, so the diff looks fairly big, but doesn't
seem to be real.  In one case I looked at, it started with a 'movzbl',
but it was that in both cases, because the type was actually 'unsigned
char' to begin with. But for some reason it just used different
registers. Example:

 - handle_control_request() in drivers/usb/gadget/udc/dummy_hcd.c

   The reason here *seems* to be that

                        char *buf;
                        buf = (char *)urb->transfer_buffer;

   where it really probably should be 'u8 *buf', since it actually
does a cast to 'u8' in one place, but there isn't even any read of
that 'buf' pointer. So the difference seems to be entirely just some
"different type in assignment" cast internal to gcc that then
incidentally generated a random other choice in register allocation.

And in some cases the differences are enormous:

 - drivers/net/wireless/ralink/rt2x00/rt2800lib.c generates a 220kB diff

which seems to be due to entirely different inlining decisions or
something, and the differences are so enormous that I didn't even
start looking at the cause.

There's a fair number of things in between, like fs/ext4/super.c that
generates a lot of differences, some of them obvious, some of them
very much not obvious that may br similar to the
handle_control_request() ones.

*Presumably* the ext4 super.c code is fine (since it has been used on
architectures that already had unsigned char), but it actually
generates a bigger diff than the p4 events driver does...

And that arch/x86/events/intel/p4.c thing that Alexey found sadly does
not stand out at all in that "somewhere in the middle" bunch.

So I think my "hey, we can automate the comparison" is pretty much a
dud, but I'm not giving up quite yet. It's annoying how *most* of the
kernel files show no differences at all, but then I can't even figure
our why other files do show differences with no obvious reason for
them at all.

                Linus
  

On Thu, Oct 20, 2022 at 01:17:33PM -0700, Linus Torvalds wrote:
> On Thu, Oct 20, 2022 at 12:39 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:

...

> And in some cases the differences are enormous:
> 
>  - drivers/net/wireless/ralink/rt2x00/rt2800lib.c generates a 220kB diff
> 
> which seems to be due to entirely different inlining decisions or
> something, and the differences are so enormous that I didn't even
> start looking at the cause.

This one is what we start the epopee from. I think Jason handled it in his last
patch against this certain driver.
  

On Fri, Oct 21, 2022 at 12:34:37AM +0300, Andy Shevchenko wrote:
> On Thu, Oct 20, 2022 at 01:17:33PM -0700, Linus Torvalds wrote:
> > On Thu, Oct 20, 2022 at 12:39 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> 
> ...
> 
> > And in some cases the differences are enormous:
> > 
> >  - drivers/net/wireless/ralink/rt2x00/rt2800lib.c generates a 220kB diff
> > 
> > which seems to be due to entirely different inlining decisions or
> > something, and the differences are so enormous that I didn't even
> > start looking at the cause.
> 
> This one is what we start the epopee from. I think Jason handled it in his last
> patch against this certain driver.

Right, and Kale is taking it for 6.1, because it fixes existing breakage
on ARM. But it's not broken on x86 with -funsigned-char.

Jason
  

On Thu, Oct 20, 2022 at 10:14:54AM -0700, Linus Torvalds wrote:
> On Thu, Oct 20, 2022 at 9:28 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Nice catch.
> >
> > Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> Can we please try to collect these all in one place?
> 
> I see that Andrew picked up the original one for -mm, but I think it
> would be better if we had one specific place for all of this (one
> branch) to collect it all.
> 
> I'm actually trying to do a "make allyesconfig" build on x86-64 with
> both signed and unsigned char, and trying to see if I can script
> something sane to show differences.

It is very entertaining, i've given up and started patching sparse
but it needs more because char constants are ints:


diff --git a/evaluate.c b/evaluate.c
index 61f59ee3..ab607581 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -321,6 +321,10 @@ static struct expression * cast_to(struct expression *old, struct symbol *type)
 	if (old->ctype != &null_ctype && is_same_type(old, type))
 		return old;
 
+	if (is_char_type(old->ctype)) {
+		sparse_error(old->pos, "XXX char");
+	}
+
 	expr = alloc_expression(old->pos, EXPR_IMPLIED_CAST);
 	expr->ctype = type;
 	expr->cast_type = type;
diff --git a/symbol.h b/symbol.h
index 5270fcd7..8e62aca2 100644
--- a/symbol.h
+++ b/symbol.h
@@ -455,6 +455,14 @@ static inline int is_byte_type(struct symbol *type)
 	return type->bit_size == bits_in_char && type->type != SYM_BITFIELD;
 }
 
+static inline int is_char_type(const struct symbol *type)
+{
+	if (type->type == SYM_NODE) {
+		type = type->ctype.base_type;
+	}
+	return type == &char_ctype;
+}
+
 static inline int is_wchar_type(struct symbol *type)
 {
 	if (type->type == SYM_NODE)
  

On Thu, Oct 20, 2022 at 01:17:33PM -0700, Linus Torvalds wrote:
> And in other cases, there's no actual difference at all, just
> different register usage, so the diff looks fairly big, but doesn't
> seem to be real.  In one case I looked at, it started with a 'movzbl',
> but it was that in both cases, because the type was actually 'unsigned
> char' to begin with. But for some reason it just used different
> registers. Example:
> 
>  - handle_control_request() in drivers/usb/gadget/udc/dummy_hcd.c
> 
>    The reason here *seems* to be that
> 
>                         char *buf;
>                         buf = (char *)urb->transfer_buffer;
> 
>    where it really probably should be 'u8 *buf', since it actually
> does a cast to 'u8' in one place, but there isn't even any read of
> that 'buf' pointer. So the difference seems to be entirely just some
> "different type in assignment" cast internal to gcc that then
> incidentally generated a random other choice in register allocation.

I've send a patch for this now:
	https://lore.kernel.org/r/20221021064453.3341050-1-gregkh@linuxfoundation.org
and will take it through the USB tree, unless Jason wants to grab it
through his tree.

thanks,

greg k-h
  

Hi Greg,

On Fri, Oct 21, 2022 at 2:48 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Oct 20, 2022 at 01:17:33PM -0700, Linus Torvalds wrote:
> > And in other cases, there's no actual difference at all, just
> > different register usage, so the diff looks fairly big, but doesn't
> > seem to be real.  In one case I looked at, it started with a 'movzbl',
> > but it was that in both cases, because the type was actually 'unsigned
> > char' to begin with. But for some reason it just used different
> > registers. Example:
> >
> >  - handle_control_request() in drivers/usb/gadget/udc/dummy_hcd.c
> >
> >    The reason here *seems* to be that
> >
> >                         char *buf;
> >                         buf = (char *)urb->transfer_buffer;
> >
> >    where it really probably should be 'u8 *buf', since it actually
> > does a cast to 'u8' in one place, but there isn't even any read of
> > that 'buf' pointer. So the difference seems to be entirely just some
> > "different type in assignment" cast internal to gcc that then
> > incidentally generated a random other choice in register allocation.
>
> I've send a patch for this now:
>         https://lore.kernel.org/r/20221021064453.3341050-1-gregkh@linuxfoundation.org
> and will take it through the USB tree, unless Jason wants to grab it
> through his tree.

This doesn't appear to have any actual effect, but just changes gcc's
register allocation unexpectedly. So feel free to take it, as it
doesn't seem like it's "one of those bad cases" that I'm keeping track
of.

Jason
  

On Fri, Oct 21, 2022 at 03:24:27AM -0400, Jason A. Donenfeld wrote:
> Hi Greg,
> 
> On Fri, Oct 21, 2022 at 2:48 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Oct 20, 2022 at 01:17:33PM -0700, Linus Torvalds wrote:
> > > And in other cases, there's no actual difference at all, just
> > > different register usage, so the diff looks fairly big, but doesn't
> > > seem to be real.  In one case I looked at, it started with a 'movzbl',
> > > but it was that in both cases, because the type was actually 'unsigned
> > > char' to begin with. But for some reason it just used different
> > > registers. Example:
> > >
> > >  - handle_control_request() in drivers/usb/gadget/udc/dummy_hcd.c
> > >
> > >    The reason here *seems* to be that
> > >
> > >                         char *buf;
> > >                         buf = (char *)urb->transfer_buffer;
> > >
> > >    where it really probably should be 'u8 *buf', since it actually
> > > does a cast to 'u8' in one place, but there isn't even any read of
> > > that 'buf' pointer. So the difference seems to be entirely just some
> > > "different type in assignment" cast internal to gcc that then
> > > incidentally generated a random other choice in register allocation.
> >
> > I've send a patch for this now:
> >         https://lore.kernel.org/r/20221021064453.3341050-1-gregkh@linuxfoundation.org
> > and will take it through the USB tree, unless Jason wants to grab it
> > through his tree.
> 
> This doesn't appear to have any actual effect, but just changes gcc's
> register allocation unexpectedly. So feel free to take it, as it
> doesn't seem like it's "one of those bad cases" that I'm keeping track
> of.

Great, will take it through my tree, thanks!

greg k-h
  

On Thu, Oct 20, 2022 at 10:59 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> It is very entertaining, i've given up and started patching sparse
> but it needs more because char constants are ints:

I think you can fix that by simply warning about character constants
with the high bit set.

Something like this..

              Linus
  

On Fri, Oct 21, 2022 at 10:11 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I think you can fix that by simply warning about character constants
> with the high bit set.
>
> Something like this..

This actually triggers for the kernel, and I think those (few)
warnings are likely worth just fixing.

Because things like

                put_tty_queue('\377', ldata);

just isn't worth it.

Or look at this horror:

                        if (c == (unsigned char) '\377' && I_PARMRK(tty))

where somebody did realize that '\377' might be -1, so they added the
"helpful" cast.

Wouldn't that be much nicer and simpler as just

                        if (c == 255 && I_PARMRK(tty))

instead? Or just 0377 if you really love octal in the context of
characters (and really, nobody should). Or 0xff.

At no point does "(unsigned char) '\377' " strike me as a really
readable way to write things. There's two of those things.

We also have

        memset(stack, '\xff', 64);

which really isn't helpful either. Why not just use 0xff, or even just -1.

We also have a lot of those in lib/hexdump.c, for no good reason,
particularly as those arrays are 'unsigned char[]' to begin with, not
'char'.

I really don't understand why people would use '\xAA' instead of just
using 0xAA.

We also have

        static char sample_rate_buffer[4] = { '\x80', '\xbb', '\x00', '\x00' };

in sound/usb/mixer_scarlett.c, and that should be a byte array, so the
'char' should probably be 'unsigned char' or 'u8' in the first place,
and again it would be just simpler and clearer to use plain hex
constants.

So that sparse warning looks simple enough, and I think every single
case was just the kernel being odd.

Now, in *string* constants, you sometimes do want to use that format, ie

    u8 array[] = "abcd\377";

is a reasonable way to avoid having to use a more complex initializer.
But that sparse patch of mine only complains about (non-wide)
character constants unless I screwed something up.

                  Linus
  

On Thu, Oct 20, 2022 at 11:33:39AM -0600, Jason A. Donenfeld wrote:
> Hi Linus,
> 
> On Thu, Oct 20, 2022 at 11:15 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > Can we please try to collect these all in one place?
> >
> > I see that Andrew picked up the original one for -mm, but I think it
> > would be better if we had one specific place for all of this (one
> > branch) to collect it all.
> 
> Sure. Andrew can drop it from -mm, and I'll collect everything in:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/log/?h=unsigned-char&r
> 
> And I'll ask Stephen to add that branch to -next.

So, as discussed, I'm doing this, and it's in -next now. And as a
result, we're getting warnings and such that I am fixing one by one.
Progress, good.

But it occurs to me that most of these bugs are not in
architecture-specific code like the x86 p4_event_bind one from last
week. That means I'll be submitting these as *fixes* during 6.1, since
they're broken on some architecture already, rather than waiting to
submit them to you via my unsigned-char tree in 6.2.

Just FYI.

Jason
  

On Thu, Oct 20, 2022 at 11:57:12AM -0700, Kees Cook wrote:
> On Thu, Oct 20, 2022 at 10:42:25AM -0700, Linus Torvalds wrote:
> > On Thu, Oct 20, 2022 at 10:33 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > Or sometimes with objdump, I've had more success by keeping debug
> > > symbols, and then trimming offsets from jmps.
> > 
> > objdump is what I'm using, and it actually seems ok on individual object files.
> > 
> > Now I just need to script the "do all the object files" and see how
> > massive the end result is.
> 
> For the a/b build, I start with all*config, then:
> 
> # Stop painful noise
> CONFIG_KCOV=n
> CONFIG_GCOV_KERNEL=n
> CONFIG_GCC_PLUGINS=n
> CONFIG_IKHEADERS=n
> CONFIG_KASAN=n
> CONFIG_UBSAN=n
> CONFIG_KCSAN=n
> CONFIG_KMSAN=n
> # Get us source/line details
> CONFIG_DEBUG_KERNEL=y
> CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y
> CONFIG_DEBUG_INFO_REDUCED=n CONFIG_DEBUG_INFO_COMPRESSED=n
> CONFIG_DEBUG_INFO_SPLIT=n
> 
> And to keep other build-time junk stabilized[1], I build with these make
> options:
> 
> KBUILD_BUILD_TIMESTAMP=1970-01-01
> KBUILD_BUILD_USER=user
> KBUILD_BUILD_HOST=host
> KBUILD_BUILD_VERSION=1

The LLVM `.ll` file thing I tried turned out to be a disaster. Too much
noise, as this is too early of a stage.

The traditional objdump comparison does work, though. It produces a good
amount of noise, but still yields a manageable amount of diffs -- 882 --
which can then be paired down more with heuristics.

I've been using this script below to compare `linux-schar/` with
`linux-uchar/`, which creates a directory `linux-schar-uchar/` filled
with color diffs that I can then flip through using
`less -R linux-schar-uchar/*.diff`. Seems to work okay, so I'll post it
here in case others are curious about looking through these.

Jason

------8<--------------------------------

#!/bin/bash

asm_diff() {
	objdump \
		--disassemble \
		--demangle \
		--no-show-raw-insn \
		--no-addresses \
		--section=.text \
		--disassembler-options=intel \
	"$1" | \
	sed \
		-e 's/[0-9a-f]\+ \(<[a-zA-Z0-9_+-]\+>\)/?? \1/g' \
		-e 's/<\([a-zA-Z0-9_-]\+\)+0x[a-f0-9]\+>/<\1>/g' \
		-e '/\/[a-zA-Z0-9._-]\+\.o:/d'
}

A=linux-schar
B=linux-uchar
C=linux-schar-uchar

rm -rf "$C"
mkdir -p "$C"

while read -r obj_a; do
	obj_b="$B/${obj_a#$A/}"
	diff_c="${obj_a#$A/}"
	diff_c="$C/${diff_c//\//--}.diff"
	[[ -f $obj_b ]] || { echo "ERROR: $obj_b is missing" >&2; exit 1; }

	echo "${obj_a#$A/}" >&2

	diff --color=always --text --unified=10 \
		<(asm_diff "$obj_a") <(asm_diff "$obj_b") > "$diff_c" && \
	rm -f "$diff_c"
done < <(exec find "$A" -name '*.o')
  

On Wed, Oct 26, 2022 at 03:50:25AM +0200, Jason A. Donenfeld wrote:
> The traditional objdump comparison does work, though. It produces a good

Another thing that appears to work well is just using Coccinelle
scripts. I've had some success just scrolling through the results of:

    @@
    char c;
    expression E;
    @@
    (
    * E > c
    |
    * E >= c
    |
    * E < c
    |
    * E <= c
    )

That also triggers on explicitly signed chars, and examining those
reveals that quite a bit of code in the tree already does do the right
thing, which is good.

From looking at this and objdump output, it looks like most naked-char
usage that isn't for strings is actually already assuming it's unsigned,
using it as a byte. I'll continue to churn, and I'm sure I'll miss a few
things here and there, but all and all, I don't think this is looking as
terrible as I initially feared.

I'm CC'ing the Coccinelle people to see if they have any nice ideas on
improvements. Specifically, the thing we're trying to identify is:

  - Usage of vanilla `char`, without a `signed` or `unsigned` qualifier,
    where:
  - It's not being used for characters; and
  - It's doing something that assumes it is signed, such as various
    types of comparisons or decrements.

LWN wrote a summary of the general problem, in case that helps describe
what would be useful: https://lwn.net/SubscriberLink/911914/f90c2ed1af23cbc4/

Any nice Cocci tricks for this?

Jason
  

+Cc: Rasmus as he has done a lot regarding library stuff and optimizations and
     he knows Coccinelle (to some extent as far as I can tell).

On Wed, Oct 26, 2022 at 02:58:34PM +0200, Jason A. Donenfeld wrote:
> On Wed, Oct 26, 2022 at 03:50:25AM +0200, Jason A. Donenfeld wrote:
> > The traditional objdump comparison does work, though. It produces a good
> 
> Another thing that appears to work well is just using Coccinelle
> scripts. I've had some success just scrolling through the results of:
> 
>     @@
>     char c;
>     expression E;
>     @@
>     (
>     * E > c
>     |
>     * E >= c
>     |
>     * E < c
>     |
>     * E <= c
>     )
> 
> That also triggers on explicitly signed chars, and examining those
> reveals that quite a bit of code in the tree already does do the right
> thing, which is good.
> 
> From looking at this and objdump output, it looks like most naked-char
> usage that isn't for strings is actually already assuming it's unsigned,
> using it as a byte. I'll continue to churn, and I'm sure I'll miss a few
> things here and there, but all and all, I don't think this is looking as
> terrible as I initially feared.
> 
> I'm CC'ing the Coccinelle people to see if they have any nice ideas on
> improvements. Specifically, the thing we're trying to identify is:
> 
>   - Usage of vanilla `char`, without a `signed` or `unsigned` qualifier,
>     where:
>   - It's not being used for characters; and
>   - It's doing something that assumes it is signed, such as various
>     types of comparisons or decrements.
> 
> LWN wrote a summary of the general problem, in case that helps describe
> what would be useful: https://lwn.net/SubscriberLink/911914/f90c2ed1af23cbc4/
> 
> Any nice Cocci tricks for this?
  

On Wed, 26 Oct 2022, Jason A. Donenfeld wrote:

> On Wed, Oct 26, 2022 at 03:50:25AM +0200, Jason A. Donenfeld wrote:
> > The traditional objdump comparison does work, though. It produces a good
>
> Another thing that appears to work well is just using Coccinelle
> scripts. I've had some success just scrolling through the results of:
>
>     @@
>     char c;
>     expression E;
>     @@
>     (
>     * E > c
>     |
>     * E >= c
>     |
>     * E < c
>     |
>     * E <= c
>     )
>
> That also triggers on explicitly signed chars, and examining those
> reveals that quite a bit of code in the tree already does do the right
> thing, which is good.
>
> From looking at this and objdump output, it looks like most naked-char
> usage that isn't for strings is actually already assuming it's unsigned,
> using it as a byte. I'll continue to churn, and I'm sure I'll miss a few
> things here and there, but all and all, I don't think this is looking as
> terrible as I initially feared.
>
> I'm CC'ing the Coccinelle people to see if they have any nice ideas on
> improvements. Specifically, the thing we're trying to identify is:
>
>   - Usage of vanilla `char`, without a `signed` or `unsigned` qualifier,
>     where:

Try putting

disable optional_qualifier

between the initial @@, to avoid the implicit matching of signed and
unsigned.

>   - It's not being used for characters; and
>   - It's doing something that assumes it is signed, such as various
>     types of comparisons or decrements.

I took a quick look at the article, but I'm not completely sure what you
are getting at here.  Could you give some examples of what you do and
don't want to find?

You don't want the case where c is 'x', for some x?

julia

> LWN wrote a summary of the general problem, in case that helps describe
> what would be useful: https://lwn.net/SubscriberLink/911914/f90c2ed1af23cbc4/
>
> Any nice Cocci tricks for this?
>
> Jason
>
  

On Wed, Nov 02, 2022 at 06:17:04PM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 26 Oct 2022, Jason A. Donenfeld wrote:
> 
> > On Wed, Oct 26, 2022 at 03:50:25AM +0200, Jason A. Donenfeld wrote:
> > > The traditional objdump comparison does work, though. It produces a good
> >
> > Another thing that appears to work well is just using Coccinelle
> > scripts. I've had some success just scrolling through the results of:
> >
> >     @@
> >     char c;
> >     expression E;
> >     @@
> >     (
> >     * E > c
> >     |
> >     * E >= c
> >     |
> >     * E < c
> >     |
> >     * E <= c
> >     )
> >
> > That also triggers on explicitly signed chars, and examining those
> > reveals that quite a bit of code in the tree already does do the right
> > thing, which is good.
> >
> > From looking at this and objdump output, it looks like most naked-char
> > usage that isn't for strings is actually already assuming it's unsigned,
> > using it as a byte. I'll continue to churn, and I'm sure I'll miss a few
> > things here and there, but all and all, I don't think this is looking as
> > terrible as I initially feared.
> >
> > I'm CC'ing the Coccinelle people to see if they have any nice ideas on
> > improvements. Specifically, the thing we're trying to identify is:
> >
> >   - Usage of vanilla `char`, without a `signed` or `unsigned` qualifier,
> >     where:
> 
> Try putting
> 
> disable optional_qualifier
> 
> between the initial @@, to avoid the implicit matching of signed and
> unsigned.

Hmm, this doesn't quite work. Here are my rules:

    @disable optional_qualifier@
    char c;
    expression E;
    @@
    (
    * E > c
    |
    * E >= c
    |
    * E < c
    |
    * E <= c
    )
    
    @disable optional_qualifier@
    char c;
    @@
    * c == -1
    
    @disable optional_qualifier@
    char c;
    @@
    * c = -1

This produces, for example:

diff -u -p ./sound/firewire/bebob/bebob_focusrite.c /tmp/nothing/sound/firewire/bebob/bebob_focusrite.c
--- ./sound/firewire/bebob/bebob_focusrite.c
+++ /tmp/nothing/sound/firewire/bebob/bebob_focusrite.c
@@ -192,7 +192,6 @@ saffirepro_both_clk_src_get(struct snd_b

        /* In a case that this driver cannot handle the value of register. */
        value &= SAFFIREPRO_CLOCK_SOURCE_SELECT_MASK;
-       if (value >= SAFFIREPRO_CLOCK_SOURCE_COUNT || map[value] < 0) {
                err = -EIO;
                goto end;
        }

Except map is defined as:

    const signed char *map;

So this would be one of those cases that I had hoped `disable
optional_qualifier` would exclude. (I think internally coccinelle might
be assuming `char` is signed, by the way.)

> >   - It's not being used for characters; and
> >   - It's doing something that assumes it is signed, such as various
> >     types of comparisons or decrements.
> 
> I took a quick look at the article, but I'm not completely sure what you
> are getting at here.  Could you give some examples of what you do and
> don't want to find?
> 
> You don't want the case where c is 'x', for some x?

Something I would want to find is `if (c < 0)`. Something I wouldn't
want to find is `if (c < '9')`. IOW, I'm looking for code that assumes
`c` is signed, and would become incorrect if `c` suddenly became
unsigned. Most things involving actual characters are fine. But most
things involving signed arithmetic or comparisons with numbers isn't
find.

Jason
  

On Thu, 3 Nov 2022, Jason A. Donenfeld wrote:

> On Wed, Nov 02, 2022 at 06:17:04PM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 26 Oct 2022, Jason A. Donenfeld wrote:
> >
> > > On Wed, Oct 26, 2022 at 03:50:25AM +0200, Jason A. Donenfeld wrote:
> > > > The traditional objdump comparison does work, though. It produces a good
> > >
> > > Another thing that appears to work well is just using Coccinelle
> > > scripts. I've had some success just scrolling through the results of:
> > >
> > >     @@
> > >     char c;
> > >     expression E;
> > >     @@
> > >     (
> > >     * E > c
> > >     |
> > >     * E >= c
> > >     |
> > >     * E < c
> > >     |
> > >     * E <= c
> > >     )
> > >
> > > That also triggers on explicitly signed chars, and examining those
> > > reveals that quite a bit of code in the tree already does do the right
> > > thing, which is good.
> > >
> > > From looking at this and objdump output, it looks like most naked-char
> > > usage that isn't for strings is actually already assuming it's unsigned,
> > > using it as a byte. I'll continue to churn, and I'm sure I'll miss a few
> > > things here and there, but all and all, I don't think this is looking as
> > > terrible as I initially feared.
> > >
> > > I'm CC'ing the Coccinelle people to see if they have any nice ideas on
> > > improvements. Specifically, the thing we're trying to identify is:
> > >
> > >   - Usage of vanilla `char`, without a `signed` or `unsigned` qualifier,
> > >     where:
> >
> > Try putting
> >
> > disable optional_qualifier
> >
> > between the initial @@, to avoid the implicit matching of signed and
> > unsigned.
>
> Hmm, this doesn't quite work. Here are my rules:
>
>     @disable optional_qualifier@
>     char c;
>     expression E;
>     @@
>     (
>     * E > c
>     |
>     * E >= c
>     |
>     * E < c
>     |
>     * E <= c
>     )
>
>     @disable optional_qualifier@
>     char c;
>     @@
>     * c == -1
>
>     @disable optional_qualifier@
>     char c;
>     @@
>     * c = -1
>
> This produces, for example:
>
> diff -u -p ./sound/firewire/bebob/bebob_focusrite.c /tmp/nothing/sound/firewire/bebob/bebob_focusrite.c
> --- ./sound/firewire/bebob/bebob_focusrite.c
> +++ /tmp/nothing/sound/firewire/bebob/bebob_focusrite.c
> @@ -192,7 +192,6 @@ saffirepro_both_clk_src_get(struct snd_b
>
>         /* In a case that this driver cannot handle the value of register. */
>         value &= SAFFIREPRO_CLOCK_SOURCE_SELECT_MASK;
> -       if (value >= SAFFIREPRO_CLOCK_SOURCE_COUNT || map[value] < 0) {
>                 err = -EIO;
>                 goto end;
>         }
>
> Except map is defined as:
>
>     const signed char *map;
>
> So this would be one of those cases that I had hoped `disable
> optional_qualifier` would exclude. (I think internally coccinelle might
> be assuming `char` is signed, by the way.)

OK, I see the problem.  Coccinelle isn't taking the "disable
optional_qualifier" into account when it checks types on expressions.  It
would work if you put, eg:

char x;
... when any
* x < 0

But that would be much slower and less general.  I will fix it.

>
> > >   - It's not being used for characters; and
> > >   - It's doing something that assumes it is signed, such as various
> > >     types of comparisons or decrements.
> >
> > I took a quick look at the article, but I'm not completely sure what you
> > are getting at here.  Could you give some examples of what you do and
> > don't want to find?
> >
> > You don't want the case where c is 'x', for some x?
>
> Something I would want to find is `if (c < 0)`. Something I wouldn't
> want to find is `if (c < '9')`. IOW, I'm looking for code that assumes
> `c` is signed, and would become incorrect if `c` suddenly became
> unsigned. Most things involving actual characters are fine. But most
> things involving signed arithmetic or comparisons with numbers isn't
> find.

This seems to do what you want:

@disable optional_qualifier@
constant char cc;
expression e;
char c;
@@

(
  c < cc
|
* c < e
)

It highlights only the two return lines in:

int main () {
        char x;
        if (x < 'd')
                return x < 0;
        else return x < y;
}


julia

>
> Jason
>
  

On Thu, 3 Nov 2022, Jason A. Donenfeld wrote:

> On Wed, Nov 02, 2022 at 06:17:04PM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 26 Oct 2022, Jason A. Donenfeld wrote:
> >
> > > On Wed, Oct 26, 2022 at 03:50:25AM +0200, Jason A. Donenfeld wrote:
> > > > The traditional objdump comparison does work, though. It produces a good
> > >
> > > Another thing that appears to work well is just using Coccinelle
> > > scripts. I've had some success just scrolling through the results of:
> > >
> > >     @@
> > >     char c;
> > >     expression E;
> > >     @@
> > >     (
> > >     * E > c
> > >     |
> > >     * E >= c
> > >     |
> > >     * E < c
> > >     |
> > >     * E <= c
> > >     )
> > >
> > > That also triggers on explicitly signed chars, and examining those
> > > reveals that quite a bit of code in the tree already does do the right
> > > thing, which is good.
> > >
> > > From looking at this and objdump output, it looks like most naked-char
> > > usage that isn't for strings is actually already assuming it's unsigned,
> > > using it as a byte. I'll continue to churn, and I'm sure I'll miss a few
> > > things here and there, but all and all, I don't think this is looking as
> > > terrible as I initially feared.
> > >
> > > I'm CC'ing the Coccinelle people to see if they have any nice ideas on
> > > improvements. Specifically, the thing we're trying to identify is:
> > >
> > >   - Usage of vanilla `char`, without a `signed` or `unsigned` qualifier,
> > >     where:
> >
> > Try putting
> >
> > disable optional_qualifier
> >
> > between the initial @@, to avoid the implicit matching of signed and
> > unsigned.
>
> Hmm, this doesn't quite work. Here are my rules:

It should work now.  However, without disable optional_qualifier, char is
still matching signed char.  If you think that should be changed, I can do
that.

julia


>
>     @disable optional_qualifier@
>     char c;
>     expression E;
>     @@
>     (
>     * E > c
>     |
>     * E >= c
>     |
>     * E < c
>     |
>     * E <= c
>     )
>
>     @disable optional_qualifier@
>     char c;
>     @@
>     * c == -1
>
>     @disable optional_qualifier@
>     char c;
>     @@
>     * c = -1
>
> This produces, for example:
>
> diff -u -p ./sound/firewire/bebob/bebob_focusrite.c /tmp/nothing/sound/firewire/bebob/bebob_focusrite.c
> --- ./sound/firewire/bebob/bebob_focusrite.c
> +++ /tmp/nothing/sound/firewire/bebob/bebob_focusrite.c
> @@ -192,7 +192,6 @@ saffirepro_both_clk_src_get(struct snd_b
>
>         /* In a case that this driver cannot handle the value of register. */
>         value &= SAFFIREPRO_CLOCK_SOURCE_SELECT_MASK;
> -       if (value >= SAFFIREPRO_CLOCK_SOURCE_COUNT || map[value] < 0) {
>                 err = -EIO;
>                 goto end;
>         }
>
> Except map is defined as:
>
>     const signed char *map;
>
> So this would be one of those cases that I had hoped `disable
> optional_qualifier` would exclude. (I think internally coccinelle might
> be assuming `char` is signed, by the way.)
>
> > >   - It's not being used for characters; and
> > >   - It's doing something that assumes it is signed, such as various
> > >     types of comparisons or decrements.
> >
> > I took a quick look at the article, but I'm not completely sure what you
> > are getting at here.  Could you give some examples of what you do and
> > don't want to find?
> >
> > You don't want the case where c is 'x', for some x?
>
> Something I would want to find is `if (c < 0)`. Something I wouldn't
> want to find is `if (c < '9')`. IOW, I'm looking for code that assumes
> `c` is signed, and would become incorrect if `c` suddenly became
> unsigned. Most things involving actual characters are fine. But most
> things involving signed arithmetic or comparisons with numbers isn't
> find.
>
> Jason
>
  

Hi Julia,

On Thu, Nov 3, 2022 at 1:45 PM Julia Lawall <julia.lawall@inria.fr> wrote:
> It should work now.

Thanks!

> However, without disable optional_qualifier, char is
> still matching signed char.  If you think that should be changed, I can do
> that.

Does `optional_qualifier` disable other things that might be
interesting to have? If so, maybe this is less than ideal? If not,
maybe it doesn't matter?

Though, for what it's worth, gcc treats `char` as a separate type,
even when using `-funsigned-char` or `-fsigned-char`.

Jason
  

On Thu, 3 Nov 2022, Jason A. Donenfeld wrote:

> Hi Julia,
>
> On Thu, Nov 3, 2022 at 1:45 PM Julia Lawall <julia.lawall@inria.fr> wrote:
> > It should work now.
>
> Thanks!
>
> > However, without disable optional_qualifier, char is
> > still matching signed char.  If you think that should be changed, I can do
> > that.
>
> Does `optional_qualifier` disable other things that might be
> interesting to have? If so, maybe this is less than ideal? If not,
> maybe it doesn't matter?

Optional qualifier only allows a metavariable declared to have a certain
type to match an expression that has the same type with signed, const, or
verbatim in front of it.  Disabling it forces you to write our signed,
const etc explicitly when you want them.  So rules may becomes more
verbose.

julia

>
> Though, for what it's worth, gcc treats `char` as a separate type,
> even when using `-funsigned-char` or `-fsigned-char`.
>
> Jason
>
  

On Thu, Nov 03, 2022 at 01:57:26PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 3 Nov 2022, Jason A. Donenfeld wrote:
> 
> > Hi Julia,
> >
> > On Thu, Nov 3, 2022 at 1:45 PM Julia Lawall <julia.lawall@inria.fr> wrote:
> > > It should work now.
> >
> > Thanks!
> >
> > > However, without disable optional_qualifier, char is
> > > still matching signed char.  If you think that should be changed, I can do
> > > that.
> >
> > Does `optional_qualifier` disable other things that might be
> > interesting to have? If so, maybe this is less than ideal? If not,
> > maybe it doesn't matter?
> 
> Optional qualifier only allows a metavariable declared to have a certain
> type to match an expression that has the same type with signed, const, or
> verbatim in front of it.  Disabling it forces you to write our signed,
> const etc explicitly when you want them.  So rules may becomes more
> verbose.

Oh, huh. Maybe best to treat it as a different type then so that's not
required? I was also thinking that it doesn't totally make sense the way
it is now, in that `char` is *NOT* signed on many platforms, such as
arm. In 6.2, it'll be unsigned everywhere, for kernel code. So in the
general case, for coccinelle, it's a bit of a heisentype and so maybe
should be treated as distinct from `signed char` or `unsigned char`.

Jason