[PATCH] x86/fpu: use __alignof__ to avoid UB in TYPE_ALIGN

[PATCH] x86/fpu: use __alignof__ to avoid UB in TYPE_ALIGN

[YingChi Long]

WG14 N2350 made very clear that it is an UB having type definitions with
in "offsetof". This patch change the implementation of macro
"TYPE_ALIGN" to builtin "__alignof__" to avoid undefined behavior.

I've grepped all source files to find any type definitions within
"offsetof".

    offsetof\(struct .*\{ .*,

This implementation of macro "TYPE_ALIGN" seemes to be the only case of
type definitions within offsetof in the kernel codebase.

Signed-off-by: YingChi Long <me@inclyc.cn>
Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
---
 arch/x86/kernel/fpu/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 621f4b6cac4a..41425ba0b6b1 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -134,7 +134,7 @@ static void __init fpu__init_system_generic(void)
 }
 
 /* Get alignment of the TYPE. */
-#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
+#define TYPE_ALIGN(TYPE) __alignof__(TYPE)
 
 /*
  * Enforce that 'MEMBER' is the last field of 'TYPE'.
-- 
2.35.1

Re: [PATCH] x86/fpu: use __alignof__ to avoid UB in TYPE_ALIGN

[Peter Zijlstra]

On Sun, Sep 25, 2022 at 11:31:50PM +0800, YingChi Long wrote:
> WG14 N2350 made very clear that it is an UB having type definitions with
> in "offsetof". This patch change the implementation of macro
> "TYPE_ALIGN" to builtin "__alignof__" to avoid undefined behavior.
> 
> I've grepped all source files to find any type definitions within
> "offsetof".
> 
>     offsetof\(struct .*\{ .*,
> 
> This implementation of macro "TYPE_ALIGN" seemes to be the only case of
> type definitions within offsetof in the kernel codebase.
> 
> Signed-off-by: YingChi Long <me@inclyc.cn>
> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> ---
>  arch/x86/kernel/fpu/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 621f4b6cac4a..41425ba0b6b1 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -134,7 +134,7 @@ static void __init fpu__init_system_generic(void)
>  }
>  
>  /* Get alignment of the TYPE. */
> -#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
> +#define TYPE_ALIGN(TYPE) __alignof__(TYPE)

IIRC there's a problem with alignof() in that it will return the ABI
alignment instead of that preferred or natural alignment for some types.

Notably I think 'long long' has 4 byte alignment on i386 and some other
32bit archs.

That said; please just replace the *one* instance of TYPE_ALIGN entirely
and get rid of the thing.

Re: [PATCH] x86/fpu: use __alignof__ to avoid UB in TYPE_ALIGN

[YingChi Long]

Seems GCC __alignof__ is not evaluated to the minimum alignment of some 
TYPE,
and depends on fields of the struct.

 > Notably I think 'long long' has 4 byte alignment on i386 and some other
 > 32bit archs.

C11 _Alignof matches in the case (see godbolt link below). How about 
switch to
_Alignof?


Link: https://godbolt.org/z/T749MfM9o
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10360
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52023
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69560

On 2022/9/26 17:01, Peter Zijlstra wrote:
> On Sun, Sep 25, 2022 at 11:31:50PM +0800, YingChi Long wrote:
>> WG14 N2350 made very clear that it is an UB having type definitions with
>> in "offsetof". This patch change the implementation of macro
>> "TYPE_ALIGN" to builtin "__alignof__" to avoid undefined behavior.
>>
>> I've grepped all source files to find any type definitions within
>> "offsetof".
>>
>>      offsetof\(struct .*\{ .*,
>>
>> This implementation of macro "TYPE_ALIGN" seemes to be the only case of
>> type definitions within offsetof in the kernel codebase.
>>
>> Signed-off-by: YingChi Long <me@inclyc.cn>
>> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
>> ---
>>   arch/x86/kernel/fpu/init.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
>> index 621f4b6cac4a..41425ba0b6b1 100644
>> --- a/arch/x86/kernel/fpu/init.c
>> +++ b/arch/x86/kernel/fpu/init.c
>> @@ -134,7 +134,7 @@ static void __init fpu__init_system_generic(void)
>>   }
>>   
>>   /* Get alignment of the TYPE. */
>> -#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
>> +#define TYPE_ALIGN(TYPE) __alignof__(TYPE)
> IIRC there's a problem with alignof() in that it will return the ABI
> alignment instead of that preferred or natural alignment for some types.
>
> Notably I think 'long long' has 4 byte alignment on i386 and some other
> 32bit archs.
>
> That said; please just replace the *one* instance of TYPE_ALIGN entirely
> and get rid of the thing.
>

Re: [PATCH] x86/fpu: use __alignof__ to avoid UB in TYPE_ALIGN

[Nick Desaulniers]

+Jiri

Hi YingChi,
Thank you very much for the patch and your consideration when
implementing this check in clang.

It looks like you sent a few different versions of this patch; please
use `-v2`, `-v3`, etc. when invoking `git format-patch` to include the
patch version in the subject line.

On Mon, Sep 26, 2022 at 6:19 AM YingChi Long <me@inclyc.cn> wrote:
>
> Seems GCC __alignof__ is not evaluated to the minimum alignment of some
> TYPE,
> and depends on fields of the struct.
>
>  > Notably I think 'long long' has 4 byte alignment on i386 and some other
>  > 32bit archs.
>
> C11 _Alignof matches in the case (see godbolt link below). How about
> switch to
> _Alignof?
>
>
> Link: https://godbolt.org/z/T749MfM9o
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10360
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52023
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69560

I think you should additionally include the following 2 link tags:

Link: https://reviews.llvm.org/D133574
Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html

While Peter is right that there is a subtle distinction between GNU
__alignof__ and ISO C11 _Alignof, reading
commit 25ec02f2c144 ("x86/fpu: Properly align size in
CHECK_MEMBER_AT_END_OF() macro")
wasn't the intent of 25ec02f2c144 to account for alignment of members
within structs? Hence shouldn't we be using __alignof__ and not
_Alignof? (If I've understood all those GCC bug report comments
correctly; will reread them again after lunch).

$ ARCH=i386 make LLVM=1 -j$(nproc) defconfig all
$ ARCH=i386 make -j$(nproc) defconfig all
$ make LLVM=1 -j$(nproc) defconfig all
$ make -j$(nproc) defconfig all

will all build either way (with __alignof__ vs _Alignof).  The comment
in fpu__init_task_struct_size() in arch/x86/kernel/fpu/init.c alludes
to struct fpu being dynamically sized; perhaps on certain kernel
configs which would be needed to tease out potential build failures.

Also, commit messages on other versions state:

>> _alignof__() will in fact return the 'sane' result

Please use more descriptive language rather than 'sane.' That
statement tells readers nothing about the distinctions between
__alignof__ and _Alignof.

Finally, I wonder if it's possible to use static_assert (defined in
include/linux/build_bug.h) here rather than BUILD_BUG_ON?

>
> On 2022/9/26 17:01, Peter Zijlstra wrote:
> > On Sun, Sep 25, 2022 at 11:31:50PM +0800, YingChi Long wrote:
> >> WG14 N2350 made very clear that it is an UB having type definitions with
> >> in "offsetof". This patch change the implementation of macro
> >> "TYPE_ALIGN" to builtin "__alignof__" to avoid undefined behavior.
> >>
> >> I've grepped all source files to find any type definitions within
> >> "offsetof".
> >>
> >>      offsetof\(struct .*\{ .*,
> >>
> >> This implementation of macro "TYPE_ALIGN" seemes to be the only case of
> >> type definitions within offsetof in the kernel codebase.
> >>
> >> Signed-off-by: YingChi Long <me@inclyc.cn>
> >> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> >> ---
> >>   arch/x86/kernel/fpu/init.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> >> index 621f4b6cac4a..41425ba0b6b1 100644
> >> --- a/arch/x86/kernel/fpu/init.c
> >> +++ b/arch/x86/kernel/fpu/init.c
> >> @@ -134,7 +134,7 @@ static void __init fpu__init_system_generic(void)
> >>   }
> >>
> >>   /* Get alignment of the TYPE. */
> >> -#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
> >> +#define TYPE_ALIGN(TYPE) __alignof__(TYPE)
> > IIRC there's a problem with alignof() in that it will return the ABI
> > alignment instead of that preferred or natural alignment for some types.
> >
> > Notably I think 'long long' has 4 byte alignment on i386 and some other
> > 32bit archs.
> >
> > That said; please just replace the *one* instance of TYPE_ALIGN entirely
> > and get rid of the thing.
> >



-- 
Thanks,
~Nick Desaulniers

RE: [PATCH] x86/fpu: use __alignof__ to avoid UB in TYPE_ALIGN

[David Laight]

From: YingChi Long
> Sent: 26 September 2022 14:18
> 
> Seems GCC __alignof__ is not evaluated to the minimum alignment of some
> TYPE, and depends on fields of the struct.
> 
>  > Notably I think 'long long' has 4 byte alignment on i386 and some other
>  > 32bit archs.
> 
> C11 _Alignof matches in the case (see godbolt link below). How about
> switch to _Alignof?

__alignof__() returns the preferred alignment, not the minimal one.
So one i386 it returns 8 for 'long long' rather than 4.

Using alignof(struct{long long x;}) does give the required 4.
(as does _Alignof()).

See https://godbolt.org/z/13xTYYd11

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

WG14 N2350 made very clear that it is an UB having type definitions with
in "offsetof". This patch change the implementation of macro
"TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.

I've grepped all source files to find any type definitions within
"offsetof".

    offsetof\(struct .*\{ .*,

This implementation of macro "TYPE_ALIGN" seemes to be the only case of
type definitions within offsetof in the kernel codebase.

I've made a clang patch that rejects any definitions within
__builtin_offsetof (usually #defined with "offsetof"), and tested
compiling with this patch, there is no error if this patch is applied.

In PATCH v1 "TYPE_ALIGN" was substituted with "__alignof__" which is a
GCC extension, which returns the *preferred alignment*, that is
different from C11 "_Alignof" returning *ABI alignment*. For example, on
i386 __alignof__(long long) evaluates to 8 but _Alignof(long long)
evaluates to 4. See godbolt links below.

In this patch, I'd like to use "__alignof__" to "_Alignof" to preserve
the behavior here.

Signed-off-by: YingChi Long <me@inclyc.cn>
Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
Link: https://godbolt.org/z/13xTYYd11
Link: https://godbolt.org/z/T749MfM9o
Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10360
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52023
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69560
Link: https://reviews.llvm.org/D133574
---
 arch/x86/kernel/fpu/init.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 621f4b6cac4a..de96c11e1fe9 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
 	fpu__init_system_mxcsr();
 }

-/* Get alignment of the TYPE. */
-#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
-
 /*
  * Enforce that 'MEMBER' is the last field of 'TYPE'.
  *
@@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
  * because that's how C aligns structs.
  */
 #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
-	BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
-					   TYPE_ALIGN(TYPE)))
+	BUILD_BUG_ON(sizeof(TYPE) !=         \
+		     ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))

 /*
  * We append the 'struct fpu' to the task_struct:
--
2.35.1

RE: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[David Laight]

From: YingChi Long
> Sent: 27 September 2022 16:34
> 
> WG14 N2350 made very clear that it is an UB having type definitions with
> in "offsetof". This patch change the implementation of macro
> "TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.

Interesting - what justification do they give?
Linux kernel requires that the compiler add no unnecessary padding
so that structure definitions are well defined.

IIRC that standard allows arbitrary padding between members.
So using a type definition inside offsetof() won't give a
useful value - but it still isn't really UB.

...
>  #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
> -	BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
> -					   TYPE_ALIGN(TYPE)))
> +	BUILD_BUG_ON(sizeof(TYPE) !=         \
> +		     ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))

Has that ever worked?
Given:
	struct foo {
		int a;
		char b;
		char c;
	};
I think CHECK_MEMBER_AT_END_OF_TYPE(struct foo, b) is true.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

> Interesting - what justification do they give?
> Linux kernel requires that the compiler add no unnecessary padding
> so that structure definitions are well defined.

Yes, that's a clarification given in 2019.

> So using a type definition inside offsetof() won't give a
> useful value - but it still isn't really UB.

WG14 may worry about commas and the scope of new definitions. So they provide
new words into the standard and said:

> If the specified type defines a new type or if the specified member is a
> bit-field, the behavior is undefined.

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm

I've provided this link in the patch.

> Has that ever worked?
> Given:
> 	struct foo {
> 		int a;
> 		char b;
> 		char c;
> 	};

TYPE_ALIGN(struct foo) evaluates to 4 in the previous approach (based on
offsetof). _Align(struct foo) evaluates to the same value.

See https://godbolt.org/z/sqebhEnsq

> I think CHECK_MEMBER_AT_END_OF_TYPE(struct foo, b) is true.

Hmm, both the previous version and after this patch the macro gives me
false. (See the godbolt link).

RE: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

In LLVM Phab we have discussed difference between using offsetof and _Alignof.

> Technically there's no requirement that they return the same value (the
> structure could insert arbitrary padding, including no padding), so it's
> theoretically possible they return different values. But I can't think of a
> situation in which you'd get a different answer from `TYPE_ALIGN` as you
> would get from `_Alignof`.

Link: https://reviews.llvm.org/D133574#3815253

RE: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[David Laight]

From: YingChi Long
> Sent: 27 September 2022 17:44
> 
> > Interesting - what justification do they give?
> > Linux kernel requires that the compiler add no unnecessary padding
> > so that structure definitions are well defined.
> 
> Yes, that's a clarification given in 2019.
> 
> > So using a type definition inside offsetof() won't give a
> > useful value - but it still isn't really UB.
> 
> WG14 may worry about commas and the scope of new definitions. So they provide
> new words into the standard and said:
> 
> > If the specified type defines a new type or if the specified member is a
> > bit-field, the behavior is undefined.
> 
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm

Except that the kernel requires it to be defined.

Did they clarify the clause that required offsetof() to return
a compile-time constant?
That stops you doing offsetof(struct x, member->array[expression]).
(Oh and the compiler for a common OS disallows any version of that
even when expression in an integer constant!)

> 
> I've provided this link in the patch.
> 
> > Has that ever worked?
> > Given:
> > 	struct foo {
> > 		int a;
> > 		char b;
> > 		char c;
> > 	};
> 
> TYPE_ALIGN(struct foo) evaluates to 4 in the previous approach (based on
> offsetof). _Align(struct foo) evaluates to the same value.
> 
> See https://godbolt.org/z/sqebhEnsq
> 
> > I think CHECK_MEMBER_AT_END_OF_TYPE(struct foo, b) is true.
> 
> Hmm, both the previous version and after this patch the macro gives me
> false. (See the godbolt link).

See https://godbolt.org/z/95shMx44j

It return 1 for a and 0 for b and c (and char d,e following b).
NFI what it is trying to do!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

> From: YingChi Long
> > Sent: 27 September 2022 17:44
> >
> > > Interesting - what justification do they give?
> > > Linux kernel requires that the compiler add no unnecessary padding
> > > so that structure definitions are well defined.
> >
> > Yes, that's a clarification given in 2019.
> >
> > > So using a type definition inside offsetof() won't give a
> > > useful value - but it still isn't really UB.
> >
> > WG14 may worry about commas and the scope of new definitions. So they provide
> > new words into the standard and said:
> >
> > > If the specified type defines a new type or if the specified member is a
> > > bit-field, the behavior is undefined.
> >
> > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
>
> Except that the kernel requires it to be defined.
>
> Did they clarify the clause that required offsetof() to return
> a compile-time constant?
> That stops you doing offsetof(struct x, member->array[expression]).
> (Oh and the compiler for a common OS disallows any version of that
> even when expression in an integer constant!)

WG14 N2350 may just not require implementation offsetof() accepts any type
definitions within the first param of (not the second), and no further changes
about whether it is compile-time constant?

https://godbolt.org/z/9GsEPnPd6

    #include <stdio.h>

    struct foo {
        int a;
        int b[100];
    };

    int main() {
        int i;
        scanf("%d", &i);
        printf("%d\n", __builtin_offsetof(struct foo, b[i]));
    }

We consider reject type definitions within the first parameter in clang.

For example

    offsetof(struct { int a, b;}, a)

However

    struct foo {
        int a;
        int b[20];
    }
    offsetof(struct foo, b[sizeof(struct { int a, b;})])

Shall be fine.

> >
> > I've provided this link in the patch.
> >
> > > Has that ever worked?
> > > Given:
> > > 	struct foo {
> > > 		int a;
> > > 		char b;
> > > 		char c;
> > > 	};
> >
> > TYPE_ALIGN(struct foo) evaluates to 4 in the previous approach (based on
> > offsetof). _Align(struct foo) evaluates to the same value.
> >
> > See https://godbolt.org/z/sqebhEnsq
> >
> > > I think CHECK_MEMBER_AT_END_OF_TYPE(struct foo, b) is true.
> >
> > Hmm, both the previous version and after this patch the macro gives me
> > false. (See the godbolt link).
>
> See https://godbolt.org/z/95shMx44j
>
> It return 1 for a and 0 for b and c (and char d,e following b).
> NFI what it is trying to do!

Switch _Alignof back to TYPE_ALIGN, CME seems return exactly the same values. I
don't know what CME do here, but seems TYPE_ALIGN and _Alignof have the same
semantics here?

https://godbolt.org/z/hYcT1M3ed

Re: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

Kindly ping :)

Re: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[Nick Desaulniers]

On Wed, Oct 5, 2022 at 12:29 AM YingChi Long <me@inclyc.cn> wrote:
>
> Kindly ping :)

Hi YingChi,
Sorry for the delay in review.

I think https://godbolt.org/z/sPs1GEhbT has convinced me that
TYPE_ALIGN is analogous to _Alignof and not __alignof__; so your patch
is correct to use _Alignof rather than __alignof__.  I think that test
case demonstrates this clearer than the other links in the commit
message.  Please consider replacing the existing godbolt links with
that one if you agree.

Please reword the paragraphs in the commit message from:
```
In PATCH v1 "TYPE_ALIGN" was substituted with "__alignof__" which is a
GCC extension, which returns the *preferred alignment*, that is
different from C11 "_Alignof" returning *ABI alignment*. For example, on
i386 __alignof__(long long) evaluates to 8 but _Alignof(long long)
evaluates to 4. See godbolt links below.

In this patch, I'd like to use "__alignof__" to "_Alignof" to preserve
the behavior here.
```
to:
```
ISO C11 _Alignof is subtly different from the GNU C extension
__alignof__. _Alignof expressions evaluate to a multiple of the object
size, while __alignof__ expressions evaluate to the alignment dictated
by the target machine's ABI.  In the case of long long on i386,
_Alignof (long long) is 8 while __alignof__ (long long) is 4.

The macro TYPE_ALIGN we're replacing has behavior that matches
_Alignof rather than __alignof__.
```
In particular, I think it's best to avoid language like "returns" in
favor of "evaluates to" since these are expressions, not function
calls.  I think it's also good to avoid the term "preferred alignment"
since that isn't meaningful; it looks like it was pulled from one of
the GCC bug reports rather than the GCC docs or latest ISO C standard
(https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf).  I'm not
sure that the links to the GCC bug tracker add anything meaningful
here; I think those can get dropped, too.  It's also perhaps confusing
to refer to earlier versions of the patch.  One thing you can do is
include comments like that "below the fold" in a commit message as a
meta comment to reviewers.  See
https://lore.kernel.org/llvm/20220512205545.992288-1-twd2.me@gmail.com/
as an example of commentary "below the fold" on differences between
patch versions.  Text in that area is discarded by git when a patch is
applied.

With those changes to the commit message in a v3, I'd be happy to sign
off on the change.  Thanks for your work on this!
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[Nick Desaulniers]

On Wed, Oct 5, 2022 at 11:30 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Oct 5, 2022 at 12:29 AM YingChi Long <me@inclyc.cn> wrote:
> >
> > Kindly ping :)
>
> Hi YingChi,
> Sorry for the delay in review.
>
> I think https://godbolt.org/z/sPs1GEhbT has convinced me that
> TYPE_ALIGN is analogous to _Alignof and not __alignof__; so your patch
> is correct to use _Alignof rather than __alignof__.  I think that test
> case demonstrates this clearer than the other links in the commit
> message.  Please consider replacing the existing godbolt links with
> that one if you agree.
>
> Please reword the paragraphs in the commit message from:
> ```
> In PATCH v1 "TYPE_ALIGN" was substituted with "__alignof__" which is a
> GCC extension, which returns the *preferred alignment*, that is
> different from C11 "_Alignof" returning *ABI alignment*. For example, on
> i386 __alignof__(long long) evaluates to 8 but _Alignof(long long)
> evaluates to 4. See godbolt links below.
>
> In this patch, I'd like to use "__alignof__" to "_Alignof" to preserve
> the behavior here.
> ```
> to:
> ```
> ISO C11 _Alignof is subtly different from the GNU C extension
> __alignof__. _Alignof expressions evaluate to a multiple of the object
> size, while __alignof__ expressions evaluate to the alignment dictated
> by the target machine's ABI.  In the case of long long on i386,
> _Alignof (long long) is 8 while __alignof__ (long long) is 4.

Oops, and I had that backwards.

In the case of long long on i386, _Alignof (long long) is 4 while
__alignof__ (long long) is 8.

So I guess my commentary on "multiple of the object size" is
wrong...hmm...this wording can probably be improved further still...

>
> The macro TYPE_ALIGN we're replacing has behavior that matches
> _Alignof rather than __alignof__.
> ```
> In particular, I think it's best to avoid language like "returns" in
> favor of "evaluates to" since these are expressions, not function
> calls.  I think it's also good to avoid the term "preferred alignment"
> since that isn't meaningful; it looks like it was pulled from one of
> the GCC bug reports rather than the GCC docs or latest ISO C standard
> (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf).  I'm not
> sure that the links to the GCC bug tracker add anything meaningful
> here; I think those can get dropped, too.  It's also perhaps confusing
> to refer to earlier versions of the patch.  One thing you can do is
> include comments like that "below the fold" in a commit message as a
> meta comment to reviewers.  See
> https://lore.kernel.org/llvm/20220512205545.992288-1-twd2.me@gmail.com/
> as an example of commentary "below the fold" on differences between
> patch versions.  Text in that area is discarded by git when a patch is
> applied.
>
> With those changes to the commit message in a v3, I'd be happy to sign
> off on the change.  Thanks for your work on this!
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[Nick Desaulniers]

On Wed, Oct 5, 2022 at 11:38 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Oct 5, 2022 at 11:30 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Wed, Oct 5, 2022 at 12:29 AM YingChi Long <me@inclyc.cn> wrote:
> > >
> > > Kindly ping :)
> >
> > Hi YingChi,
> > Sorry for the delay in review.
> >
> > I think https://godbolt.org/z/sPs1GEhbT has convinced me that
> > TYPE_ALIGN is analogous to _Alignof and not __alignof__; so your patch
> > is correct to use _Alignof rather than __alignof__.  I think that test
> > case demonstrates this clearer than the other links in the commit
> > message.  Please consider replacing the existing godbolt links with
> > that one if you agree.
> >
> > Please reword the paragraphs in the commit message from:
> > ```
> > In PATCH v1 "TYPE_ALIGN" was substituted with "__alignof__" which is a
> > GCC extension, which returns the *preferred alignment*, that is
> > different from C11 "_Alignof" returning *ABI alignment*. For example, on
> > i386 __alignof__(long long) evaluates to 8 but _Alignof(long long)
> > evaluates to 4. See godbolt links below.
> >
> > In this patch, I'd like to use "__alignof__" to "_Alignof" to preserve
> > the behavior here.
> > ```
> > to:
> > ```
> > ISO C11 _Alignof is subtly different from the GNU C extension
> > __alignof__. _Alignof expressions evaluate to a multiple of the object
> > size, while __alignof__ expressions evaluate to the alignment dictated
> > by the target machine's ABI.  In the case of long long on i386,
> > _Alignof (long long) is 8 while __alignof__ (long long) is 4.
>
> Oops, and I had that backwards.
>
> In the case of long long on i386, _Alignof (long long) is 4 while
> __alignof__ (long long) is 8.
>
> So I guess my commentary on "multiple of the object size" is
> wrong...hmm...this wording can probably be improved further still...

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf
Section 6.2.8 "Alignment of objects" refers to "fundamental alignment"
and "extended alignment."

I wonder if it would be precise to say that "_Alignof evaluates to the
fundamental alignment while __alignof__ evaluates to the extended
alignment (which is implementation defined, typically by the machine
specific ABI)." Though even that seems imprecise since it sounds like
a fundamental alignment could be less than or equal to what alignof
evaluates to.

Grepping for `alignment requirement` turns up perhaps relevant
portions of the spec.

>
> >
> > The macro TYPE_ALIGN we're replacing has behavior that matches
> > _Alignof rather than __alignof__.
> > ```
> > In particular, I think it's best to avoid language like "returns" in
> > favor of "evaluates to" since these are expressions, not function
> > calls.  I think it's also good to avoid the term "preferred alignment"
> > since that isn't meaningful; it looks like it was pulled from one of
> > the GCC bug reports rather than the GCC docs or latest ISO C standard
> > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf).  I'm not
> > sure that the links to the GCC bug tracker add anything meaningful
> > here; I think those can get dropped, too.  It's also perhaps confusing
> > to refer to earlier versions of the patch.  One thing you can do is
> > include comments like that "below the fold" in a commit message as a
> > meta comment to reviewers.  See
> > https://lore.kernel.org/llvm/20220512205545.992288-1-twd2.me@gmail.com/
> > as an example of commentary "below the fold" on differences between
> > patch versions.  Text in that area is discarded by git when a patch is
> > applied.
> >
> > With those changes to the commit message in a v3, I'd be happy to sign
> > off on the change.  Thanks for your work on this!
> > --
> > Thanks,
> > ~Nick Desaulniers
>
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

RE: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[David Laight]

From: Nick Desaulniers
> Sent: 05 October 2022 19:58
...
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf
> Section 6.2.8 "Alignment of objects" refers to "fundamental alignment"
> and "extended alignment."
> 
> I wonder if it would be precise to say that "_Alignof evaluates to the
> fundamental alignment while __alignof__ evaluates to the extended
> alignment (which is implementation defined, typically by the machine
> specific ABI)." Though even that seems imprecise since it sounds like
> a fundamental alignment could be less than or equal to what alignof
> evaluates to.

Except that neither of those terms makes any sense to most
people.
Something like "__alignof__() is the preferred alignment and
_Alignof() the minimal alignment. For 'long long' on x86 these
are 8 and 4 respectively."

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

WG14 N2350 made very clear that it is an UB having type definitions with
in "offsetof". This patch change the implementation of macro
"TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.

I've grepped all source files to find any type definitions within
"offsetof".

    offsetof\(struct .*\{ .*,

This implementation of macro "TYPE_ALIGN" seemes to be the only case of
type definitions within offsetof in the kernel codebase.

I've made a clang patch that rejects any definitions within
__builtin_offsetof (usually #defined with "offsetof"), and tested
compiling with this patch, there are no error if this patch applied.

ISO C11 _Alignof is subtly different from the GNU C extension
__alignof__. __alignof__ is the preferred alignment and _Alignof the
minimal alignment. For 'long long' on x86 these are 8 and 4
respectively.

The macro TYPE_ALIGN we're replacing has behavior that matches
_Alignof rather than __alignof__.

Signed-off-by: YingChi Long <me@inclyc.cn>
Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
Link: https://godbolt.org/z/sPs1GEhbT
Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
Link: https://reviews.llvm.org/D133574
---
v3:
- commit message changes suggested by Nick and David

v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
---
 arch/x86/kernel/fpu/init.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 621f4b6cac4a..de96c11e1fe9 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
 	fpu__init_system_mxcsr();
 }

-/* Get alignment of the TYPE. */
-#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
-
 /*
  * Enforce that 'MEMBER' is the last field of 'TYPE'.
  *
@@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
  * because that's how C aligns structs.
  */
 #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
-	BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
-					   TYPE_ALIGN(TYPE)))
+	BUILD_BUG_ON(sizeof(TYPE) !=         \
+		     ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))

 /*
  * We append the 'struct fpu' to the task_struct:
--
2.35.1

Re: [PATCH v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[Nick Desaulniers]

On Thu, Oct 6, 2022 at 7:15 AM YingChi Long <me@inclyc.cn> wrote:
>
> WG14 N2350 made very clear that it is an UB having type definitions with
> in "offsetof". This patch change the implementation of macro
> "TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.
>
> I've grepped all source files to find any type definitions within
> "offsetof".
>
>     offsetof\(struct .*\{ .*,
>
> This implementation of macro "TYPE_ALIGN" seemes to be the only case of
> type definitions within offsetof in the kernel codebase.
>
> I've made a clang patch that rejects any definitions within
> __builtin_offsetof (usually #defined with "offsetof"), and tested
> compiling with this patch, there are no error if this patch applied.
>
> ISO C11 _Alignof is subtly different from the GNU C extension
> __alignof__. __alignof__ is the preferred alignment and _Alignof the
> minimal alignment. For 'long long' on x86 these are 8 and 4
> respectively.
>
> The macro TYPE_ALIGN we're replacing has behavior that matches
> _Alignof rather than __alignof__.
>
> Signed-off-by: YingChi Long <me@inclyc.cn>
> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> Link: https://godbolt.org/z/sPs1GEhbT
> Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
> Link: https://reviews.llvm.org/D133574

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
> v3:
> - commit message changes suggested by Nick and David
>
> v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
> ---
>  arch/x86/kernel/fpu/init.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 621f4b6cac4a..de96c11e1fe9 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
>         fpu__init_system_mxcsr();
>  }
>
> -/* Get alignment of the TYPE. */
> -#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
> -
>  /*
>   * Enforce that 'MEMBER' is the last field of 'TYPE'.
>   *
> @@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
>   * because that's how C aligns structs.
>   */
>  #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
> -       BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
> -                                          TYPE_ALIGN(TYPE)))
> +       BUILD_BUG_ON(sizeof(TYPE) !=         \
> +                    ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))
>
>  /*
>   * We append the 'struct fpu' to the task_struct:
> --
> 2.35.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

Ping :)

[PATCH RESEND v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

WG14 N2350 made very clear that it is an UB having type definitions with
in "offsetof". This patch change the implementation of macro
"TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.

I've grepped all source files to find any type definitions within
"offsetof".

    offsetof\(struct .*\{ .*,

This implementation of macro "TYPE_ALIGN" seemes to be the only case of
type definitions within offsetof in the kernel codebase.

I've made a clang patch that rejects any definitions within
__builtin_offsetof (usually #defined with "offsetof"), and tested
compiling with this patch, there are no error if this patch applied.

ISO C11 _Alignof is subtly different from the GNU C extension
__alignof__. __alignof__ is the preferred alignment and _Alignof the
minimal alignment. For 'long long' on x86 these are 8 and 4
respectively.

The macro TYPE_ALIGN we're replacing has behavior that matches
_Alignof rather than __alignof__.

Signed-off-by: YingChi Long <me@inclyc.cn>
Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
Link: https://godbolt.org/z/sPs1GEhbT
Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
Link: https://reviews.llvm.org/D133574
---
v3:
- commit message changes suggested by Nick and David

v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
Signed-off-by: YingChi Long <me@inclyc.cn>
---
 arch/x86/kernel/fpu/init.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 8946f89761cc..851eb13edc01 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
 	fpu__init_system_mxcsr();
 }

-/* Get alignment of the TYPE. */
-#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
-
 /*
  * Enforce that 'MEMBER' is the last field of 'TYPE'.
  *
@@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
  * because that's how C aligns structs.
  */
 #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
