Re: [PATCH v5 1/8] common: assembly entry point type/size annotations

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 15, 2024 at 03:34:05PM +0100, Jan Beulich wrote:
> Recent gas versions generate minimalistic Dwarf debug info for items
> annotated as functions and having their sizes specified [1]. Furthermore
> generating live patches wants items properly annotated. "Borrow" Arm's
> END() and (remotely) derive other annotation infrastructure from
> Linux'es, for all architectures to use.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> [1] 
> https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28
> ---
> v5: Drop CODE_ALIGN fallback. s/algn/align/g.
> v3: New, generalized from earlier x86-only version. LAST() (now
>     LASTARG()) moved to macros.h.
> ---
> TBD: {CODE,DATA}_ALIGN are byte granular, such that a value of 0 can be
>      specified (in case this has some special meaning on an arch;
>      conceivably it could mean to use some kind of arch default). We may
>      not strictly need that, and hence we could also make these power-
>      of-2 values (using .p2align).
> 
> Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we
> still have ALIGN.
> 
> Note further that FUNC()'s etc "align" parameter is intended to allow
> for only no or a single argument. If we wanted to also make the fill
> value customizable per call site, the constructs would need re-doing to
> some degree.
> 
> --- /dev/null
> +++ b/xen/include/xen/linkage.h
> @@ -0,0 +1,54 @@
> +#ifndef __LINKAGE_H__
> +#define __LINKAGE_H__
> +
> +#ifdef __ASSEMBLY__
> +
> +#include <xen/macros.h>
> +
> +/* CODE_ALIGN needs to be specified by every architecture. */
> +#ifndef CODE_FILL
> +# define CODE_FILL ~0
> +#endif
> +
> +#ifndef DATA_ALIGN
> +# define DATA_ALIGN 0
> +#endif
> +#ifndef DATA_FILL
> +# define DATA_FILL ~0
> +#endif
> +
> +#define SYM_ALIGN(align...) .balign align
> +
> +#define SYM_L_GLOBAL(name) .globl name
> +#define SYM_L_WEAK(name)   .weak name
> +#define SYM_L_LOCAL(name)  /* nothing */
> +
> +#define SYM_T_FUNC         STT_FUNC
> +#define SYM_T_DATA         STT_OBJECT
> +#define SYM_T_NONE         STT_NOTYPE
> +
> +#define SYM(name, typ, linkage, align...)         \
> +        .type name, SYM_T_ ## typ;                \
> +        SYM_L_ ## linkage(name);                  \
> +        SYM_ALIGN(align);                         \
> +        name:
> +
> +#define END(name) .size name, . - name
> +
> +#define FUNC(name, align...) \
> +        SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
> +#define LABEL(name, align...) \
> +        SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
> +#define DATA(name, align...) \
> +        SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL)
> +
> +#define FUNC_LOCAL(name, align...) \
> +        SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
> +#define LABEL_LOCAL(name, align...) \
> +        SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)

One thing that I've noticed while reviewing further patches, the usage
of CODE_ALIGN and CODE_FILL in LABEL_LOCAL() means that CODE_FILL must
always be a nop, or else the assembler will put garbage while padding
the symbol, and hence the preceding code will no longer fallthrough
into the label?

Looking further, LABEL_LOCAL() is always used when there's no
fallthrough, but it's IMO not obvious from the description here.

Thanks, Roger.