Re: [RFC PATCH 2/2] local_lock: do not put btf_type_tag on the lock guard type

[Chris Down <chris@chrisdown.name>]

Hi Vineet,

Vineet Gupta <vineet.gupta@linux.dev> writes:
>	typedef local_lock_t __seg_gs
>	__attribute__((btf_type_tag("percpu"))) lock_local_lock_t;
>
>	typedef struct { local_lock_t __seg_gs
>	__attribute__((btf_type_tag("percpu"))) *lock; } class_local_lock_t;
>
>Both put the tag on local_lock_t, an aggregate: the first on a typedef of
>it, the second on the pointee of the guard's member.

The typedef is the aggregate case, and it's the only one of the two that
GCC warns about. But the second one is just the "T __percpu *p" spelling
that 1/2 keeps the tag for. _T->lock really is a pointer to per-cpu memory,
since local_lock() passes it to this_cpu_ptr(). So the tag is correct
there. This patch drops it from the guard's lock member and the
constructor's argument as well, not only from the typedef. I think it
should keep the tag on those two and only drop it from the typedef.

>Every variant of an
>aggregate shares one DIE, so there is nowhere to record it, exactly as for
>DECLARE_PER_CPU() of a struct. clang writes the annotation into DWARF
>without complaint, but pahole does not translate it from that position,
>so with today's toolchains it reaches BTF from neither compiler.

That's true of the typedef, but not of the member. With clang 23 and
pahole 1.32 the lock member comes out as PTR -> TYPE_TAG 'percpu' ->
local_lock_t, the same as any other per-cpu pointer.

For the typedef, clang 23 puts the annotation on the typedef's own DIE, as
you say, so there is somewhere to record it. It's only pahole that doesn't
pick it up yet. You also mentioned GCC patches for type tags on typedefs
in the pahole thread [0]. Once those land, does this warning go away?

>Unlike the per-cpu declarations this changes no BTF at all. A tag in that
>position was never recorded, so there is nothing to remove, and the one
>percpu TYPE_TAG that does reach a local_lock type comes from those casts,
>which keep it. An x86 defconfig built with clang is identical with and
>without this patch: 152551 BTF types and a 6828300 byte .BTF either way,
>and .text is unchanged. The patch removes a GCC diagnostic and no more.

The defconfig BTF only stays the same because nothing in a defconfig uses
these guards. lib/test_context-analysis.c is the only user in the tree.
With CONFIG_CONTEXT_ANALYSIS_TEST=y, class_local_lock_t,
class_local_lock_irq_t, class_local_lock_irqsave_t and
class_local_lock_nested_bh_t all lose the percpu tag on lock with this
patch applied.

Nothing in BPF looks at those types today, so this doesn't break anything.
But it does remove correct annotations to get rid of a warning that is
only ever caused by the typedef itself. When the guards were added, the
type was only ever used as _type *. The typedef came later with
3931d4b98039 ("cleanup: Basic compatibility with context analysis"). If
GCC isn't going to accept the tag on a typedef, could this be limited to
that typedef in cleanup.h instead, so the guard's pointer keeps its tag?
As far as I can see, lock_##_name##_t is only ever used through a
pointer, in DECLARE_LOCK_GUARD_1_ATTRS().

[0] https://lore.kernel.org/r/252c53f3-3f06-40f4-9cd3-795eb298687c@linux.dev

Thanks!

Chris