Messages in this thread | | | From | Nick Desaulniers <> | Date | Wed, 5 Oct 2022 11:30:40 -0700 | Subject | Re: [PATCH v2] x86/fpu: use _Alignof to avoid UB in TYPE_ALIGN |
| |
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
| |