On 3/23/21 12:24 AM, Oliver Hartkopp wrote: > Hi Rong, > > On 22.03.21 09:52, Rong Chen wrote: > >> On 3/21/21 10:19 PM, Oliver Hartkopp wrote: >>> Two reminders in two days? ;-) >>> >>> Did you check my answer here? >>> https://lore.kernel.org/lkml/afffeb73-ba4c-ca2c-75d0-9e7899e5cbe1@hartkopp.net/ >>> >>> >>> And did you try the partly revert? >> >> Hi Oliver, >> >> Sorry for the delay, we tried the revert patch and the problem still >> exists, >> we also found that commit c7b74967 changed the error message which >> triggered >> the report. >> >> The problem is that offsetof(struct can_frame, data) != >> offsetof(struct canfd_frame, data) >> the following struct layout shows that the offset has been changed by >> union: >> >> struct can_frame { >>          canid_t                    can_id;               /* 0     4 */ >>          union { >>                  __u8               len;                  /* 4     1 */ >>                  __u8               can_dlc;              /* 4     1 */ >>          };                                               /* 4     4 */ > > Ugh! Why did the compiler extend the space for the union to 4 bytes?!? > >>          __u8 __pad;                /* 8     1 */ >>          __u8                       __res0;               /* 9     1 */ >>          __u8                       len8_dlc;             /* 10     1 */ >> >>          /* XXX 5 bytes hole, try to pack */ >> >>          __u8                       data[8] >> __attribute__((__aligned__(8))); /*    16     8 */ >> >>          /* size: 24, cachelines: 1, members: 6 */ >>          /* sum members: 19, holes: 1, sum holes: 5 */ >>          /* forced alignments: 1, forced holes: 1, sum forced holes: >> 5 */ >>          /* last cacheline: 24 bytes */ >> } __attribute__((__aligned__(8))); >> >> struct canfd_frame { >>          canid_t                    can_id;               /* 0     4 */ >>          __u8                       len;                  /* 4     1 */ >>          __u8                       flags;                /* 5     1 */ >>          __u8                       __res0;               /* 6     1 */ >>          __u8                       __res1;               /* 7     1 */ >>          __u8                       data[64] >> __attribute__((__aligned__(8))); /*     8    64 */ >> >>          /* size: 72, cachelines: 2, members: 6 */ >>          /* forced alignments: 1 */ >>          /* last cacheline: 8 bytes */ >> } __attribute__((__aligned__(8))) >> >> >> and I tried to add "__attribute__((packed))" to the union, the issue >> is gone: >> >> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h >> index f75238ac6dce..9842bb55ffd9 100644 >> --- a/include/uapi/linux/can.h >> +++ b/include/uapi/linux/can.h >> @@ -113,7 +113,7 @@ struct can_frame { >>                   */ >>                  __u8 len; >>                  __u8 can_dlc; /* deprecated */ >> -       }; >> +       } __attribute__((packed)); >>          __u8 __pad; /* padding */ >>          __u8 __res0; /* reserved / padding */ >>          __u8 len8_dlc; /* optional DLC for 8 byte payload length (9 >> .. 15) */ > > This is pretty strange! > > pahole on my x86_64 machine shows the correct data structure layout: > > struct can_frame { >         canid_t                    can_id;               /* 0     4 */ >         union { >                 __u8               len;                  /* 4     1 */ >                 __u8               can_dlc;              /* 4     1 */ >         };                                               /* 4     1 */ >         __u8                       __pad;                /* 5     1 */ >         __u8                       __res0;               /* 6     1 */ >         __u8                       len8_dlc;             /* 7     1 */ >         __u8                       data[8] > __attribute__((__aligned__(8))); /*     8     8 */ > >         /* size: 16, cachelines: 1, members: 6 */ >         /* forced alignments: 1 */ >         /* last cacheline: 16 bytes */ > } __attribute__((__aligned__(8))); > > Target: x86_64-linux-gnu > gcc version 10.2.1 20210110 (Debian 10.2.1-6) > Linux 5.12.0-rc3-00070-g8b12a62a4e3e x86_64 GNU/Linux > > So it looks like your compiler does not behave correctly - and I > wonder if it would be the correct approach to add the __packed() > attribute or better fix/change the (ARM) compiler. Hi Oliver, I tried arm-linux-gnueabi (gcc version 10.2.0) and the problem still exists, btw we prefer to not use the latest gcc compiler to avoid false positives. Best Regards, Rong Chen > > At least I'm very happy that the BUILD_BUG_ON() triggered correctly - > so it was worth to have it ;-) > > Best regards, > Oliver > > >>> >>> Maybe there's a mismatch in include files - or BUILD_BUG_ON() >>> generally does not work with unions on ARM as assumed here: >>> >>> https://lore.kernel.org/lkml/6e57d5d2-9b88-aee6-fb7a-82e24144d179@hartkopp.net/ >>> >>> >>> In both cases I can not really fix the issue. >>> When the partly revert (suggested above) works, this would be a hack >>> too. >>> >>> Best, >>> Oliver >>> >>> On 20.03.21 21:43, kernel test robot wrote: >>>> Hi Oliver, >>>> >>>> FYI, the error/warning still remains. >>>> >>>> tree: >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git >>>> master >>>> head:   812da4d39463a060738008a46cfc9f775e4bfcf6 >>>> commit: c7b74967799b1af52b3045d69d4c26836b2d41de can: replace >>>> can_dlc as variable/element for payload length >>>> date:   4 months ago >>>> config: arm-randconfig-r016-20210321 (attached as .config) >>>> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0 >>>> reproduce (this is a W=1 build): >>>>          wget >>>> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross >>>> -O ~/bin/make.cross >>>>          chmod +x ~/bin/make.cross >>>>          # >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c7b74967799b1af52b3045d69d4c26836b2d41de >>>> >>>>          git remote add linus >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git >>>>          git fetch --no-tags linus master >>>>          git checkout c7b74967799b1af52b3045d69d4c26836b2d41de >>>>          # save the attached .config to linux build tree >>>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 >>>> make.cross ARCH=arm >>>> >>>> If you fix the issue, kindly add following tag as appropriate >>>> Reported-by: kernel test robot >>>> >>>> All errors (new ones prefixed by >>): >>>> >>>>     In file included from : >>>>     net/can/af_can.c: In function 'can_init': >>>>>> include/linux/compiler_types.h:315:38: error: call to >>>>>> '__compiletime_assert_536' declared with attribute error: >>>>>> BUILD_BUG_ON failed: offsetof(struct can_frame, len) != >>>>>> offsetof(struct canfd_frame, len) || offsetof(struct can_frame, >>>>>> data) != offsetof(struct canfd_frame, data) >>>>       315 |  _compiletime_assert(condition, msg, >>>> __compiletime_assert_, __COUNTER__) >>>>           |                                      ^ >>>>     include/linux/compiler_types.h:296:4: note: in definition of >>>> macro '__compiletime_assert' >>>>       296 |    prefix ## suffix();    \ >>>>           |    ^~~~~~ >>>>     include/linux/compiler_types.h:315:2: note: in expansion of >>>> macro '_compiletime_assert' >>>>       315 |  _compiletime_assert(condition, msg, >>>> __compiletime_assert_, __COUNTER__) >>>>           |  ^~~~~~~~~~~~~~~~~~~ >>>>     include/linux/build_bug.h:39:37: note: in expansion of macro >>>> 'compiletime_assert' >>>>        39 | #define BUILD_BUG_ON_MSG(cond, msg) >>>> compiletime_assert(!(cond), msg) >>>>           | ^~~~~~~~~~~~~~~~~~ >>>>     include/linux/build_bug.h:50:2: note: in expansion of macro >>>> 'BUILD_BUG_ON_MSG' >>>>        50 |  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " >>>> #condition) >>>>           |  ^~~~~~~~~~~~~~~~ >>>>     net/can/af_can.c:891:2: note: in expansion of macro 'BUILD_BUG_ON' >>>>       891 |  BUILD_BUG_ON(offsetof(struct can_frame, len) != >>>>           |  ^~~~~~~~~~~~ >>>> >>>> >>>> vim +/__compiletime_assert_536 +315 include/linux/compiler_types.h >>>> >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  301 >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  302  #define >>>> _compiletime_assert(condition, msg, prefix, suffix) \ >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  303 >>>> __compiletime_assert(condition, msg, prefix, suffix) >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  304 >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  305  /** >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  306   * compiletime_assert - >>>> break build and emit msg if condition is false >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  307   * @condition: a >>>> compile-time constant condition to check >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  308   * @msg:       a >>>> message to emit if condition is false >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  309   * >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  310   * In tradition of >>>> POSIX assert, this macro will break the build if the >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  311   * supplied condition >>>> is *false*, emitting the supplied error message if the >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  312   * compiler has support >>>> to do so. >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  313   */ >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  314  #define >>>> compiletime_assert(condition, msg) \ >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21 @315 >>>> _compiletime_assert(condition, msg, __compiletime_assert_, >>>> __COUNTER__) >>>> eb5c2d4b45e3d2 Will Deacon 2020-07-21  316 >>>> >>>> :::::: The code at line 315 was first introduced by commit >>>> :::::: eb5c2d4b45e3d2d5d052ea6b8f1463976b1020d5 compiler.h: Move >>>> compiletime_assert() macros into compiler_types.h >>>> >>>> :::::: TO: Will Deacon >>>> :::::: CC: Will Deacon >>>> >>>> --- >>>> 0-DAY CI Kernel Test Service, Intel Corporation >>>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org >>>> >>> _______________________________________________ >>> kbuild-all mailing list -- kbuild-all@lists.01.org >>> To unsubscribe send an email to kbuild-all-leave@lists.01.org >>