This gets me all 6 reports. The details about the array and the index don't really matter for the basic metrics:

Here's a test-case. I'd expect 6 remarks from building this:

This appears to be working for me. For before/after changes, the other half is still needed, i.e. a "accessing array of unknown size" and eventually splitting the dynamic sizing check off of that one (once -fsanitize=bounds checks __builtin_dynamic_object_size).

This information will be useful for evaluating the coverage of the bounds checker for a given program, which in turn can help guide both improvements to the bounds checker (e.g. adding knowledge from __builtin_dynamic_object_size()) and improvements to the built code base (e.g. refactoring data structures to take advantage of known compile-time or run-time bounds that will be covered by the bounds checker). It answers the question, "What percentage of array accesses are being bounds checked?" (With the implied goal of reaching 100% going forward.)

Looks good. Likely future work here would be to examine why the EX_IOERR exit is being exposed instead of just leaving SIGPIPE set to SIG_DFLT. But that's a separate issue entirely. :)

The self-tests all look correct to me, so I expect it is working how I'd expect. I haven't had a chance to do kernel builds with this yet, but I'm hoping to make time soon. I'd say if others are happy, let's land it, and if anything unexpected happens we can fix those issues if they appear. :)

This looks great to me. Thanks!

In D135727#3856978, @void wrote:

I think we opened the can or worms. :-)

In D135727#3853896, @void wrote:

@kees @serge-sans-paille: It appears to me that a terminating array of size > 2 *isn't* treated as a FAM in Clang, at least Clang warns about it. The first failure above (clang/test/Sema/array-bounds-ptr-arith.c) shows that. It turns out that the same failure occurs in that testcase when the array isn't the last in a structure, so I'll change it.

Change log typo? "but for all" should be "but not for all" ?

In D134902#3848595, @serge-sans-paille wrote:

I second the opinion here. C99 says nothing about flexible array member for unions, that's already a "language extension". (and so not be considered as FAM by -fstrict-flex-arrays=3)

In D134902#3848246, @void wrote:

@rsmith, @serge-sans-paille, and @kees, I need some advice. There's a test in clang/test/CodeGen/bounds-checking.c that's checking bounds stuff on unions. The behavior is...weird to me. It says that an array of 0 or 1 is a FAM, but one larger is not (see below).

In D135411#3841841, @samitolvanen wrote:

That being said, I did compile a 64-bit MIPS kernel using this pass, but I didn't try booting it. I would expect to run into quite a few issues initially.

What's the best way to test this in the kernel? I assume add ARCH_SUPPORTS_CFI_CLANG to an arch, and see what blows up? :) Have you tried this on any other arch yet?

LGTM

Update optional removal target release to Clang 18

use Group<clang_ignored_legacy_options_Group>

Keep git-clang-format happy

Adjust subject to be more accurate ("deprecate" vs "remove")

rebase and tweak

In D125142#3825972, @MaskRay wrote:

[clang][auto-init] Remove -enable flag for "zero" mode

The subject should be updated to mention the exact option name, not a vague -enable and "zero" mode

Excellent! Thank you for getting this prepared. Having this properly managed in the kernel means we don't have to do horrible ugly hacks to work around old 0-length arrays in UAPI (which have all been unioned with a proper flexible array now).

Thanks for all the careful consideration; I appreciate it! I'll land this tomorrow unless there are new specific objections. :)

LKML thread: https://lore.kernel.org/lkml/20220902213750.1124421-3-morbo@google.com/

Example of the bug I want to block:

In D126864#3646854, @jyknight wrote:

In D126864#3645994, @kees wrote:

I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and sizeof([]) == error, they are being treated differently already by the compiler causing bugs in Linux. The kernel must still have a way to reject the _use_ of a [0] array. We cannot reject _declaration_ of them due to userspace API.

Looks like the linux kernel is currently chock-full of zero-length-arrays which are actually intended/required to work as flexible array members. Do you have a pending patch to convert all of them to [] which should be?

