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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2024 15:52, Roger Pau Monné wrote:
> 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?

Well, except when an arch has no such cases, it'll need to override
CODE_FILL. Hence why for now Arm and PPC architectures get away without
such an override. It may well be that at least Arm will need to gain
one as soon as the first case of falling through is converted to this
new model. See also the RISC-V change, where it becomes permissible for
CODE_FILL to expand to nothing (thus using assembler built-in defaults).

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

Not sure what adjustment to the description you are thinking of. What's
used where isn't dictated by this patch / framework.

Jan