-	BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
-					   TYPE_ALIGN(TYPE)))
+	BUILD_BUG_ON(sizeof(TYPE) !=         \
+		     ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))

 /*
  * We append the 'struct fpu' to the task_struct:
--
2.37.4

Re: [PATCH RESEND v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[Nick Desaulniers]

On Sat, Oct 29, 2022 at 5:27 AM YingChi Long <me@inclyc.cn> wrote:
>
> WG14 N2350 made very clear that it is an UB having type definitions with
> in "offsetof". This patch change the implementation of macro
> "TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.
>
> I've grepped all source files to find any type definitions within
> "offsetof".
>
>     offsetof\(struct .*\{ .*,
>
> This implementation of macro "TYPE_ALIGN" seemes to be the only case of
> type definitions within offsetof in the kernel codebase.
>
> I've made a clang patch that rejects any definitions within
> __builtin_offsetof (usually #defined with "offsetof"), and tested
> compiling with this patch, there are no error if this patch applied.
>
> ISO C11 _Alignof is subtly different from the GNU C extension
> __alignof__. __alignof__ is the preferred alignment and _Alignof the
> minimal alignment. For 'long long' on x86 these are 8 and 4
> respectively.
>
> The macro TYPE_ALIGN we're replacing has behavior that matches
> _Alignof rather than __alignof__.
>
> Signed-off-by: YingChi Long <me@inclyc.cn>
> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> Link: https://godbolt.org/z/sPs1GEhbT
> Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
> Link: https://reviews.llvm.org/D133574

YingChi,
You may retain my reviewed by tag when resending a rebased patch that
hasn't changed significantly.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

That reminds me, I need to resend one of my own patches; the x86
maintainers must be very busy.

> ---
> v3:
> - commit message changes suggested by Nick and David
>
> v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
> Signed-off-by: YingChi Long <me@inclyc.cn>
> ---
>  arch/x86/kernel/fpu/init.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 8946f89761cc..851eb13edc01 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
>         fpu__init_system_mxcsr();
>  }
>
> -/* Get alignment of the TYPE. */
> -#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
> -
>  /*
>   * Enforce that 'MEMBER' is the last field of 'TYPE'.
>   *
> @@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
>   * because that's how C aligns structs.
>   */
>  #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
> -       BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
> -                                          TYPE_ALIGN(TYPE)))
> +       BUILD_BUG_ON(sizeof(TYPE) !=         \
> +                    ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))
>
>  /*
>   * We append the 'struct fpu' to the task_struct:
> --
> 2.37.4
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH RESEND v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[Borislav Petkov]

On Sat, Oct 29, 2022 at 08:25:52PM +0800, YingChi Long wrote:
> WG14 N2350 made very clear that it is an UB having type definitions
> with in "offsetof".

Pls put the working group URL note here, not below in a Link tag.

Also, write out "UB" pls.

> This patch change the implementation of macro

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> "TYPE_ALIGN" to builtin "_Alignof" to avoid undefined behavior.

But you don't change the implementation of TYPE_ALIGN - you replace it
with _Alignof().

> I've grepped all source files to find any type definitions within
> "offsetof".
>
>     offsetof\(struct .*\{ .*,
>
> This implementation of macro "TYPE_ALIGN" seemes to be the only case
> of type definitions within offsetof in the kernel codebase.
>
> I've made a clang patch that rejects any definitions within
> __builtin_offsetof (usually #defined with "offsetof"), and tested
> compiling with this patch, there are no error if this patch applied.

What does that paragraph have to do with fixing the kernel?

Is this patch going to be part of clang? If so, which version?

Does gcc need it too?

If so, should a gcc bug be opened too?

> ISO C11 _Alignof is subtly different from the GNU C extension
> __alignof__. __alignof__ is the preferred alignment and _Alignof the
> minimal alignment. For 'long long' on x86 these are 8 and 4
> respectively.
> 
> The macro TYPE_ALIGN we're replacing has behavior that matches

Who's "we"?

> _Alignof rather than __alignof__.
> 
> Signed-off-by: YingChi Long <me@inclyc.cn>
> Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
> Link: https://godbolt.org/z/sPs1GEhbT

Put that link in the text above where you talk about the *align*
differences.

> Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
> Link: https://reviews.llvm.org/D133574

Aha, that's the clang patch. Put it in the text above too pls.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH RESEND v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

> What does that paragraph have to do with fixing the kernel?

The clang patch D133574 has been made to satisfy the requirements of WG14
N2350. Compiling the kernel with this patched clang allows me to test where
type definitions are used in the kernel in the first argument of offsetof.

> Is this patch going to be part of clang? If so, which version?

Yes. Probably clang-16 because this patch is not landed to LLVM codebase
currently. The kernel needs this patch to be successfully compiled, in order
not to break the ability of LLVM mainline to compile the kernel, I am happy to
not landing D133574 for now.

> Does gcc need it too?

Since WG14 N2350 is generally applied to the C standard, I feel that GCC should
reject/fire a warning pointing out type definitions within offsetof.

> If so, should a gcc bug be opened too?

I'm not very familiar with the GCC community, but I thought it should be good
to file a bug.

Link: https://reviews.llvm.org/D133574

[PATCH v4] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[YingChi Long]

Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm

WG14 N2350 made very clear that it is an undefined behavior having type
definitions with in "offsetof". Replace the macro "TYPE_ALIGN" to
builtin "_Alignof" to avoid undefined behavior.

I've grepped all source files to find any type definitions within
"offsetof".

    offsetof\(struct .*\{ .*,

This implementation of macro "TYPE_ALIGN" seemes to be the only case of
type definitions within offsetof in the kernel codebase.

Link: https://reviews.llvm.org/D133574

A clang patch has been made that rejects any definitions within
__builtin_offsetof (usually #defined with "offsetof"), and tested
compiling the kernel using clang, there are no error if this patch
applied.

Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
Link: https://godbolt.org/z/sPs1GEhbT

ISO C11 _Alignof is subtly different from the GNU C extension
__alignof__. __alignof__ is the preferred alignment and _Alignof the
minimal alignment. For 'long long' on x86 these are 8 and 4
respectively.

The macro TYPE_ALIGN has behavior that matches _Alignof rather than
__alignof__.

Signed-off-by: YingChi Long <me@inclyc.cn>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
v4:
- commit message changes suggested by Borislav Petkov

v3:
- commit message changes suggested by Nick and David

v3: https://lore.kernel.org/all/20221006141442.2475978-1-me@inclyc.cn/

v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
---
 arch/x86/kernel/fpu/init.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 8946f89761cc..851eb13edc01 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
 	fpu__init_system_mxcsr();
 }

-/* Get alignment of the TYPE. */
-#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
-
 /*
  * Enforce that 'MEMBER' is the last field of 'TYPE'.
  *
@@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
  * because that's how C aligns structs.
  */
 #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