I should clarify: I still need the =3 mode. Since sizeof([0]) == 0 and sizeof([]) == error, they are being treated differently already by the compiler causing bugs in Linux. The kernel must still have a way to reject the _use_ of a [0] array. We cannot reject _declaration_ of them due to userspace API.

In D126864#3641939, @serge-sans-paille wrote:

@kees are you ok with current state?

In D126864#3598257, @serge-sans-paille wrote:

@kees does the new version looks good to you?

In D126864#3584536, @serge-sans-paille wrote:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 does toward -fstrict-flex-arrays=<n> with

• n=0 ⇒ -fno-strict-flex-arrays, current state (the default)
• n=1 ⇒ only consider [ 0], [1} and [ ] as flex array member
• n=2 ⇒ only consider [ 0] and [ ] as flex array member
• n=3 ⇒ only consider [ ] as flex array member

I personnally like that approach, and I'd rather land that implementation.

In D126864#3582360, @nickdesaulniers wrote:

If the GCC developers split this into two distinct flags, should we land something we're just going to have to turn around and modify to match the new flags/semantics they've created?

Are the presubmit build failures unrelated?

In D126864#3567647, @nickdesaulniers wrote:

@kees maybe we should think about what would be needed for toolchains that don't yet support -fstrict-flex-arrays in kernel sources? Does this become a toolchain portability issue for older released toolchains we still intend to support?

In D126864#3564519, @efriedma wrote:

Do we want some builtin define so headers can check if we're in -fstrict-flex-arrays mode? It might be hard to mess with the definitions otherwise.

In D126864#3556262, @efriedma wrote:

I'm a little concerned about the premise of this, though. See https://github.com/llvm/llvm-project/issues/29694 for why we relaxed this check in the first place. I mean, the Linux kernel itself can maybe ensure it isn't doing anything silly, but most code has to deal with system headers, which are apparently broken. So this option is a trap for most code.

Thanks for working on this!

In D125142#3505767, @jfb wrote:

I think the most relevant post from @rsmith is: https://discourse.llvm.org/t/making-ftrivial-auto-var-init-zero-a-first-class-option/55143/40

He has a prototype: https://reviews.llvm.org/D79249
I assume he would like someone to pursue it further, it was a good faith attempt at not just demanding. I'd played with it and it needed a few fixes, but overall it was pretty complete. Does someone want to give it a go?

This is marked "needs revision", but I think it just needs wider review?

In D125142#3502732, @MaskRay wrote:

This cannot be committed as is. In particular, @rsmith's "We do not want to create or encourage the creation of language dialects and non-portable code," concern on https://discourse.llvm.org/t/making-ftrivial-auto-var-init-zero-a-first-class-option/55143/2 (shared by someone else) will be affected, I'd like to see that they lift their concerns.

add release notes

update with suggestions

Report flag as "unused"

In D125142#3499010, @brooks wrote:

It would be somewhat helpful as a transition aid if -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang remained as a no-op producing a warning (a generic unused argument warning would be fine).

I tested this (with D123958), and it appears to be working as intended. These two fptrs are the first and second listed, and are happily randomized:

In D123958#3459205, @aaron.ballman wrote:

I had assumed that any structure not marked for randomization would not be randomized. Based on that, I don't think inner structure objects (anonymous or otherwise) should automatically randomize their fields. WDYT?

FWIW, related problems with pskb_expand_head were seen again here:
https://github.com/ClangBuiltLinux/linux/issues/1599

I can build and boot with this. Nice! :) One issue I see is in instruction sequence ordering.

Ping; Nick, do you have a moment to rework this patch? AIUI, this is still important to getting CFI more sane for Android.

Is https://bugs.llvm.org/show_bug.cgi?id=50322 a duplicate of https://bugs.llvm.org/show_bug.cgi?id=23280 ? (Can both be closed?)

