On Sat, Jun 27, 2026 at 03:57:36PM -0700, Andrea Pinski wrote:
> On Thu, Jun 18, 2026 at 1:45 PM Kees Cook <[email protected]> wrote:
> > [...]
> > diff --git a/gcc/kcfi.h b/gcc/kcfi.h
> > new file mode 100644
> > index 000000000000..ea7e7881040f
> > --- /dev/null
> > +++ b/gcc/kcfi.h
> > [...]
> > +#include "config.h"
> > +#include "system.h"
> > +#include "coretypes.h"
> > +#include "rtl.h"
>
> This is wrong. Don't include config.h nor system.h in a header file.
Okay, removing.
>
> > +
> > +/* Common helper for RTL patterns to emit .kcfi_traps section entry.
> > + Call after emitting trap label and instruction with the trap symbol
> > + reference. */
> > +extern void kcfi_emit_traps_section (FILE *file, rtx trap_label_sym,
> > + int labelno);
> > +
> > +/* Extract KCFI type ID from current GIMPLE statement. */
> > +extern rtx internal_kcfi_get_type_id_for_expanding_gimple_call (void);
> > +
> > +/* Convenience wrapper to check for SANITIZE_KCFI. */
> > +static inline rtx
> Remove static.
Okay. This is a Linux-ism in my finger, I think. :)
> > [...]
> > --- /dev/null
> > +++ b/gcc/kcfi.cc
> > @@ -0,0 +1,696 @@
> > +/* Kernel Control Flow Integrity (KCFI) support for GCC.
> > + Copyright (C) 2025 Free Software Foundation, Inc.
>
> 2026.
Yup, I went and hunted down all of these.
> > [...]
> > +#include "config.h"
> `#define INCLUDE_STRING` is needed based on review of the previous patch.
Got it.
> > [...]
> > +/* KCFI label counter, incremented by KCFI insn emission. */
> > +static int kcfi_labelno = 0;
>
> I am not 100% sure if this needs a GTY marker or not. I suspect no
> because we should not have emitted assembly code yet.
If this moves with kcfi_next_labelno into final, I think it's okay
without GTY?
> > +
> > +/* Get next KCFI label number. Returns the current KCFI label number
> > + and increments the internal counter for the next call. The label is
> > + used to provide a unique number for each indirect callsite of the
> > + current module of the compilation. */
> > +
> > +int
> > +kcfi_next_labelno (void)
> > +{
> > + return kcfi_labelno++;
> > +}
>
> Seems like this should be instead in `final.{cc,h}` rather than `kfci.{cc,h}`.
I think you want this because it's exclusively used during output phase?
I'll get it moved.
> > +/* Function attribute to store KCFI type ID. */
> > +static tree kcfi_type_id_attr = NULL_TREE;
>
> This 100% needs a GTY marker. to make sure the identifier does not get freed.
Got it.
> > +
> > +/* Get KCFI type ID for a function type. Set it if missing. FN_TYPE
> > + is the function type tree node. Returns the cached or newly computed
> > + 32-bit KCFI type identifier, storing it as a type attribute. */
> > +
> > +static uint32_t
> > +kcfi_get_type_id (tree fn_type)
> > +{
> > + uint32_t type_id;
> > +
> > + /* Cache the attribute identifier for build_tree_list usage. */
> > + if (!kcfi_type_id_attr)
> > + kcfi_type_id_attr = get_identifier ("kcfi_type_id");
> > +
> > + tree attr = lookup_attribute ("kcfi_type_id", TYPE_ATTRIBUTES (fn_type));
> > + if (attr)
> > + {
> > + tree value = TREE_VALUE (attr);
> > + gcc_assert (value && TREE_CODE (value) == INTEGER_CST);
> gcc_assert (tree_fits_uhwi_p (value));
> type_id = tree_to_uhwi (value);
Updated.
> > + type_id = (uint32_t) TREE_INT_CST_LOW (value);
> > + }
> > + else
> > + {
> > + type_id = compute_kcfi_type_id (fn_type);
> > +
> > + tree type_id_tree = build_int_cst (unsigned_type_node, type_id);
> > + tree attr = build_tree_list (kcfi_type_id_attr, type_id_tree);
> > +
> > + TYPE_ATTRIBUTES (fn_type) = chainon (TYPE_ATTRIBUTES (fn_type),
> > attr);
> > + }
>
> Instead of an attribute there must be a better way of doing this.
> Maybe a hashset instead.
Perhaps? I will go examine this vs LTO, etc.
> > [...]
> > +rtx
> > +internal_kcfi_get_type_id_for_expanding_gimple_call (void)
> > +{
> > + gcc_assert (currently_expanding_gimple_stmt);
>
> gcall *call_stmt = dyn_cast<gcall *> (currently_expanding_gimple_stmt);
> gcc_assert (call_stmt);
Updated.
> > [...]
> > + /* Emit .type directive. */
> > + ASM_OUTPUT_TYPE_DIRECTIVE (asm_file, cfi_symbol_name.c_str (),
> > "function");
> > + ASM_OUTPUT_LABEL (asm_file, cfi_symbol_name.c_str ());
> > +
> > + /* Emit any needed alignment padding NOPs using target's NOP template.
> > */
> > + for (int i = 0; i < kcfi_alignment_padding_nops; i++)
> > + output_asm_insn (kcfi_nop, NULL);
> I think you need to calculate kcfi_nop each time you call this
> function. Instead of caching it. For GC reasons.
Uh, do I have to? This seems wasteful: the nop is generated frequently.
Should kcfi_nop gain GTY instead? I will take a closer look at this.
> > [...]
> > +static unsigned int
> > +ipa_kcfi_execute (void)
> > +{
> > + struct cgraph_node *node;
> > +
> > + /* Prepare global KCFI alignment NOPs calculation once for all
> > functions. */
> > + kcfi_prepare_alignment_nops ();
> > +
> > + /* Process all functions - both local and external. */
> > + FOR_EACH_FUNCTION (node)
> > + {
> > + tree fndecl = node->decl;
> > +
> > + /* Skip all non-NORMAL builtins (MD, FRONTEND) entirely.
> > + For NORMAL builtins, skip those that lack an implicit
> > + implementation (closest way to distinguishing DEF_LIB_BUILTIN
> > + from others). E.g. we need to have typeids for memset(). */
> > + if (fndecl_built_in_p (fndecl))
> > + {
> > + if (DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL)
> > + continue;
> > + if (!builtin_decl_implicit_p (DECL_FUNCTION_CODE (fndecl)))
> > + continue;
> > + }
> > +
> > + /* Cache the type_id in the function type. */
> > + kcfi_get_type_id (TREE_TYPE (fndecl));
>
> Why are you needing to cache here?
This was originally added for the __kcfi_typeid emission for functions
known to the TU since at the time I couldn't figure out how to deal with
lacking FN_TYPE (to see the function argument types), but that logic
changed in v3, and I never noticed I didn't have to cache it any more.
It looks like I can drop this.
So... we could just not cache this at all, I guess? It seems wrong to
keep resolving the same typeid over and over though. I will look at
hashset, though, as you suggested.
-Kees
--
Kees Cook
