Hi Oliver and Rong, This is an interesting and quite surprising issue! On Tue. 23 mars 2021 at 11:54, Rong Chen <rong.a.chen@xxxxxxxxx> wrote: > 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@xxxxxxxxxxxx/ > >>> > >>> > >>> 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?!? Just a random idea but maybe the added padding is due to some kind of odd intrication with the __attribute__((__aligned__(8))) just below? Does this reproduce if we remove the __attribute__((__aligned__(8)))? (I am not saying that we should permanently remove it, this is only a suggestion for troubleshooting). > >> __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. I had a look at the ISO/IEC 9899-1999 (aka C99 standard). In section 6.7.2.1 "Structure and union specifiers", there are no clauses to forbid this behavior... Here are the relevant clauses of that section: * 12 Each non-bit-field member of a structure or union object is aligned in an implementation-defined appropriate to its type. * 13 [...] There may be unnamed padding within a structure object, but not at its beginning. * 14 The size of a union is sufficient to contain the largest of its members. [...] * 15 There may be unnamed padding at the end of a structure or union. So while I am really curious to understand why the compiler behaves like that, technically speaking, it does not violate the standard. As such, I think that Mark's patch (which negates clause 15) makes sense. > 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@xxxxxxxxxxxx/ > >>> > >>> > >>> 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 <lkp@xxxxxxxxx> > >>>> > >>>> All errors (new ones prefixed by >>): > >>>> > >>>> In file included from <command-line>: > >>>> 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 <will@xxxxxxxxxx> > >>>> :::::: CC: Will Deacon <will@xxxxxxxxxx> > >>>> > >>>> --- > >>>> 0-DAY CI Kernel Test Service, Intel Corporation > >>>> https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx > >>>> > >>> _______________________________________________ > >>> kbuild-all mailing list -- kbuild-all@xxxxxxxxxxxx > >>> To unsubscribe send an email to kbuild-all-leave@xxxxxxxxxxxx > >> >
