Messages in this thread | | | From | Linus Torvalds <> | Date | Sun, 27 Mar 2022 10:47:38 -0700 | Subject | Re: [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
| |