[DTrace-devel] [PATCH v2 13/20] Store strings in the string table with varint length prefix

[Kris Van Hees]

On Fri, Jun 04, 2021 at 03:14:08PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> 
> In
>          libdtrace/dt_bpf.c
>          libdtrace/dt_cc.c
>          libdtrace/dt_dis.c
> I see
>          +#include <dt_varint.h>
> Why?  I think this include is not needed since you encapsulate the vint 
> stuff in the new function, which is declared in dt_impl.h.

Yes, good catch.

> In
>          dt_strtab_index()
>          dt_strtab_insert()
> is "s = malloc()" a memory leak?  In the case of dt_strtab_insert(), 
> freeing s means paying attention to multiple return paths.

Hm, I thought I had fixed that but obviously not.  Thanks for pointing this
out because I have a few other things that might be leaking also.  This one
will be fixed in v3, and I'll investigate the other potentials that actually
might be long-standing.

> Finally...
> 
> On 6/3/21 11:17 AM, Kris Van Hees wrote:
> > With the introduction of variable length integers, strings in D (and
> > then also BPF) can be stored with their length embedded in the byte
> > stream.  This makes it possible to perform operations on strings
> > without the need to recalculate the length of the string multiple
> > times.
> >
> > The string constant table stored in DIFOs contains strings that will
> > be made available to the BPF program by loading the string constant
> > table into a BPF map.  It makes sense to already store strings using
> > the variable length prefix when the string constant table is
> > constructed, which then also requires code that makes of it to know
> > about the variable length integer prefixing each string.
> 
> "makes of it" -> "makes use of it"
> But what does this last phrase mean?  Is this the same message as that 
> of the following paragraph?  Or a different idea?

It is a lead in... it establishes what is needed, and the next paragraph
states how we provide what is needed.

> > This patch introduces a dt_difo_getstr() function that returns a
> > regular C char * entity given a DIFO and a string offset into the
> > string constant table for that DIFO.  Any code that needs to access
> > a string constant from a DIFO is updated to make use of this new
> > function.
> >
> > The string constant table creation code is updated to account for
> > the fact that the empty string will now occupy 2 bytes (one for the
> > length that is 0, and one for the terminating 0-byte.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel