Re: [PATCH v2 0/4] ELF note-based USDT support

[Kris Van Hees <kris.van.hees@oracle.com>]

Thank you for rebasing your work on the newest tree.  That will certainly
help review them and move things forward.

I would definitely rework the commit message though, because

1. DTrace has a specific understanding of what USDT probes are and how they
   work and stap-based probes do not provide the same functionality.  One
   example of that you already point ay: they are not discoverable - i.e.
   they are not registered upon startup which is why you need to refer to
   them directly by provider name (with embedded pid).  I think you need to
   be very clear about the distinction.  Using STAPSDT or stapsdt might be
   a better choice than referring to USDT.

2. I think that the commit message fails to highlight that this support is
   to make it possible to trace programs (and shared librearies)that have been 
   built with stap style probes.  I don't think it is in the best interest of
   DTrace users to build their executables and shared libraries with stap
   style probes over DTrace USDT probes, especially given the significant
   advantage that DTrace USDT probes have (see 1. above).

3. While I can see the point of mentioning how to add stapsdt probes to
   code, it also is a source for confusion and thus is probably better left
   out.  Since this is a compability feature, surely those wanting to use it
   already have executables with such probes or know how to create them.
   By including it here, you also introduce the very unfortunate fact that
   stapsdt uses DTRACE_PROBE*() macros even though the probes have never really
   been DTrace compatible, and it is only with your proposed patch now that
   they could be used in DTrace.  The systemtap project shouldn't have
   piggy-backed on DTRACE_PROBE*() in the first place because it causes this
   type of confusion and complications, so I would very much prefer not to
   highlight that mess with this patch series.

The stap probe support is very significant because we unfortunately do have to
live with a world where there are multiple ways that such userspace probes
have been implemented.  And given that packages are released with probes and
people may want to trace them makes this addition certainly very worthwhile.
But I think it should be clear that this is for compatibilty/interoperability
purposes only.

	Kris

On Wed, Jan 29, 2025 at 02:55:18PM +0000, Alan Maguire wrote:
> This series adds support (patch 1) for ELF-note defined USDT
> probes in binaries and libraries; patches 2-4 add tests.
> 
> Basic pid-specific USDT support is added; i.e. it is necessary
> to specify the target pid in the provider name such as
> "example1234" ; future work could add pid wildcarding.
> 
> ELF note defined probes are defined by including sys/sdt.h
> from the systemtap-sdt-devel package, and are defined in
> C programs via
> 
> DTRACE_PROBEn(provider, probe, [args...])
> 
> See the tests for concrete examples.
> 
> For python, go, etc, USDT probes can be added via libstapsdt [1]
> and associated language-specific bindings.  This allows users
> of those languages to add USDT probes too.  For example, the
> following example program adds a "pythonapp" probe "firstProbe"
> using the python-specific libstapsdt binding:
> 
> #!/usr/bin/python3
> from time import sleep
> import stapsdt
> provider = stapsdt.Provider("pythonapp")
> probe = provider.add_probe(
> "firstProbe", stapsdt.ArgTypes.uint64, stapsdt.ArgTypes.int32)
> provider.load()
> while True:
>     print("Firing probe...")
>     if probe.fire("My little probe", 42):
>         print("Probe fired!")
>     sleep(1)
> 
> We can then trace this via dtrace:
> 
> # dtrace -n 'pythonapp503211:::* { printf("args %s, %d\n", copyinstr(arg0), arg1); }'
> 
> dtrace: description 'pythonapp503211:::* ' matched 1 probe
> CPU     ID                    FUNCTION:NAME
>   6 286628                      :firstProbe args My little probe, 42
> 
> [1] https://github.com/linux-usdt/libstapsdt
> 
> Alan Maguire (4):
>   USDT: support ELF-note-defined probes
>   selftests/usdt: add test for USDT note-defined probe firing, args
>   selftests/usdt: add test for USDT notes in shared library
>   selftests/usdt: add test covering different forms of USDT note args
> 
>  include/dtrace/pid.h                      |  29 ++
>  libdtrace/dt_cg.c                         |  47 ++
>  libdtrace/dt_cg.h                         |   1 +
>  libdtrace/dt_pid.c                        | 466 ++++++++++++++++++++
>  libdtrace/dt_prov_uprobe.c                |  19 +-
>  test/unittest/usdt/sdt_notes.h            | 504 ++++++++++++++++++++++
>  test/unittest/usdt/tst.usdt-notes-args.r  |   2 +
>  test/unittest/usdt/tst.usdt-notes-args.sh |  51 +++
>  test/unittest/usdt/tst.usdt-notes-lib.r   |  14 +
>  test/unittest/usdt/tst.usdt-notes-lib.sh  | 145 +++++++
>  test/unittest/usdt/tst.usdt-notes.r       |  14 +
>  test/unittest/usdt/tst.usdt-notes.sh      | 121 ++++++
>  12 files changed, 1406 insertions(+), 7 deletions(-)
>  create mode 100644 test/unittest/usdt/sdt_notes.h
>  create mode 100644 test/unittest/usdt/tst.usdt-notes-args.r
>  create mode 100755 test/unittest/usdt/tst.usdt-notes-args.sh
>  create mode 100644 test/unittest/usdt/tst.usdt-notes-lib.r
>  create mode 100755 test/unittest/usdt/tst.usdt-notes-lib.sh
>  create mode 100644 test/unittest/usdt/tst.usdt-notes.r
>  create mode 100755 test/unittest/usdt/tst.usdt-notes.sh
> 
> -- 
> 2.43.5
>