Yeah, I can confirm that many cases are improved with this patch (others are more sensitive and depend on the __bos behavior I mentioned). With the 17 kernel FORTIFY self-tests, all 17 fail without this patch. With this patch, 9 start passing. Nice!

I'm setting up to test this patch (thank you!) using my current kernel FORTIFY improvements. Right now I have a bunch of compile-time behavior selftests written:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/overflow&id=3c5221f3f4fd865a780f544b72c68f4209bd2e76

Does this address https://bugs.llvm.org/show_bug.cgi?id=50322 ? (I assume this is a new version of https://reviews.llvm.org/D92657 ?)

PoC from Sami:
https://godbolt.org/z/xfsWjhGhq

FWIW, I've tested this on a full Linux kernel build and I can confirm it's building correctly; I'm able to start my incremental FORTIFY fixes now. :) Thanks!

Is it possible to plumb fd instead of pathname? Then fchown(), fsetxattr(), etc, can all be used?

The kernel's stance on switch statements reads:

Ah! Yes, I see it now. Thanks and sorry for the noise!

Sorry if I missed something here, but why is this marked as "Closed"? It seems like the feature has still not landed (i.e. it got reverted).

In D80791#2120503, @nickdesaulniers wrote:

Might someone wish to disable PAC/BTI on an individual function, while having it on for the rest? I guess that would mean you can't call that function indirectly?

Specifically, this appears to be a legitimate bug, found by the warnings: https://bugs.llvm.org/show_bug.cgi?id=46258

Should the per-function analysis warning actually be removed? That seems like a helpful check to catch a different form of bad behavior.

In D75225#1896066, @MaskRay wrote:

In D75225#1895813, @kees wrote:

.text.* -> .text

This is not accurate: ld.bfd will keep the .text.$foo names, but place them all after the .text (it does not merge them into .text). Currently, ld.lld seems to merge them into .text. FGKASLR depends on the non-merging behavior.

I think the description is correct. I have a line // If a SECTIONS command is not specified in the code block.

Here is GNU ld's internal linker script:

.text           :
{
  *(.text.unlikely .text.*_unlikely .text.unlikely.*)
  *(.text.exit .text.exit.*)
  *(.text.startup .text.startup.*)
  *(.text.hot .text.hot.*)
  *(.text .stub .text.* .gnu.linkonce.t.*)
  /* .gnu.warning sections are handled specially by elf32.em.  */
  *(.gnu.warning)
}

(As you can see, -z keep-text-section-prefix does less than what GNU ld does. One issue with GNU ld's internal linker script is that -ffunction-sections (typical when building a libc) will cause the function exit to be reordered before others...)

Hi! What's the state of this change? Do you need help committing this?

This is not accurate: ld.bfd will keep the .text.$foo names, but place them all after the .text (it does not merge them into .text). Currently, ld.lld seems to merge them into .text. FGKASLR depends on the non-merging behavior.

Awesome! With this and D75149 my defconfig kernel build now only shows:

On my orphan checking kernel series, I'm left with only .rela_* and .rela.* getting reported, along with:

Thank you! I can confirm this fixes the problems I saw building the Linux kernel with CONFIG_UBSAN=y.

Thank you for the quick fix! I can confirm my builds with --string-debug work now. :)

For latest version see https://reviews.llvm.org/D64838

Just FYI, I can confirm a happily running arm64 kernel with CFI enabled built with this patch series. The C wrappers aren't needed and CFI is still triggering on mismatches:

Nick points out that "REQUIRES: x86-registered-target" is likely not needed.

Rebasing to monorepo...

Rebased to latest LLVM

I can confirm this fixes the Linux kernel relocation visibility problem I saw. Thank you!

Should I respin to make booleans always zero extended? I can adjust the X86 code at the same time...

For the non-X86 case: https://reviews.llvm.org/D60224

For note, this is based on https://reviews.llvm.org/D60208