-	BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
-					   TYPE_ALIGN(TYPE)))
+	BUILD_BUG_ON(sizeof(TYPE) !=         \
+		     ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))

 /*
  * We append the 'struct fpu' to the task_struct:
--
2.37.4

RE: [PATCH RESEND v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[David Laight]

From: Borislav Petkov
> Sent: 02 November 2022 16:56
> 
> On Sat, Oct 29, 2022 at 08:25:52PM +0800, YingChi Long wrote:
> > WG14 N2350 made very clear that it is an UB having type definitions
> > with in "offsetof".
> 
> Pls put the working group URL note here, not below in a Link tag.
> 
> Also, write out "UB" pls.

I'm also pretty sure it is only undefined because a compiler
is allowed to add padding between structure members.
So the type definition inside offsetof() could be laid out
differently from any other similar definition.

However the kernel requires that the compiler only adds the
minimum padding required to align members.
So in the kernel it is actually fine.

OTOH using Alignof() (etc) is cleaner.

This is all similar to (probably) why clang objects to arithmetic
on NULL (ie implementing offsetof by looking at the address of
a field when NULL is cast to the struct type).
This is only undefined because the NULL pointer might not
have the all-zero bit pattern.
But pretty much every C ABI uses the zero bit pattern for NULL.
If it used any other value then most of the memset() of structures
would need changing. So for portability they ought to generate
warnings as well.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH RESEND v4] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN

[Yingchi Long]

From: YingChi Long <me@inclyc.cn>

Link: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm

WG14 N2350 made very clear that it is an undefined behavior having type
definitions with in "offsetof". Replace the macro "TYPE_ALIGN" to
builtin "_Alignof" to avoid undefined behavior.

I've grepped all source files to find any type definitions within
"offsetof".

    offsetof\(struct .*\{ .*,

This implementation of macro "TYPE_ALIGN" seemes to be the only case of
type definitions within offsetof in the kernel codebase.

Link: https://reviews.llvm.org/D133574

A clang patch has been made that rejects any definitions within
__builtin_offsetof (usually #defined with "offsetof"), and tested
compiling the kernel using clang, there are no error if this patch
applied.

Link: https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
Link: https://godbolt.org/z/sPs1GEhbT

ISO C11 _Alignof is subtly different from the GNU C extension
__alignof__. __alignof__ is the preferred alignment and _Alignof the
minimal alignment. For 'long long' on x86 these are 8 and 4
respectively.

The macro TYPE_ALIGN being replacing has behavior that matches _Alignof
rather than __alignof__.

Signed-off-by: YingChi Long <me@inclyc.cn>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
v4:
- commit message changes suggested by Borislav Petkov

v3:
- commit message changes suggested by Nick and David

v3: https://lore.kernel.org/all/20221006141442.2475978-1-me@inclyc.cn/

v2: https://lore.kernel.org/all/20220927153338.4177854-1-me@inclyc.cn/
Signed-off-by: Yingchi Long <me@inclyc.cn>
---
 arch/x86/kernel/fpu/init.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 8946f89761cc..851eb13edc01 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
 	fpu__init_system_mxcsr();
 }

-/* Get alignment of the TYPE. */
-#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
-
 /*
  * Enforce that 'MEMBER' is the last field of 'TYPE'.
  *
@@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
  * because that's how C aligns structs.
  */
 #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
