Re: [kbuild-all] Re: 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(struc...

Re: [kbuild-all] Re: 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(struc...

[Oliver Hartkopp]

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.

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 <lkp@intel.com>
>>>
>>> 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@kernel.org>
>>> :::::: CC: Will Deacon <will@kernel.org>
>>>
>>> ---
>>> 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
> 

Re: [kbuild-all] Re: 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(struc...

[Rong Chen]

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 <lkp@intel.com>
>>>>
>>>> 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@kernel.org>
>>>> :::::: CC: Will Deacon <will@kernel.org>
>>>>
>>>> ---
>>>> 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
>>

Re: [kbuild-all] Re: 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(struc...

[Vincent MAILHOL]

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@intel.com> 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@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?!?

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@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 <lkp@intel.com>
> >>>>
> >>>> 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@kernel.org>
> >>>> :::::: CC: Will Deacon <will@kernel.org>
> >>>>
> >>>> ---
> >>>> 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
> >>
>

Re: [kbuild-all] Re: 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(struc...

[Rong Chen]

Hi Vincent,

On 3/23/21 1:46 PM, Vincent MAILHOL wrote:
> 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@intel.com> 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@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?!?
> 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)))?

Here is the layout without __attribute__((__aligned__(8))),
the union is still extended to 4 bytes:

struct can_frame {
         canid_t                    can_id;               /* 0     4 */
         union {
                 __u8               len;                  /* 4     1 */
                 __u8               can_dlc;              /* 4     1 */
         };                                               /* 4     4 */
         __u8                       __pad;                /* 8     1 */
         __u8                       __res0;               /* 9     1 */
         __u8                       len8_dlc;             /* 10     1 */
         __u8                       data[8];              /* 11     8 */

         /* size: 20, cachelines: 1, members: 6 */
         /* padding: 1 */
         /* last cacheline: 20 bytes */
};

Best Regards,
Rong Chen

>
> (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@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 <lkp@intel.com>
>>>>>>
>>>>>> 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@kernel.org>
>>>>>> :::::: CC: Will Deacon <will@kernel.org>
>>>>>>
>>>>>> ---
>>>>>> 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
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org

Re: [kbuild-all] Re: 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(struc...

[Patrick Menschel]

Am 23.03.21 um 07:06 schrieb Rong Chen:
>>>> 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)))?
> 
> Here is the layout without __attribute__((__aligned__(8))),
> the union is still extended to 4 bytes:
> 
> struct can_frame {
>         canid_t                    can_id;               /* 0     4 */
>         union {
>                 __u8               len;                  /* 4     1 */
>                 __u8               can_dlc;              /* 4     1 */
>         };                                               /* 4     4 */
>         __u8                       __pad;                /* 8     1 */
>         __u8                       __res0;               /* 9     1 */
>         __u8                       len8_dlc;             /* 10     1 */
>         __u8                       data[8];              /* 11     8 */
> 
>         /* size: 20, cachelines: 1, members: 6 */
>         /* padding: 1 */
>         /* last cacheline: 20 bytes */
> };
> 
> Best Regards,
> Rong Chen

Hi,

I would suggest a try with __attribute__((__aligned__(8))) only on
can_frame, not on data[8].

If the structure length is a multiple of 8, the compiler should
recognize this and keep the union a single byte in favor of an array
configuration of that struct.

The __attribute__((__aligned__(8))) on data[8] has strange propagation
effects upwards.

If the attributes are really necessary, I would suggest to have both

__attribute__((packed))
__attribute__((__aligned__(8)))

on structure level instead of inside, so no padding happens inside the
structure while the structure itself is aligned.
Using aligned and packaged inside a structure may be contradictive to
the compiler.

This reminds me of the alignment/gap issue with my python3
implementation of bcm message while alternating between X86_64 and
ARMHF. Using c_types was a mess but bytestrings worked in the end.
Be aware native alignment apparently is 4 on armhf linux and 8 on X86_64
linux.

https://marc.info/?l=linux-can&m=161251622904527&w=2
https://gitlab.com/Menschel/socketcan/-/commit/afc6744129448ae4333629fc0297808dd42e3530#e522710a8423075cfd1147ae6b7f44facac3ffb0_133_132

Best Regards,
Patrick Menschel

Re: [kbuild-all] Re: 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(struc...

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On 23.03.2021 10:54:40, Rong Chen wrote:
> 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.

FWIW:

I'm using latest debian arm compiler and the BUILD_BUG never triggered.
gcc version 10.2.1 20210110 (Debian 10.2.1-6)

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [kbuild-all] Re: 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(struc...

[Oliver Hartkopp]

On 23.03.21 08:34, Marc Kleine-Budde wrote:
> On 23.03.2021 10:54:40, Rong Chen wrote:
>> 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.
> 
> FWIW:
> 
> I'm using latest debian arm compiler and the BUILD_BUG never triggered.
> gcc version 10.2.1 20210110 (Debian 10.2.1-6)
> 

Thanks Marc!

IMO we facing a compiler problem here - and we should be very happy that 
the BUILD_BUG_ON() triggered an issue after years of silence.

I do not have a good feeling about what kind of strange effects this 
compiler issue might have in other code of other projects.

So I would explicitly suggest NOT to change the af_can.c code to work 
around this compiler issue.

Let the gcc people fix their product and let them thank all of us for 
detecting it.

Regards,
Oliver

Re: [kbuild-all] Re: 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(struc...

[Oliver Hartkopp]

Answering myself ...

On 23.03.21 08:45, Oliver Hartkopp wrote:
> On 23.03.21 08:34, Marc Kleine-Budde wrote:
>> On 23.03.2021 10:54:40, Rong Chen wrote:
>>> 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.
>>
>> FWIW:
>>
>> I'm using latest debian arm compiler and the BUILD_BUG never triggered.
>> gcc version 10.2.1 20210110 (Debian 10.2.1-6)
>>

@Rong / Marc:

I wonder if the compiler configurations (gcc -v) or the options used at 
kernel build time are identical.

Maybe there is a different optimization option selected which causes the 
compiler to extend the u8 union to a 32 bit space?!?

And maybe Debian is a bit more conservative in selecting their 
optimizations than the setup that Rong was using for the build ...

Best,
Oliver

> 
> Thanks Marc!
> 
> IMO we facing a compiler problem here - and we should be very happy that 
> the BUILD_BUG_ON() triggered an issue after years of silence.
> 
> I do not have a good feeling about what kind of strange effects this 
> compiler issue might have in other code of other projects.
> 
> So I would explicitly suggest NOT to change the af_can.c code to work 
> around this compiler issue.
> 
> Let the gcc people fix their product and let them thank all of us for 
> detecting it.
> 
> Regards,
> Oliver

Re: [kbuild-all] Re: 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(struc...

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On 23.03.2021 09:32:10, Oliver Hartkopp wrote:
> I wonder if the compiler configurations (gcc -v) or the options used at
> kernel build time are identical.

I tested several compilers and with my .config never triggered a
problem, but with Rong Chen it does. I'm trying to figure out which
option it is, stay tuned.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [kbuild-all] Re: 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(struc...

[Rong Chen]

On 3/23/21 4:54 PM, Marc Kleine-Budde wrote:
> On 23.03.2021 09:32:10, Oliver Hartkopp wrote:
>> I wonder if the compiler configurations (gcc -v) or the options used at
>> kernel build time are identical.
> I tested several compilers and with my .config never triggered a
> problem, but with Rong Chen it does. I'm trying to figure out which
> option it is, stay tuned.
>
> Marc
>

Hi Marc, Oliver,

We use the below cross compiler:

https://download.01.org/0day-ci/cross-package/gcc-9.3.0-nolibc/x86_64-gcc-9.3.0-nolibc_arm-linux-gnueabi.tar.xz

and here is the detail:

$ 
/home/nfs/0day/gcc-9.3.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc 
-v
Using built-in specs.
COLLECT_GCC=/home/nfs/0day/gcc-9.3.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc
COLLECT_LTO_WRAPPER=/home/nfs/0day/gcc-9.3.0-nolibc/arm-linux-gnueabi/bin/../libexec/gcc/arm-linux-gnueabi/9.3.0/lto-wrapper
Target: arm-linux-gnueabi
Configured with: /tmp/build-crosstools-xh/gcc/gcc-9.3.0/configure 
--target=arm-linux-gnueabi --enable-targets=all 
--prefix=/tmp/build-crosstools-xh/cross --enable-languages=c 
--without-headers --disable-bootstrap --disable-nls --disable-threads 
--disable-shared --disable-libmudflap --disable-libssp --disable-libgomp 
--disable-decimal-float --disable-libquadmath --disable-libatomic 
--disable-libcc1 --disable-libmpx --enable-checking=release
Thread model: single
gcc version 9.3.0 (GCC)

Best Regards,
Rong Chen

Re: [kbuild-all] Re: 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(struc...

[Rasmus Villemoes]

On 23/03/2021 08.45, Oliver Hartkopp wrote:

> IMO we facing a compiler problem here - and we should be very happy that
> the BUILD_BUG_ON() triggered an issue after years of silence.
> 
> I do not have a good feeling about what kind of strange effects this
> compiler issue might have in other code of other projects.
> 
> So I would explicitly suggest NOT to change the af_can.c code to work
> around this compiler issue.
> 
> Let the gcc people fix their product and let them thank all of us for
> detecting it.

I'm sure you'd be eligible for a full refund in case this was a bug in
gcc. It is not. It's a pretty clear ABI requirement for (at least some
flavors of) ARM:

https://stackoverflow.com/questions/43786747/struct-layout-in-apcs-gnu-abi

and more directly from the horse's mouth:

https://developer.arm.com/documentation/dui0067/d/arm-compiler-reference/c-and-c---implementation-details/structures--unions--enumerations--and-bitfields

Field alignment

    Structures are arranged with the first-named component at the lowest
address. Fields are aligned as follows:

        A field with a char type is aligned to the next available byte.

        A field with a short type is aligned to the next even-addressed
byte.

        Bitfield alignment depends on how the bitfield is declared. See
Bitfields in packed structures for more information.

        All other types are aligned on word boundaries.

That anonymous union falls into the "All other types" bullet.

__packed is the documented and standard way to overrule the
compiler's/ABI's layout decisions.

Rasmus

Re: [kbuild-all] Re: 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(struc...

[Oliver Hartkopp]

On 23.03.21 12:36, Rasmus Villemoes wrote:
> On 23/03/2021 08.45, Oliver Hartkopp wrote:
> 
>> IMO we facing a compiler problem here - and we should be very happy that
>> the BUILD_BUG_ON() triggered an issue after years of silence.
>>
>> I do not have a good feeling about what kind of strange effects this
>> compiler issue might have in other code of other projects.
>>
>> So I would explicitly suggest NOT to change the af_can.c code to work
>> around this compiler issue.
>>
>> Let the gcc people fix their product and let them thank all of us for
>> detecting it.
> 
> I'm sure you'd be eligible for a full refund in case this was a bug in
> gcc. It is not. It's a pretty clear ABI requirement for (at least some
> flavors of) ARM:
> 
> https://stackoverflow.com/questions/43786747/struct-layout-in-apcs-gnu-abi
> 
> and more directly from the horse's mouth:
> 
> https://developer.arm.com/documentation/dui0067/d/arm-compiler-reference/c-and-c---implementation-details/structures--unions--enumerations--and-bitfields
> 
> Field alignment
> 
>      Structures are arranged with the first-named component at the lowest
> address. Fields are aligned as follows:
> 
>          A field with a char type is aligned to the next available byte.
> 
>          A field with a short type is aligned to the next even-addressed
> byte.
> 
>          Bitfield alignment depends on how the bitfield is declared. See
> Bitfields in packed structures for more information.
> 
>          All other types are aligned on word boundaries.
> 
> That anonymous union falls into the "All other types" bullet.
> 
> __packed is the documented and standard way to overrule the
> compiler's/ABI's layout decisions.

So why is there a difference between

gcc version 10.2.0

and

gcc version 10.2.1 20210110 (Debian 10.2.1-6)

https://lore.kernel.org/linux-can/20210323073437.yo63wreqnubbeqby@pengutronix.de/

??

Would this mean that either STRUCTURE_SIZE_BOUNDARY or the command line 
option -mstructure_size_boundary=<n>

are set differently?

https://stackoverflow.com/questions/43786747/struct-layout-in-apcs-gnu-abi/43829053#43829053

I'm not a compiler expert but this does not seem to be consistent.

Especially as we only have byte sizes (inside and outside of the union) 
and "A field with a char type is aligned to the next available byte."

The union is indeed aligned to the word boundary - but the following 
byte is not aligned to the next available byte.

Regards,
Oliver

Re: [kbuild-all] Re: 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(struc...

[Rasmus Villemoes]

On 23/03/2021 13.49, Oliver Hartkopp wrote:
> 
> 
> On 23.03.21 12:36, Rasmus Villemoes wrote:
>>
>> and more directly from the horse's mouth:
>>
>> https://developer.arm.com/documentation/dui0067/d/arm-compiler-reference/c-and-c---implementation-details/structures--unions--enumerations--and-bitfields
>>
>>
>> Field alignment
>>
>>      Structures are arranged with the first-named component at the lowest
>> address. Fields are aligned as follows:
>>
>>          A field with a char type is aligned to the next available byte.
>>
>>          A field with a short type is aligned to the next even-addressed
>> byte.
>>
>>          Bitfield alignment depends on how the bitfield is declared. See
>> Bitfields in packed structures for more information.
>>
>>          All other types are aligned on word boundaries.
>>
>> That anonymous union falls into the "All other types" bullet.
>>
>> __packed is the documented and standard way to overrule the
>> compiler's/ABI's layout decisions.
> 
> So why is there a difference between
> 
> gcc version 10.2.0
> 
> and
> 
> gcc version 10.2.1 20210110 (Debian 10.2.1-6)

I'm guessing there's no difference between those (in this respect), but
they are invoked differently.

> Would this mean that either STRUCTURE_SIZE_BOUNDARY or the command line
> option -mstructure_size_boundary=<n>
> 
> are set differently?

Yes, though very likely -mstructure_size_boundary is not set explicitly
but via some other option.

gcc has a rather helpful but almost unknown feature that one can
actually query for lots of different parameters and their
default/current values. So on my Ubuntu system (20.04, gcc 9.3), for
example, if I do

$ arm-linux-gnueabihf-gcc -O2 -Q --help=target | grep struct
  -mstructure-size-boundary=            8

So that would seem to say that the union should work as expected.
However, when I actually try to compile with the .config that kbuild
reports failing, I do see that BUILD_BUG_ON triggering.

So let us inspect the actual command line used to build some other
random .o file in net/can; look at net/can/.bcm.o.cmd

cmd_net/can/bcm.o := arm-linux-gnueabihf-gcc -Wp,-MMD,net/can/.bcm.o.d
-nostdinc -isystem /usr/lib/gcc-cross/arm-linux-gnueabihf/9/include
-I./arch/arm/include -I./arch/arm/include/generated  -I./include
-I./arch/arm/include/uapi -I./arch/arm/include/generated/uapi
-I./include/uapi -I./include/generated/uapi -include
./include/linux/compiler-version.h -include ./include/linux/kconfig.h
-include ./include/linux/compiler_types.h -D__KERNEL__ -mlittle-endian
-I./arch/arm/mach-footbridge/include -fmacro-prefix-map=./= -Wall
-Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing
-fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration
-Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu89
-fno-dwarf2-cfi-asm -mno-unaligned-access -fno-omit-frame-pointer -mapcs
-mno-sched-prolog -fno-ipa-sra -mabi=apcs-gnu -mno-thumb-interwork -marm
-Wa,-mno-warn-deprecated -D__LINUX_ARM_ARCH__=4 -march=armv4
-mtune=strongarm110 -msoft-float -Uarm -fno-delete-null-pointer-checks
-Wno-frame-address -Wno-format-truncation -Wno-format-overflow
-Wno-address-of-packed-member -O2 --param=allow-store-data-races=0
-Wframe-larger-than=1024 -fno-stack-protector
-Wno-unused-but-set-variable -Wimplicit-fallthrough
-Wno-unused-const-variable -fno-omit-frame-pointer
-fno-optimize-sibling-calls -fno-inline-functions-called-once
-Wdeclaration-after-statement -Wvla -Wno-pointer-sign
-Wno-stringop-truncation -Wno-array-bounds -Wno-stringop-overflow
-Wno-restrict -Wno-maybe-uninitialized -fno-strict-overflow
-fno-stack-check -fconserve-stack -Werror=date-time
-Werror=incompatible-pointer-types -Werror=designated-init
-Wno-packed-not-aligned    -fsanitize-coverage=trace-pc
-DKBUILD_MODFILE='"net/can/can-bcm"' -DKBUILD_BASENAME='"bcm"'
-DKBUILD_MODNAME='"can_bcm"' -D__KBUILD_MODNAME=kmod_can_bcm -c -o
net/can/bcm.o net/can/bcm.c

Lots of gunk. But just to see if one of those options have affected the
-mstructure-size-boundary= value, just take that whole command line and
throw in -Q --help=target at the end, and we get

  -mstructure-size-boundary=            32

So let us guess that it's the ABI choice -mabi=apcs-gnu

$ arm-linux-gnueabihf-gcc -O2 -msoft-float -mabi=apcs-gnu -Q
--help=target | grep struct
  -mstructure-size-boundary=            32

Bingo. (-msoft-float is also included just as in the real command line
because gcc barfs otherwise).

Now what CONFIG_* knobs are responsible for putting -mabi=apcs-gnu in
CFLAGS is left as an exercise for the reader. Regardless, it is not a
bug in the compiler. The error is the assumption that this language

"Aggregates and Unions

Structures and unions assume the alignment of their most strictly
aligned component.
Each member is assigned to the lowest available offset with the appropriate
alignment. The size of any object is always a multiple of the object‘s
alignment."

from the x86-64 ABI applies on all other architectures/ABIs.

> I'm not a compiler expert but this does not seem to be consistent.
> 
> Especially as we only have byte sizes (inside and outside of the union)
> and "A field with a char type is aligned to the next available byte."

Yes, and that's exactly what you got before the anon union was introduced.

> The union is indeed aligned to the word boundary - but the following
> byte is not aligned to the next available byte.

Yes it is, because the union occupies 4 bytes. The first byte is shared
by the two char members, the remaining three bytes are padding.

Rasmus

Re: [kbuild-all] Re: 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(struc...

[Oliver Hartkopp]

On 23.03.21 15:00, Rasmus Villemoes wrote:
> On 23/03/2021 13.49, Oliver Hartkopp wrote:
>>
>>
>> On 23.03.21 12:36, Rasmus Villemoes wrote:
>>>
>>> and more directly from the horse's mouth:
>>>
>>> https://developer.arm.com/documentation/dui0067/d/arm-compiler-reference/c-and-c---implementation-details/structures--unions--enumerations--and-bitfields
>>>
>>>
>>> Field alignment
>>>
>>>       Structures are arranged with the first-named component at the lowest
>>> address. Fields are aligned as follows:
>>>
>>>           A field with a char type is aligned to the next available byte.
>>>
>>>           A field with a short type is aligned to the next even-addressed
>>> byte.
>>>
>>>           Bitfield alignment depends on how the bitfield is declared. See
>>> Bitfields in packed structures for more information.
>>>
>>>           All other types are aligned on word boundaries.
>>>
>>> That anonymous union falls into the "All other types" bullet.
>>>
>>> __packed is the documented and standard way to overrule the
>>> compiler's/ABI's layout decisions.
>>
>> So why is there a difference between
>>
>> gcc version 10.2.0
>>
>> and
>>
>> gcc version 10.2.1 20210110 (Debian 10.2.1-6)
> 
> I'm guessing there's no difference between those (in this respect), but
> they are invoked differently.
> 
>> Would this mean that either STRUCTURE_SIZE_BOUNDARY or the command line
>> option -mstructure_size_boundary=<n>
>>
>> are set differently?
> 
> Yes, though very likely -mstructure_size_boundary is not set explicitly
> but via some other option.
> 
> gcc has a rather helpful but almost unknown feature that one can
> actually query for lots of different parameters and their
> default/current values. So on my Ubuntu system (20.04, gcc 9.3), for
> example, if I do
> 
> $ arm-linux-gnueabihf-gcc -O2 -Q --help=target | grep struct
>    -mstructure-size-boundary=            8
> 
> So that would seem to say that the union should work as expected.
> However, when I actually try to compile with the .config that kbuild
> reports failing, I do see that BUILD_BUG_ON triggering.
> 
> So let us inspect the actual command line used to build some other
> random .o file in net/can; look at net/can/.bcm.o.cmd
> 
> cmd_net/can/bcm.o := arm-linux-gnueabihf-gcc -Wp,-MMD,net/can/.bcm.o.d
> -nostdinc -isystem /usr/lib/gcc-cross/arm-linux-gnueabihf/9/include
> -I./arch/arm/include -I./arch/arm/include/generated  -I./include
> -I./arch/arm/include/uapi -I./arch/arm/include/generated/uapi
> -I./include/uapi -I./include/generated/uapi -include
> ./include/linux/compiler-version.h -include ./include/linux/kconfig.h
> -include ./include/linux/compiler_types.h -D__KERNEL__ -mlittle-endian
> -I./arch/arm/mach-footbridge/include -fmacro-prefix-map=./= -Wall
> -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing
> -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration
> -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu89
> -fno-dwarf2-cfi-asm -mno-unaligned-access -fno-omit-frame-pointer -mapcs
> -mno-sched-prolog -fno-ipa-sra -mabi=apcs-gnu -mno-thumb-interwork -marm
> -Wa,-mno-warn-deprecated -D__LINUX_ARM_ARCH__=4 -march=armv4
> -mtune=strongarm110 -msoft-float -Uarm -fno-delete-null-pointer-checks
> -Wno-frame-address -Wno-format-truncation -Wno-format-overflow
> -Wno-address-of-packed-member -O2 --param=allow-store-data-races=0
> -Wframe-larger-than=1024 -fno-stack-protector
> -Wno-unused-but-set-variable -Wimplicit-fallthrough
> -Wno-unused-const-variable -fno-omit-frame-pointer
> -fno-optimize-sibling-calls -fno-inline-functions-called-once
> -Wdeclaration-after-statement -Wvla -Wno-pointer-sign
> -Wno-stringop-truncation -Wno-array-bounds -Wno-stringop-overflow
> -Wno-restrict -Wno-maybe-uninitialized -fno-strict-overflow
> -fno-stack-check -fconserve-stack -Werror=date-time
> -Werror=incompatible-pointer-types -Werror=designated-init
> -Wno-packed-not-aligned    -fsanitize-coverage=trace-pc
> -DKBUILD_MODFILE='"net/can/can-bcm"' -DKBUILD_BASENAME='"bcm"'
> -DKBUILD_MODNAME='"can_bcm"' -D__KBUILD_MODNAME=kmod_can_bcm -c -o
> net/can/bcm.o net/can/bcm.c
> 
> Lots of gunk. But just to see if one of those options have affected the
> -mstructure-size-boundary= value, just take that whole command line and
> throw in -Q --help=target at the end, and we get
> 
>    -mstructure-size-boundary=            32
> 
> So let us guess that it's the ABI choice -mabi=apcs-gnu
> 
> $ arm-linux-gnueabihf-gcc -O2 -msoft-float -mabi=apcs-gnu -Q
> --help=target | grep struct
>    -mstructure-size-boundary=            32
> 
> Bingo. (-msoft-float is also included just as in the real command line
> because gcc barfs otherwise).

Thanks for all the comprehensive explanations!

> Now what CONFIG_* knobs are responsible for putting -mabi=apcs-gnu in
> CFLAGS is left as an exercise for the reader. Regardless, it is not a
> bug in the compiler. The error is the assumption that this language
> 
> "Aggregates and Unions
> 
> Structures and unions assume the alignment of their most strictly
> aligned component.

(parse error in sentence)

> Each member is assigned to the lowest available offset with the appropriate
> alignment. The size of any object is always a multiple of the object‘s
> alignment."
> 
> from the x86-64 ABI applies on all other architectures/ABIs.
> 
>> I'm not a compiler expert but this does not seem to be consistent.
>>
>> Especially as we only have byte sizes (inside and outside of the union)
>> and "A field with a char type is aligned to the next available byte."
> 
> Yes, and that's exactly what you got before the anon union was introduced.

Before(!) the union there is nothing to pad.

>> The union is indeed aligned to the word boundary - but the following
>> byte is not aligned to the next available byte.
> 
> Yes it is, because the union occupies 4 bytes. The first byte is shared
> by the two char members, the remaining three bytes are padding.

But why is the union 4 bytes long here and adds a padding of three bytes 
at the end? IMO this is an error.

Thanks for your patience,
Oliver

Re: [kbuild-all] Re: 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(struc...

[Rasmus Villemoes]

On 23/03/2021 19.59, Oliver Hartkopp wrote:
> 
> 
> On 23.03.21 15:00, Rasmus Villemoes wrote:

>> Now what CONFIG_* knobs are responsible for putting -mabi=apcs-gnu in
>> CFLAGS is left as an exercise for the reader. Regardless, it is not a
>> bug in the compiler. The error is the assumption that this language
>>
>> "Aggregates and Unions
>>
>> Structures and unions assume the alignment of their most strictly
>> aligned component.
> 
> (parse error in sentence)

It was a direct quote, but I can try to paraphrase with an example. If
you have a struct foo { T1 m1; T2 m2; T3 m3; }, then alignof(struct foo)
= max(alignof(T1), alignof(T2), alignof(T3)). Same for a "union foo".

But this is specifically for x86-64; for (some flavors of) ARM, other
rules apply - namely, alignof(T) is 4 unless T is char or short (or
(un)signed variants), ignoring bitfields which have their own rules.
Note that while

union u {char a; char b;}

has alignment 4 on ARM and 1 on x86-64, other types are less strictly
aligned on ARM; e.g. s64 aka long long is 8-byte aligned on x86-64 but
(still) just 4-byte aligned on ARM. And again, this is just for specific
-mabi= options.

>> Each member is assigned to the lowest available offset with the
>> appropriate
>> alignment. The size of any object is always a multiple of the object‘s
>> alignment."
>>
>> from the x86-64 ABI applies on all other architectures/ABIs.
>>
>>> I'm not a compiler expert but this does not seem to be consistent.
>>>
>>> Especially as we only have byte sizes (inside and outside of the union)
>>> and "A field with a char type is aligned to the next available byte."
>>
>> Yes, and that's exactly what you got before the anon union was
>> introduced.
> 
> Before(!) the union there is nothing to pad.

Just to be clear, my "before" was in the temporal sense, i.e. "prior to
commit ea7800565a128", all the u8s in struct can_frame were placed one
after the other. But after that commit, struct can_frame has a new
member replacing can_dlc which happens to occupy 4 bytes (for some
ABIs), pushing the subsequent members __pad, __res0 and len8_dlc
(formerly known as __res1) ahead.

>>> The union is indeed aligned to the word boundary - but the following
>>> byte is not aligned to the next available byte.
>>
>> Yes it is, because the union occupies 4 bytes. The first byte is shared
>> by the two char members, the remaining three bytes are padding.
> 
> But why is the union 4 bytes long here and adds a padding of three bytes
> at the end?

Essentially, because arrays. It's true for _any_ type T that sizeof(T)
must be a multiple of alignof(T). Take an array "T x[9]". If x[0] is
4-byte aligned, then in order for x[1] to be 4-byte aligned as well,
x[0] must occupy a multiple of 4 bytes.

It doesn't matter at all that this happens to be an anonymous union.
Layout-wise, you could as well have a definition

union uuu { __u8 len; __u8 can_dlc; }

and made struct can_frame

struct can_frame {
   canid_t can_id;
   union uuu u;
   __u8 __pad;
   ...
};

(you lose the anonymity trick so you'd have to do frame->u.can_dlc
instead of just frame->can_dlc). You have a member with alignof()==4 and
 sizeof()==4; that sizeof() cannot magically become 1 just because that
particular instance of the type is not part of an array. Imagine what
would happen if the compiler pulled subsequent char members into
trailing padding of a previous compound member. E.g. consider

struct a { int x; char y; } // alignof==4, sizeof==8, offsetof(y)==4
struct b { struct a a; char z; }

If I have a "struct b *b", I'm allowed to do "&b->a" and get a "pointer
to struct a". Then I can do memset(&b->a, 0, sizeof(struct a)). Clearly,
z must not have been placed inside the trailing padding of struct a.

Rasmus

Re: [kbuild-all] Re: 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(struc...

[Oliver Hartkopp]

On 23.03.21 21:54, Rasmus Villemoes wrote:
> On 23/03/2021 19.59, Oliver Hartkopp wrote:
>>
>>
>> On 23.03.21 15:00, Rasmus Villemoes wrote:
> 
>>> Now what CONFIG_* knobs are responsible for putting -mabi=apcs-gnu in
>>> CFLAGS is left as an exercise for the reader. Regardless, it is not a
>>> bug in the compiler. The error is the assumption that this language
>>>
>>> "Aggregates and Unions
>>>
>>> Structures and unions assume the alignment of their most strictly
>>> aligned component.
>>
>> (parse error in sentence)
> 
> It was a direct quote, but I can try to paraphrase with an example. If
> you have a struct foo { T1 m1; T2 m2; T3 m3; }, then alignof(struct foo)
> = max(alignof(T1), alignof(T2), alignof(T3)). Same for a "union foo".
> 
> But this is specifically for x86-64; for (some flavors of) ARM, other
> rules apply - namely, alignof(T) is 4 unless T is char or short (or
> (un)signed variants), ignoring bitfields which have their own rules.
> Note that while
> 
> union u {char a; char b;}
> 
> has alignment 4 on ARM and 1 on x86-64, other types are less strictly
> aligned on ARM; e.g. s64 aka long long is 8-byte aligned on x86-64 but
> (still) just 4-byte aligned on ARM. And again, this is just for specific
> -mabi= options.
> 
>>> Each member is assigned to the lowest available offset with the
>>> appropriate
>>> alignment. The size of any object is always a multiple of the object‘s
>>> alignment."
>>>
>>> from the x86-64 ABI applies on all other architectures/ABIs.
>>>
>>>> I'm not a compiler expert but this does not seem to be consistent.
>>>>
>>>> Especially as we only have byte sizes (inside and outside of the union)
>>>> and "A field with a char type is aligned to the next available byte."
>>>
>>> Yes, and that's exactly what you got before the anon union was
>>> introduced.
>>
>> Before(!) the union there is nothing to pad.
> 
> Just to be clear, my "before" was in the temporal sense, i.e. "prior to
> commit ea7800565a128", all the u8s in struct can_frame were placed one
> after the other. But after that commit, struct can_frame has a new
> member replacing can_dlc which happens to occupy 4 bytes (for some
> ABIs), pushing the subsequent members __pad, __res0 and len8_dlc
> (formerly known as __res1) ahead.
> 
>>>> The union is indeed aligned to the word boundary - but the following
>>>> byte is not aligned to the next available byte.
>>>
>>> Yes it is, because the union occupies 4 bytes. The first byte is shared
>>> by the two char members, the remaining three bytes are padding.
>>
>> But why is the union 4 bytes long here and adds a padding of three bytes
>> at the end?
> 
> Essentially, because arrays. It's true for _any_ type T that sizeof(T)
> must be a multiple of alignof(T). Take an array "T x[9]". If x[0] is
> 4-byte aligned, then in order for x[1] to be 4-byte aligned as well,
> x[0] must occupy a multiple of 4 bytes.
> 
> It doesn't matter at all that this happens to be an anonymous union.
> Layout-wise, you could as well have a definition
> 
> union uuu { __u8 len; __u8 can_dlc; }
> 
> and made struct can_frame
> 
> struct can_frame {
>     canid_t can_id;
>     union uuu u;
>     __u8 __pad;
>     ...
> };
> 
> (you lose the anonymity trick so you'd have to do frame->u.can_dlc
> instead of just frame->can_dlc). You have a member with alignof()==4 and
>   sizeof()==4; that sizeof() cannot magically become 1 just because that
> particular instance of the type is not part of an array. Imagine what
> would happen if the compiler pulled subsequent char members into
> trailing padding of a previous compound member. E.g. consider
> 
> struct a { int x; char y; } // alignof==4, sizeof==8, offsetof(y)==4
> struct b { struct a a; char z; }
> 
> If I have a "struct b *b", I'm allowed to do "&b->a" and get a "pointer
> to struct a". Then I can do memset(&b->a, 0, sizeof(struct a)). Clearly,
> z must not have been placed inside the trailing padding of struct a.
> 
> Rasmus
> 

Thanks Rasmus!

@Marc: Looks like we can not get around the __packed() fix :-(

At least we now have some more documentation to be referenced and I 
would suggest to point out that some compilers handle the union 
alignment like this.

To make clear in the comments what we are suppressing here any why.

Many thanks,
Oliver

Re: [kbuild-all] Re: 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(struc...

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On 24.03.2021 10:09:22, Oliver Hartkopp wrote:
> @Marc: Looks like we can not get around the __packed() fix :-(
> 
> At least we now have some more documentation to be referenced and I would
> suggest to point out that some compilers handle the union alignment like
> this.

It's not the compiler, but the ABI.

> To make clear in the comments what we are suppressing here any why.

Feel free to post an updated patch description.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [kbuild-all] Re: 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(struc...

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

On 23.03.2021 15:00:55, Rasmus Villemoes wrote:
[...]
> So let us guess that it's the ABI choice -mabi=apcs-gnu
> 
> $ arm-linux-gnueabihf-gcc -O2 -msoft-float -mabi=apcs-gnu -Q
> --help=target | grep struct
>   -mstructure-size-boundary=            32
> 
> Bingo. (-msoft-float is also included just as in the real command line
> because gcc barfs otherwise).
> 
> Now what CONFIG_* knobs are responsible for putting -mabi=apcs-gnu in
> CFLAGS is left as an exercise for the reader. Regardless, it is not a
> bug in the compiler. The error is the assumption that this language

For the record:

If CONFIG_AEABI is not set "-mabi=apcs-gnu" is used, which leads to the
bigger structure size boundary.

| ifeq ($(CONFIG_AEABI),y)
| CFLAGS_ABI      :=-mabi=aapcs-linux -mfpu=vfp
| else
| CFLAGS_ABI      :=$(call cc-option,-mapcs-32,-mabi=apcs-gnu) $(call cc-option,-mno-thumb-interwork,)
| endif

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]