[DTrace-devel] [PATCH 5/8] symtab: add support for 'traceable' flag

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Note...

Both files need updated copyright years?

On 3/7/25 16:34, Kris Van Hees via DTrace-devel wrote:
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   libdtrace/dt_symtab.c | 53 +++++++++++++++++++++++++++++++++++++++----
>   libdtrace/dt_symtab.h |  6 +++++
>   2 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/libdtrace/dt_symtab.c b/libdtrace/dt_symtab.c
> index db63cc88..4e46f280 100644
> --- a/libdtrace/dt_symtab.c
> +++ b/libdtrace/dt_symtab.c
> @@ -23,9 +23,12 @@
>   #include <dt_string.h>
>   #include <unistd.h>
>   
> -#define DT_ST_SORTED 0x01		/* Sorted, ready for searching. */
> -#define DT_ST_PACKED 0x02		/* Symbol table packed
> +#define DT_ST_SORTED	0x01		/* Sorted, ready for searching. */
> +#define DT_ST_PACKED	0x02		/* Symbol table packed
>   					 * (necessarily sorted too) */
> +#define DT_ST_TRACEABLE	0x04		/* Symbols have traceable flag */
> +
> +#define DT_STB_TRACE	8		/* traceable symbol */
>   
>   struct dt_symbol {
>   	dt_list_t dts_list;		/* list forward/back pointers */
> @@ -275,6 +278,12 @@ dt_symbol_by_name(dtrace_hdl_t *dtp, const char *name)
>   	return dt_htab_lookup(dtp->dt_kernsyms, &tmpl);
>   }
>   
> +dt_symbol_t *
> +dt_symbol_by_name_next(const dt_symbol_t *symbol)
> +{
> +	return symbol ? (dt_symbol_t *)symbol->dts_he.next : NULL;
> +}
> +

Obviously, not a big deal, but this function seems to have nothing 
really to do with this patch.  Arguably, its introduction should be 
deferred to the next patch.  But, I'm fine with leaving it here.

>   /* Find a symbol in a given module.  */
>   dt_symbol_t *
>   dt_module_symbol_by_name(dtrace_hdl_t *dtp, dt_module_t *dmp, const char *name)
> @@ -548,7 +557,7 @@ dt_symbol_name(const dt_symbol_t *symbol)
>   void
>   dt_symbol_to_elfsym64(dtrace_hdl_t *dtp, dt_symbol_t *symbol, Elf64_Sym *elf_symp)
>   {
> -	elf_symp->st_info = symbol->dts_info;
> +	elf_symp->st_info = symbol->dts_info & ~GELF_ST_INFO(DT_STB_TRACE, 0);
>   	elf_symp->st_value = symbol->dts_addr;
>   	elf_symp->st_size = symbol->dts_size;
>   	elf_symp->st_shndx = 1; /* 'not SHN_UNDEF' is all we guarantee */
> @@ -557,7 +566,7 @@ dt_symbol_to_elfsym64(dtrace_hdl_t *dtp, dt_symbol_t *symbol, Elf64_Sym *elf_sym
>   void
>   dt_symbol_to_elfsym32(dtrace_hdl_t *dtp, dt_symbol_t *symbol, Elf32_Sym *elf_symp)
>   {
> -	elf_symp->st_info = symbol->dts_info;
> +	elf_symp->st_info = symbol->dts_info & ~GELF_ST_INFO(DT_STB_TRACE, 0);
>   	elf_symp->st_value = symbol->dts_addr;
>   	elf_symp->st_size = symbol->dts_size;
>   	elf_symp->st_shndx = 1; /* 'not SHN_UNDEF' is all we guarantee */
> @@ -581,3 +590,39 @@ dt_symbol_module(dt_symbol_t *symbol)
>   {
>   	return symbol->dts_dmp;
>   }
> +
> +/*
> + * Mark the symtab annotated with traceable flags on symbols.
> + */
> +void
> +dt_symtab_set_traceable(dt_symtab_t *symtab)
> +{
> +	symtab->dtst_flags |= DT_ST_TRACEABLE;
> +}
> +
> +/*
> + * Return whether symbols have traceable flags.
> + */
> +int
> +dt_symtab_traceable(const dt_symtab_t *symtab)
> +{
> +	return symtab->dtst_flags & DT_ST_TRACEABLE;
> +}
> +
> +/*
> + * Mark a symbol as traceable.
> + */
> +void
> +dt_symbol_set_traceable(dt_symbol_t *symbol)
> +{
> +	symbol->dts_info |= GELF_ST_INFO(DT_STB_TRACE, 0);
> +}
> +
> +/*
> + * Return true if the symbol can be traced.
> + */
> +int
> +dt_symbol_traceable(const dt_symbol_t *symbol)
> +{
> +	return GELF_ST_BIND(symbol->dts_info) & DT_STB_TRACE;
> +}
Very minor, but these four functions are very similar and yet their 
comments seem to have spurious differences.  I would think the comments 
would be boringly similar.  Also, the first comment ("Mark the symtab 
annotated with traceable flags on symbols.") makes sense to me only if I 
read the code and think about the comment a lot. How about these 
comments instead?

         /* Mark a symtab as having traceable flags on symbols. */
         /* Return whether a symtab has traceable flags on symbols. */
         /* Mark a symbol as traceable. */
         /* Return whether a symbol is traceable. */

Or even have "set" and "get" functions and reduce the number of comments 
altogether.  I mean, one can explain what "traceable" means in each 
context, but then people can easily enough understand what the "set" and 
"get" variants do.