| Message ID | 38de36da-0822-4452-9594-8d50a5a77335@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | annotate entry points with type and size | expand |
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@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > [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. SYM_ALIGN seems fine for the purpose here. > > 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 I find the fills a bit odd, compared to what we use now (nops for x86 IIRC). Thanks, Roger.
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@suse.com> > > [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.
On 17.01.2024 18:02, 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@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> --- /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 > > I find the fills a bit odd, compared to what we use now (nops for x86 > IIRC). Well, these are generic defaults. X86 then overrides CODE_FILL for it to remain NOP. ~0 is the best I can think of as an arch-agnostic default, considering the half dozen architectures I know at least a little. Jan
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@suse.com> >> >> [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
--- /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) +#define DATA_LOCAL(name, align...) \ + SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) + +#endif /* __ASSEMBLY__ */ + +#endif /* __LINKAGE_H__ */ --- a/xen/include/xen/macros.h +++ b/xen/include/xen/macros.h @@ -21,6 +21,15 @@ #define count_args(args...) \ count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0) +#define ARG1_(x, y...) (x) +#define ARG2_(x, y...) ARG1_(y) +#define ARG3_(x, y...) ARG2_(y) +#define ARG4_(x, y...) ARG3_(y) + +#define ARG__(nr) ARG ## nr ## _ +#define ARG_(nr) ARG__(nr) +#define LASTARG(x, y...) ARG_(count_args(x, ## y))(x, ## y) + /* Indirect macros required for expanded argument pasting. */ #define PASTE_(a, b) a ## b #define PASTE(a, b) PASTE_(a, b)
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@suse.com> [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.