Hi Nathan,
On Thu, Oct 10, 2024 at 03:24:51PM -0700, Nathan Chancellor wrote:
> Hi Dmitry,
>
> On Tue, Oct 01, 2024 at 08:42:36AM -0700, Dmitry Torokhov wrote:
> > We can easily calculate the size of the item using arithmetic (shifts).
> > This allows to pull duplicated code out of the switch statement, making
> > it cleaner.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > ---
> > drivers/hid/hid-core.c | 31 ++++++++++++++-----------------
> > 1 file changed, 14 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 988d0acbdf04..00942d40fe08 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -754,35 +754,32 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
> > }
> >
> > item->format = HID_ITEM_FORMAT_SHORT;
> > - item->size = b & 3;
> > + item->size = BIT(b & 3) >> 1; /* 0, 1, 2, 3 -> 0, 1, 2, 4 */
> > +
> > + if (end - start < item->size)
> > + return NULL;
> >
> > switch (item->size) {
> > case 0:
> > - return start;
> > + break;
> >
> > case 1:
> > - if ((end - start) < 1)
> > - return NULL;
> > - item->data.u8 = *start++;
> > - return start;
> > + item->data.u8 = *start;
> > + break;
> >
> > case 2:
> > - if ((end - start) < 2)
> > - return NULL;
> > item->data.u16 = get_unaligned_le16(start);
> > - start = (__u8 *)((__le16 *)start + 1);
> > - return start;
> > + break;
> >
> > - case 3:
> > - item->size++;
> > - if ((end - start) < 4)
> > - return NULL;
> > + case 4:
> > item->data.u32 = get_unaligned_le32(start);
> > - start = (__u8 *)((__le32 *)start + 1);
> > - return start;
> > + break;
> > +
> > + default:
> > + unreachable();
> > }
> >
> > - return NULL;
> > + return start + item->size;
> > }
>
> I am noticing some interesting behavior when building with clang, namely
> some objtool warnings and a failed boot when LTO is enabled, which I
> bisected to this change as commit 61595012f280 ("HID: simplify code in
> fetch_item()"), such as:
>
> $ make -skj"$(nproc)" ARCH=x86_64 LLVM=1 mrproper defconfig vmlinux
> vmlinux.o: warning: objtool: hid_open_report() falls through to next function hid_parser_main()
> vmlinux.o: warning: objtool: hid_scan_report() falls through to next function hid_allocate_device()
>
> With LTO enabled, the warning becomes:
>
> vmlinux.o: warning: objtool: hid_open_report+0x21b: can't find jump dest instruction at .text.hid_open_report+0x40f
>
> A bare unreachable(), especially in the default case of a switch
> statement, is generally considered harmful in my experience, as it can
> introduce undefined behavior, which can mess up how a compiler might
> optimize a function. Commit d652d5f1eeeb ("drm/edid: fix objtool warning
> in drm_cvt_modes()") and commit 3764647b255a ("bcachefs: Remove
> undefined behavior in bch2_dev_buckets_reserved()") have some good
> commit messages talking about it.
>
> Getting rid of the unreachable() in some way resolves the issue. I
> tested using BUG() in lieu of unreachable() like the second change I
> mentioned above, which resolves the issue cleanly, as the default case
> clearly cannot happen. Another option I tested was some sort of printk
> statement and returning NULL, which some maintainers prefer, even in
> spite of impossible conditions. I am happy to send a patch with one of
> those changes or open to other suggestions.
Oh well, if our toolchain does not like "unreachable()" then we can
simply remove it - the switch does cover all possible values and the
"return" statement should be valid even if compiler somehow decides that
"switch" statement can be skipped.
If you can send a patch that would be great.
I'm adding Paul and a few others to CC who apparently seeing the same
issue.
Thanks.
--
Dmitry
