Message-ID: <CAHk-=wi=eDN4Ub0qSN27ztBAvHSXCyiY2stu3_XbTpYpQX4x7w@mail.gmail.com>
Date:   Fri, 23 Jun 2023 11:24:16 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Dave Airlie <airlied@...il.com>
Cc:     Arnd Bergmann <arnd@...db.de>, Tejun Heo <tj@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: arm32 build warnings in workqueue.c

On Thu, 22 Jun 2023 at 20:57, Dave Airlie <airlied@...il.com> wrote:
>
> Not sure what changed (maybe I ended up with -Werror on recently), but
> my 32-bit arm build started to fail. 6.4.0-rc7.
>
> gcc version 13.1.1 20230519 (Red Hat Cross 13.1.1-2) (GCC)
>
> /home/airlied/devel/kernel/dim/drm-fixes/kernel/workqueue.c: In
> function ‘get_work_pwq’:
> /home/airlied/devel/kernel/dim/drm-fixes/kernel/workqueue.c:713:24:
> error: cast to pointer from integer of different size
> [-Werror=int-to-pointer-cast]
>   713 |                 return (void *)(data & WORK_STRUCT_WQ_DATA_MASK);
>       |                        ^

Ok, that's some nasty code, but I'm not entirely sure why gcc has
started complaining about it now.

'data' is of type 'unsigned long', and we cast things to pointers all the time..

Now, WORK_STRUCT_WQ_DATA_MASK is this odd enum type, and very very
arguably that is absolutely *horrible*, but the code does

  enum {
        [..]
        WORK_STRUCT_FLAG_BITS   = WORK_STRUCT_COLOR_SHIFT +
                WORK_STRUCT_COLOR_BITS,
        [..]
        WORK_STRUCT_FLAG_MASK   = (1UL << WORK_STRUCT_FLAG_BITS) - 1,
        WORK_STRUCT_WQ_DATA_MASK = ~WORK_STRUCT_FLAG_MASK,
  }

and while the above is absolutely disgusting and the type is not very
well defined, an enum type should be wide enough to contain all the
values of the enum. It should all default to 'int', but gcc has some
extensions.

The 'intent' here is that WORK_STRUCT_FLAG_MASK is of type UL, but
that's not actually how enum types necessarily work. Again, the enum
should be large enough to contain the *values* of the elements, not
necessarily the *types* of the elements. The type is always going to
be that enum.

(And again, I think according to the standard, it's always of type
'int', but every compiler does some kind of type widening for larger
values as an extension, and may use a smaller type for storage).

But even if WORK_STRUCT_FLAG_MASK isn't an unsigned long, and is
something smaller, the expression '~WORK_STRUCT_FLAG_MASK' must be *at
least* an 'int', and would be a negative one if so. That's just how C
integer expressions work - the '~' operator is guaranteed to make it
at *least* an int.

Now, again, the final type of an enum is not determined by the types
of the element initializers, but by their *values*. But that means
that the final type of an enum will have two choices:

 - either ~WORK_STRUCT_FLAG_MASK was extended to be 'int', and the
value is negative, and the enum type has to be a signed type that can
contain that negative value.

   IOW, the enum might actually be of type 'signed char', but then
doing that 'data & enum' will sign-extend the mask and everything is
fine.

 - *or* the type of WORK_STRUCT_FLAG_MASK was interpreted to be an
unsigned type >= int, and the negation made it a really big unsigned
value, and that enum must thus be a large unsigned type to fit it

   IOW, the enum might be an unsigned type, but it will be at LEAST of
size 'int', and I don't see why it would be a problem on 32-bit ARM.
Making it be 'unsigned long long' sounds insane. But maybe that is
what happened.

In either case, the code above should do the right thing, at least on
32-bit, but the warning is, I feel, valid.

Because as you can tell, the 'type' of an enum really isn't very obvious.

I think the whole 'type of an enum' is not only a very very bad thing
to rely on, I think it's something that C23 ends up codifying new
rules for. It may be why gcc started complaining now.

Anyway, that code absolutely has to be fixed. Using enums for types is
wrong, wrong, wrong. You should consider an enum to be a weakly typed
integer expression with some specific advantages (the automatic
incrementing at definition time, and the compiler being able to limit
ranges in switch() statements and maybe doing limited type warnings
elsewhere)

Any time you think it has a specific type size, you're setting
yourself up for pain and suffering. Gcc is right to warn when we do
odd arithmetic with it and rely on the width of the result.

So I really think that code needs fixing, and the gcc warning was very valid.

Maybe something like the attached. Does this fix the gcc warning?
Tejun, comments?

                      Linus

View attachment "patch.diff" of type "text/x-patch" (3066 bytes)

Powered by blists - more mailing lists