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

From: Nick Desaulniers <ndesaulniers@google.com>
To: YingChi Long <me@inclyc.cn>
Cc: bp@alien8.de, chang.seok.bae@intel.com,
	dave.hansen@linux.intel.com, hpa@zytor.com,
	linux-kernel@vger.kernel.org, mingo@redhat.com,
	pbonzini@redhat.com, tglx@linutronix.de, x86@kernel.org,
	peterz@infradead.org, david.laight@aculab.com
Subject: Re: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN
Date: Wed, 5 Oct 2022 11:30:40 -0700	[thread overview]
Message-ID: <CAKwvOdm6wSgG-_HrRR_9+mLnksbK4qNA8-F--bAVTjwY1C4brA@mail.gmail.com> (raw)
In-Reply-To: <20221005072913.982634-1-me@inclyc.cn>

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