Re: git: ddf0ed09bd8f - main - sdt: Implement SDT probes using hot-patching

From: Mark Johnston <markj_at_freebsd.org>
Date: Mon, 24 Jun 2024 18:51:27 UTC
On Mon, Jun 24, 2024 at 09:27:55AM -0700, Ryan Libby wrote:
> On Mon, Jun 24, 2024 at 9:06 AM Mark Johnston <markj@freebsd.org> wrote:
> >
> > On Mon, Jun 24, 2024 at 08:36:55AM -0700, Ryan Libby wrote:
> > > On Wed, Jun 19, 2024 at 1:58 PM Mark Johnston <markj@freebsd.org> wrote:
> > > >
> > > > The branch main has been updated by markj:
> > > >
> > > > URL: https://cgit.FreeBSD.org/src/commit/?id=ddf0ed09bd8f83677407db36828aca2c10f419c9
> > > >
> > > > commit ddf0ed09bd8f83677407db36828aca2c10f419c9
> > > > Author:     Mark Johnston <markj@FreeBSD.org>
> > > > AuthorDate: 2024-06-19 20:57:09 +0000
> > > > Commit:     Mark Johnston <markj@FreeBSD.org>
> > > > CommitDate: 2024-06-19 20:57:41 +0000
> > > >
> > > >     sdt: Implement SDT probes using hot-patching
> > > >
> > > >     The idea here is to avoid a memory access and conditional branch per
> > > >     probe site.  Instead, the probe is represented by an "unreachable"
> > > >     unconditional function call.  asm goto is used to store the address of
> > > >     the probe site (represented by a no-op sled) and the address of the
> > > >     function call into a tracepoint record.  Each SDT probe carries a list
> > > >     of tracepoints.
> > >
> > > Questions out of curiosity and maybe ignorance:
> > >
> > > How does this work with relocations?  Something must be adjusting these
> > > addresses?
> >
> > The compiler handles this as part of the implementation of asm goto:
> > the inline assembly can reference jump targets with "%l<index>" and
> > they're specified as operands to the asm goto statement.  In the kernel
> > these references are resolved statically, and kernel modules will
> > contain relocations for the sdt_tracepoint_set section.
> >
> > > > +/*
> > > > + * Work around an apparent clang bug or limitation which prevents the use of the
> > > > + * "i" (immediate) constraint with the probe structure.
> > > > + */
> > > > +#define        _SDT_ASM_PROBE_CONSTRAINT       "Ws"
> > > > +#define        _SDT_ASM_PROBE_OPERAND          "p"
> > >
> > > Is it because i386 kmods are built with -fPIC?
> >
> > I suspect that that's related, yeah.  The compiler might be assuming
> > that some indirection is needed to compute the target address, but in
> > this case it's an address in the same function and presumably can safely
> > be assumed to be an immediate.
> >
> 
> That makes sense for the "%l1", does it also apply to the "%c0"?  Or
> does use of "%c" for the probe pointer require non-PIC?  As in, don't
> the _probes_ get relocated, and don't we need to patch the pointers to
> the probes?

When I use '%c0' to refer to the input operand, the intent is to insert
the symbol name, not the address of the probe structure as computed by
the compiler.  In an earlier iteration, there was no input operand and I
just had something like

__asm(
  ...
  ".quad " __STRING(_SDT_PROBE_NAME(...)) "\n"
  ...);

But this doesn't work when the probe symbol is local but has global
linkage (i.e., it was defined with "static"), since we don't know what
the symbol name is at compile time.  Hence the indirection, and I needed
"c" to get clang to do what I want.  The assembler encounters SDT probe
symbol names and emits relocations accordingly.  Maybe there's a better
way to do what I want?  It seems that this doesn't work at all with gcc
when -fPIC is defined.

> > > By the way, it seems gcc13 (latest in ports) doesn't support the "Ws"
> > > constraint.  It seems to have been added to gcc 14.  I know i386 is tier
> > > 2 these days, and gcc is a second consideration anyway.  Trying to test
> > > out a patch for i386 gcc, I found that it doesn't build currently and
> > > this is one of a few reasons.
> >
> > What happens if "i" is used with gcc?  IIRC I had tried gcc+i386 when
> > testing the patch but the kernel failed to build for other reasons at
> > the time.
> 
> It also errors out with constraint "%i" and modifier "%c".
> /usr/src/freebsd/sys/sys/sdt.h:216:9: error: 'asm' operand 0 probably
> does not match constraints [-Werror]
> ...
> /usr/src/freebsd/sys/sys/sdt.h:216:9: error: impossible constraint in 'asm'
> 
> It works if I remove -fPIC, but I assume we're doing that for a reason...