lkml.org 
[lkml]   [2022]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [GIT PULL] x86/core for 5.18
On Sun, Mar 27, 2022 at 1:22 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Duh! I pushed the tag to the wrong repo (peterz/queue.git)... I just
> pushed it out to tip/tip/git as well, so hopefully it should all work
> now.

Thanks, looks good.

There's a few merges that don't have good explanations ("avoid
conflicts" really is not an explanation on its own), but those weren't
actually yours.

Looking around, the only other thing I notice is that the CC_HAS_IBT
conditional is horrendously unreadable.

There is no reason to make a Kconfig option use one single expression
from hell, when the Kconfig language is designed to be able to do so
much better.

That

def_bool ((CC_IS_GCC && $(cc-option, -fcf-protection=branch
-mindirect-branch-register)) || \
(CC_IS_CLANG && CLANG_VERSION >= 140000)) && \
$(as-instr,endbr64)

really is pretty horrendous. Yeah, we have other examples ot those
kinds of horrible expressions (*cough*KCSAN*cough*), but wouldn't it
be much nicer to just write those things legibly.

And it's nice to see the comments about _why_ some particular compiler
checks are done, but when it's all part of one very complicated thing
it's not exactly easy to read or understand.

I've obviously already pulled it (you should have seen the
pr-tracker-bot reply), but I really think it should have been written
in a much more straight-forward and obvious manner, something like:

# work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654
config GCC_HAS_IBT
def_bool y
depends on CC_IS_GCC
depends on $(cc-option, -fcf-protection=branch
-mindirect-branch-register)

# Clang/LLVM >= 14
# https://github.com/llvm/llvm-project/commit/e0b89df2e0f0130881bf6c39bf31d7f6aac00e0f
# https://github.com/llvm/llvm-project/commit/dfcf69770bc522b9e411c66454934a37c1f35332
config CLANG_HAS_IBT
def_bool y
depends on CC_IS_CLANG && CLANG_VERSION >= 140000

# binutils >= 2.29 for 'endbr64' instruction
config CC_HAS_IBT
def_bool y
depends on GCC_HAS_IBT || CLANG_HAS_IBT
depends on $(as-instr,endbr64)

instead (ok, using "AS_HAS_IBT" and "LD_HAS_IBT" to clarify all
_those_ tests too would probably have been even more legible, but I
got bored).

The above suggested Kconfig snippet is entirely untested, and meant
purely as a "look, you can split these expressions up to be more
legible".

The actual *code* seems to be nicely abstracted out, which is why I
then reacted to that Kconfig mess.

Linus

\
 
 \ /
  Last update: 2022-03-27 19:48    [W:0.052 / U:3.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site