Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Nov 2022 17:55:39 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     YingChi Long <me@inclyc.cn>
Cc:     chang.seok.bae@intel.com, dave.hansen@linux.intel.com,
        david.laight@aculab.com, hpa@zytor.com,
        linux-kernel@vger.kernel.org, mingo@redhat.com,
        ndesaulniers@google.com, pbonzini@redhat.com, tglx@linutronix.de,
        x86@kernel.org
Subject: Re: [PATCH RESEND v3] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN
Message-ID: <Y2KhCyofvfG5W5hE@zn.tnic>
References: <20221006141442.2475978-1-me@inclyc.cn>
 <20221029122552.2855941-1-me@inclyc.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20221029122552.2855941-1-me@inclyc.cn>
Precedence: bulk

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