Re: [PATCH v7 3/3] kallsyms: Unroll 24-bit sequence reconstruction in get_symbol_seq()

[David Laight]

On Tue, 29 Sep 2026 23:56:12 -0700
Kees Cook <kees@xxxxxxxxxx> wrote:

> On Tue, Sep 29, 2026 at 12:07:32PM -0600, Jim Cromie via B4 Relay wrote:
> > Mark get_symbol_seq() as static inline and unroll the 3-byte extraction
> > into direct byte shifts: (p[0] << 16) | (p[1] << 8) | p[2]. This
> > eliminates loop induction variable maintenance and allows the compiler
> > to generate direct loads and constant shifts.  
> 
> So... did you actually see any difference in the output binary? This is
> such a small loop I'd expect every compiler to both inline and unroll
> it already. *time passes* I've checked; I was mostly right. :)
> 
> It's already inlined and unrolled by the compilers, but GCC weirdly
> didn't see through the array math:
> 
>   At -O2 GCC (and Clang, mostly the same) on x86_64:
> 
>   before                                  after
>   lea    (%rax,%rax,2),%eax               lea    (%rax,%rax,2),%edx
>   mov    %rax,%rdx                        movslq %edx,%rdx
>   movzbl seqs(%rax),%eax                  movzbl seqs(%rdx),%eax
>   lea    0x1(%rdx),%ecx                   movzbl seqs+2(%rdx),%ecx
>   add    $0x2,%edx                        movzbl seqs+1(%rdx),%edx
>   movzbl seqs(%rcx),%ecx                  shl    $0x10,%eax
>   shl    $0x8,%eax                        shl    $0x8,%edx
>   movzbl seqs(%rdx),%edx                  or     %ecx,%eax
>   or     %ecx,%eax                        or     %edx,%eax
>   shl    $0x8,%eax
>   or     %edx,%eax
> 
> So it's actually the loss of the "+ i" part that does it.

A quick 'register dependency chain count' gives the old as 7 and the
new as 6.
The annoying 'movslq' might be removable by making the parameter
either 'unsigned int' or 'long' (and might go away if the function
in inlined).

> 
> > -static unsigned int get_symbol_seq(int index)
> > +static inline unsigned int get_symbol_seq(int index)
> >  {
> > -	unsigned int i, seq = 0;
> > +	const u8 *p = &kallsyms_seqs_of_names[3 * index];
> >  
> > -	for (i = 0; i < 3; i++)
> > -		seq = (seq << 8) | kallsyms_seqs_of_names[3 * index + i];
> > -
> > -	return seq;
> > +	return (p[0] << 16) | (p[1] << 8) | p[2];
> >  }  
> 
> I'm on the fence about readability, but I think it's improved.

Ditto.
There might be a get_unaligned_be24() but that doesn't help readability.

The code would be better if the value were host-endian.
gcc 16+ then uses a 16bit read for two of the bytes.
That also lets the table be defined as:
	extern struct { unsigned int v:24; } kallsyms_seqs_of_names[];
so that accesses can just be kallsyms_seqs_of_names[n].v
(Although even gcc 16 seems to do three reads in that case.)

If the cpu supports misaligned accesses you can do a 32bit read
and an 8bit shift.

David

> 
> Reviewed-by: Kees Cook <kees@xxxxxxxxxx>
>