[DTrace-devel] [PATCH v2] Add support for uregs[]

[Kris Van Hees]

On Fri, Feb 24, 2023 at 06:52:43PM -0500, Eugene Loh via DTrace-devel wrote:
> On 2/24/23 15:15, Kris Van Hees wrote:
> 
> > On Fri, Feb 24, 2023 at 02:12:36PM -0500, Kris Van Hees via DTrace-devel wrote:
> > > On Tue, Feb 21, 2023 at 11:49:11AM -0500, eugene.loh--- via DTrace-devel wrote:
> > > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > > +#define DT_CG_REG_TRAPNO      25
> > > > +#define DT_CG_REG_GS          24
> > > > +#define DT_CG_REG_FS          23
> > > > +#define DT_CG_REG_ES          22
> > > > +#define DT_CG_REG_DS          21
> > > > +#define DT_CG_REG_SS          20
> > > > +#define DT_CG_REG_RSP         19
> > > > +#define DT_CG_REG_RFL         18
> > > > +#define DT_CG_REG_CS          17
> > > > +#define DT_CG_REG_RIP         16
> > > > +#define DT_CG_REG_ERR         15
> > > > +#define DT_CG_REG_RDI         14
> > > > +#define DT_CG_REG_RSI         13
> > > > +#define DT_CG_REG_RDX         12
> > > > +#define DT_CG_REG_RCX         11
> > > > +#define DT_CG_REG_RAX         10
> > > > +#define DT_CG_REG_R8          9
> > > > +#define DT_CG_REG_R9          8
> > > > +#define DT_CG_REG_R10         7
> > > > +#define DT_CG_REG_R11         6
> > > > +#define DT_CG_REG_RBX         5
> > > > +#define DT_CG_REG_RBP         4
> > > > +#define DT_CG_REG_R12         3
> > > > +#define DT_CG_REG_R13         2
> > > > +#define DT_CG_REG_R14         1
> > > > +#define DT_CG_REG_R15         0
> > > We do not really need these (except for perhaps the _DS, _ES, etc ones since
> > > they are used in a switch statement below.  The rest can just be mentioned in
> > > comments in the regmap table which then can just use the numeric values.  At
> > > some point, we probably should ensure we have a single file that defines these
> > > constants and then use that here *and* in the regs.d file.  But right now, just
> > > use the values and comment.
> > > 
> > > > +
> > > > +static void
> > > > +dt_cg_uregs(unsigned int idx, dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > > > +{
> > > > +	int regmap[] = { DT_CG_REG_RBX,	/*  0 -> EBX */
> > > > +			 DT_CG_REG_RCX,	/*  1 -> ECX */
> > > > +			 DT_CG_REG_RDX,	/*  2 -> EDX */
> > > > +			 DT_CG_REG_RSI,	/*  3 -> ESI */
> > > > +			 DT_CG_REG_RDI,	/*  4 -> EDI */
> > > > +			 DT_CG_REG_RBP,	/*  5 -> EBP */
> > > > +			 DT_CG_REG_RAX,	/*  6 -> EAX */
> > > > +			 DT_CG_REG_DS,	/*  7 -> DS */
> > > > +			 DT_CG_REG_ES,	/*  8 -> ES */
> > > > +			 DT_CG_REG_FS,	/*  9 -> FS */
> > > > +			 DT_CG_REG_GS,	/* 10 -> GS */
> > > > +			 DT_CG_REG_TRAPNO,	/* 11 -> TRAPNO */
> > > > +			 DT_CG_REG_RIP,	/* 12 -> EIP */
> > > > +			 DT_CG_REG_CS,	/* 13 -> CS */
> > > > +			 DT_CG_REG_RFL,	/* 14 -> EFL */
> > > > +			 DT_CG_REG_RSP,	/* 15 -> UESP */
> > > > +			 DT_CG_REG_SS,	/* 16 -> SS */
> > > > +			};
> > Looking at this a bit more, I think that this mapping should really be
> > reflected in the regs.d file instead of here.  Since there is nothing that is
> > expected (or even supported) to depend on the numeric values of any of the
> > constants that are provided by regs.d, and since there is nothing that documents
> > the values such that you can expect to acces uregs using numeric values and
> > expect it to do "the right thing", I really think the mapping should be done
> > there...  I.e. ensure that each symbolic name has a value index value that
> > refers to the correct location of the value in pt_regs[].
> > 
> > And that means this entire business of mapping values can be removed here.  It
> > is too much of a mess (and was a mess in v1 also - without needing to be).
> 
> I did what you first suggested.
> 
> Now, there are three "ranges" of indexes.  I *think*:
> 
> 1)  The very highest are "aliases".  This is why we have this regmap.  They do not have symbolic names and are not documented.  They cannot go into regs.d unless we make up (and document) names for them.

When you compare the list in dt_cg.c and regs.d you will find that both have
all the values.  So I do not know what you mean with them not having symbolic
names because they clearly do.

> 2)  The next highest map to task->thread. members.  They are documented and in regs.d.
> 
> 3)  The lowest are documented and in regs.d and index pt_regs[].
> 
> So, I guess the indexes are already in regs.d (not in dt_cg.c) except:
> 
> a) There is the funny business of how to map "aliases" -- that's what regmap does.

But since regmap is a static mapping it is as easily done by updating the
values in regs.d to be the correct values.

> b) There are indexes that pick out task->thread. members, which is not a problem one solves in regs.d.

Well, yes, you would still need some special casing for those few uregs[]
values.  That is perfectly fine.

I still do not see the problem why you don't use the suggestion to just do the
mapping in regs.d and be done with it.  From what I can see it seems to be
primarily i386 vs amd64 register naming.