-	BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
-					   TYPE_ALIGN(TYPE)))
+	BUILD_BUG_ON(sizeof(TYPE) !=         \
+		     ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))

 /*
  * We append the 'struct fpu' to the task_struct:
--
2.37.4

[tip: x86/fpu] x86/fpu: Use _Alignof to avoid undefined behavior in TYPE_ALIGN

[tip-bot2 for YingChi Long]

The following commit has been merged into the x86/fpu branch of tip:

Commit-ID:     55228db2697c09abddcb9487c3d9fa5854a932cd
Gitweb:        https://git.kernel.org/tip/55228db2697c09abddcb9487c3d9fa5854a932cd
Author:        YingChi Long <me@inclyc.cn>
AuthorDate:    Fri, 18 Nov 2022 08:55:35 +08:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Tue, 22 Nov 2022 17:13:03 +01:00

x86/fpu: Use _Alignof to avoid undefined behavior in TYPE_ALIGN

WG14 N2350 specifies that it is an undefined behavior to have type
definitions within offsetof", see

  https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm

This specification is also part of C23.

Therefore, replace the TYPE_ALIGN macro with the _Alignof builtin to
avoid undefined behavior. (_Alignof itself is C11 and the kernel is
built with -gnu11).

ISO C11 _Alignof is subtly different from the GNU C extension
__alignof__. Latter is the preferred alignment and _Alignof the
minimal alignment. For long long on x86 these are 8 and 4
respectively.

The macro TYPE_ALIGN's behavior matches _Alignof rather than
__alignof__.

  [ bp: Massage commit message. ]

Signed-off-by: YingChi Long <me@inclyc.cn>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Link: https://lore.kernel.org/r/20220925153151.2467884-1-me@inclyc.cn
---
 arch/x86/kernel/fpu/init.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 8946f89..851eb13 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -133,9 +133,6 @@ static void __init fpu__init_system_generic(void)
 	fpu__init_system_mxcsr();
 }
 
-/* Get alignment of the TYPE. */
-#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)
-
 /*
  * Enforce that 'MEMBER' is the last field of 'TYPE'.
  *
@@ -143,8 +140,8 @@ static void __init fpu__init_system_generic(void)
  * because that's how C aligns structs.
  */
 #define CHECK_MEMBER_AT_END_OF(TYPE, MEMBER) \
-	BUILD_BUG_ON(sizeof(TYPE) != ALIGN(offsetofend(TYPE, MEMBER), \
-					   TYPE_ALIGN(TYPE)))
+	BUILD_BUG_ON(sizeof(TYPE) !=         \
+		     ALIGN(offsetofend(TYPE, MEMBER), _Alignof(TYPE)))
 
 /*
  * We append the 'struct fpu' to the task_struct: