[DTrace-devel] [PATCH v3] Add rodata support

[Kris Van Hees]

On Fri, Aug 18, 2023 at 03:17:23PM -0400, Eugene Loh wrote:
> On 8/17/23 22:04, Kris Van Hees wrote:
> 
> > On Thu, Aug 17, 2023 at 06:10:25PM -0400, Eugene Loh via DTrace-devel wrote:
> >> On 8/17/23 10:53, Kris Van Hees via DTrace-devel wrote:
> >>> @@ -711,13 +716,20 @@ gmap_create_strtab(dtrace_hdl_t *dtp)
> >>>    	int		fd, rc, err;
> >>>    	dtp->dt_strlen = dt_strtab_size(dtp->dt_ccstab);
> >>> -	dtp->dt_zerooffset = P2ROUNDUP(dtp->dt_strlen, 8);
> >>> +	dtp->dt_rooffset = P2ROUNDUP(dtp->dt_strlen, 8);
> >>> +	dtp->dt_rosize = dt_rodata_size(dtp->dt_rodata);
> >>> +	dtp->dt_zerooffset = P2ROUNDUP(dtp->dt_rooffset + dtp->dt_rosize, 8);
> >>> +	dtp->dt_zerosize = 0;
> >>> +	sz = dt_rodata_max_item_size(dtp->dt_rodata);
> >> To me, it makes much more sense to put those last two lines after the next
> >> comment.
> > I see it more as first initializing these variables, and then we do some
> > updating based on the conditions.
> 
> In particular, "sz" is simply a temporary variable that is used only for 
> the convenience for that next block of code.? It has no use otherwise.? 
> It is local to that code block.

We're just going to have to accept that we have a difference of opinion on
this.

> >> I'd make that second argument NULL.? (See my comment later about
> >> dt_rodata_write().)
> > I disagree (likewise, see below).
> >
> >>> diff --git a/libdtrace/dt_rodata.c b/libdtrace/dt_rodata.c
> >>> @@ -0,0 +1,307 @@
> >> I'd like a block comment like this near the top of this file:
> > That seems really out of place.
> 
> Out of place because this file is so sparsely commented? Conceptually, 
> all we're doing is concatenating data, but there is so much more than 
> that going on.? Why ask new developers to reverse-engineer a file that 
> is 100s of lines long?? I do not understand the objection to a comment 
> block to explain what's going on in this file.

I'm OK with adding comments to the code (and will), though I don't think that
the code is that complex that someone cannot just look at it and see what it is
doing.  But I object to this kind of big comment block at the beginning of the
file - especially because to me) it does not really help gaining understanding
of this code.

I'll add comments to the code.

> >>  ??? The function dt_rodata_insert(dp, ptr, len) takes an input block of data,
> >>  ??? described by ptr and len, and appends it to a monolithic buffer described
> >>  ??? by dp.? The function returns the offset in the buffer where the data is
> >>  ??? stored.
> >>
> >>  ??? Actually, dp also has a hash table so that if the data block already appears
> >>  ??? in the buffer, an offset to the pre-existing copy is returned and no new
> >>  ??? copy is added.
> >>
> >>  ??? Further, dp actually stores the buffer in a growable set of smaller chunks,
> >>  ??? each chunk of size originally specified in dt_rodata_create(bufsz).
> >>  ??? The buffer can be written out to a single, linear block of memory using
> >>  ??? dt_rodata_write().
> >>> +dt_rodata_t *
> >>> +dt_rodata_create(size_t bufsz)
> >>> +{
> >>> +	dt_rodata_t	*dp = malloc(sizeof(dt_rodata_t));
> >>> +	size_t		nbuckets = _dtrace_strbuckets;
> >>> +
> >>> +	assert(bufsz != 0);
> >>> +
> >>> +	if (dp == NULL)
> >>> +		return NULL;
> >>> +
> >>> +	memset(dp, 0, sizeof(dt_rodata_t));
> >>> +	dp->hash = malloc(nbuckets * sizeof(dt_rohash_t *));
> >>> +	if (dp->hash == NULL)
> >>> +		goto err;
> >>> +
> >>> +	memset(dp->hash, 0, nbuckets * sizeof(dt_rohash_t *));
> >>> +	dp->hashsz = nbuckets;
> >>> +	dp->bufs = NULL;
> >>> +	dp->ptr = NULL;
> >>> +	dp->nbufs = 0;
> >>> +	dp->bufsz = bufsz;
> >>> +
> >>> +	if (dt_rodata_grow(dp) == -1)
> >>> +		goto err;
> >> Why not let the first dt_rodata_grow() be made by the first "insert"?
> > Since we want a first NULL element.
> 
> Why?? And why not insert it with an insert call?

Because you cannot add NULL to the rodata and because I need it there so I
can have a guaranteed value for NULL (offset 0).  I believe I commented on
that already before (as quoted below).

> >>> +
> >>> +	/* Pre-populate the read-only data with a 0-byte at offset 0.  */
> >>> +	*dp->ptr++ = 0;
> >>> +	dp->size = 1;
> >>> +	dp->count = 1;
> >> Why prepopulate?? And why not with an "insert" call?
> > To guarantee a NULL element at offset 0.
> >
> >
> >
> >>> +ssize_t
> >>> +dt_rodata_index(dt_rodata_t *dp, const char *ptr, size_t len)
> >>> +{
> >>> +	ssize_t		rc;
> >>> +	uint32_t	h;
> >>> +
> >>> +	if (ptr == NULL || ptr[0] == '\0')
> >>> +		return 0;	/* NULL or a single 0-byte are at offset 0. */
> >> Why the comment and the ptr[0]=='\0' check?
> > Because we have a prepopulated entry at offset 0 to represent a NULL or empty
> > memory block (considered to be the same as NULL).
> 
> I'm still not getting it.? Why are we hardwiring this case?? And why are 
> we treating all blocks of data whose first byte is 0 all the same?? 
> E.g., let's say we have a strtab and its first byte just happens to be 
> 0.? Now we try to insert this strtab into the rodata area.? We call 
> insert.? The insert function checks the first byte and sees it's 0.? So, 
> the entire strtab is dropped on the floor. And then when I try to use 
> strtab, I get garbage.? Or what am I missing?

I made a mistake - the test should be just if (ptr == NULL).  My stupidity.
Mea culpa.  And yes, that means I will update the comment also.

> >>> +
> >>> +	h = dt_gen_hval(ptr, len, len) % dp->hashsz;
> >>> +	rc = dt_rodata_xindex(dp, ptr, len, h);
> >>> +
> >>> +	return rc;
> >>> +}
> >>> +
> >>> +ssize_t
> >>> +dt_rodata_insert(dt_rodata_t *dp, const char *ptr, size_t len)
> >>> +{
> >>> +	dt_rohash_t	*hp;
> >>> +	ssize_t		off;
> >>> +	uint32_t	h;
> >>> +
> >>> +	if (ptr == NULL || ptr[0] == '\0')
> >>> +		return 0;	/* NULL or a single 0-byte are at offset 0. */
> >>> +
> >> Why the comment and the ptr[0]=='\0' check?
> > See above.
> >
> >>> +	h = dt_gen_hval(ptr, len, len) % dp->hashsz;
> >>> +	off = dt_rodata_xindex(dp, ptr, len, h);
> >>> +	if (off != -1)
> >>> +		return off;
> >>> +
> >>> +	/*
> >>> +	 * Create a new hash bucket, initialize it, and insert it at the front
> >>> +	 * of the hash chain for the appropriate bucket.
> >>> +	 */
> >>> +	if ((hp = malloc(sizeof(dt_rohash_t))) == NULL)
> >>> +		return -1L;
> >>> +
> >>> +	hp->data = dp->ptr;
> >>> +	hp->buf = dp->nbufs - 1;
> >>> +	hp->off = dp->size;
> >>> +	hp->len = len;
> >>> +	hp->next = dp->hash[h];
> >>> +
> >>> +	/*
> >>> +	 * Now copy the data into our buffer list, and then update the global
> >>> +	 * counts of items and bytes.  Return the item's byte offset.
> >>> +	 */
> >>> +	if (dt_rodata_copyin(dp, ptr, len) == -1) {
> >>> +		free(hp);
> >>> +		return -1L;
> >>> +	}
> >>> +
> >>> +	dp->count++;
> >>> +	dp->size += len;
> >>> +	dp->hash[h] = hp;
> >>> +	if (dp->item_size < len)
> >>> +		dp->item_size = len;
> >>> +
> >>> +	return hp->off;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Copy data from the compile-time read-only data into a DIFO-style read-only
> >> What do "compile-time" and "DIFO-style" mean?
> > Explaining that is beyond the scope of this source code file.  That is matter
> > for the overall internals of DTrace.
> 
> All right.? So a different question:? why are "compile-time" and 
> "DIFO-style" being mentioned at all?? What do they have to do with 
> anything here?? There is some linear buffer that's being stored in some 
> funny way peculiar to this file and we're going to write it out as one, 
> long, serial buffer... the sort of thing the rest of the world expects 
> when handed a ptr and a len.? Why should anyone care about 
> "compile-time" or "DIFO-style"?
> 
> >>> + * storage memory block.
> >>> + */
> >>> +ssize_t
> >>> +dt_rodata_copy(const char *s, size_t n, size_t off, char *buf)
> >> Why not just inline this function?? There is no other copy function to use.
> > But we can support multiple ones, and in fact, when strtab is implemented based
> > on rodata, we can use a single writer function and supply it with a custom
> > copy function (distinct from what rodata needs).
> 
> What would that function look like?? Today, dt_strtab_copystr() is 
> simply a memcpy.

A function like that can (and should - though never did because the original
DTrace code is not always as robust as we'd like) ensure that we do not try
to write more data than the output buffer can accept.  Or a function can be
added (as I have done in debugging) to output the string table rather than
writing it to a memory block.

> >>> +{
> >>> +	memcpy(buf + off, s, n);
> >>> +	return n;
> >>> +}
> >>> +
> >>> +ssize_t
> >>> +dt_rodata_write(const dt_rodata_t *dp, dt_rodata_copy_f *func, void *private)
> >> Why make the caller specify func?? In particular, the only func used is
> >> dt_rodata_copy(), which is in this file but the callers are outside of this
> >> file.? At the very least, insert "if (func == NULL) func = dt_rodata_copy"
> >> so that a caller does not HAVE to specify func.
> > See above.  I don't like the concept of a default function because that leads
> > to needing to go look at the writer function to figure out what the default is.
> > There is no real benefit from using a default.
> 
> Maybe it would help if I knew what other functions were being considered.

I honestly do not see that as being relevant for a code review.  The code
provides for using a callback function that is used for the actual processing
of the data chunks, and assuming the code here does what it needs to do: call
the callback function for each chunk in order, and verify return value of the
callback function to ensure the data was processed without problems, it does
what it is meant to do.

> Right now, my understanding is that rodata is some big linear space 
> where I can dump blocks of data.? I'm given the linear offset to each 
> block.? There is protection against duplicates.? The "write" function 
> presumably dumps that linear space to somewhere designated by a 
> pointer.? Why should I need to specify anything other than that 
> pointer?? Why should I have to go looking at the writer function to 
> figure out what the func argument is supposed to be?

Because, as I explain above, this function can support more callbacks.  It is
quite common when this programming technique is used that you need to consider
the call chain:

File A defines a callback function cb and calls an iterator function for some
opaque data structure, passing in cb to do the actual work.  This way the
actual implementation details of the data structure are encapsulated and in
a sense hidden from the code using it.  Which means it can change without the
need to change the code that uses it.

> > +{
> > +	ssize_t	res, total = 0;
> > +	size_t	i;
> > +	size_t	n;
> > +
> > +	for (i = 0; i < dp->nbufs; i++, total += res) {
> > +		if (i == dp->nbufs - 1)
> > +			n = dp->ptr - dp->bufs[i];
> > +		else
> > +			n = dp->bufsz;
> > +
> > +		if ((res = func(dp->bufs[i], n, total, private)) <= 0)
> > +			break;
> > +	}
> > +
> > +	if (total == 0 && dp->size != 0)
> > +		return -1;
> >> Why not just (total != dp->size)?
> > Consistency with the strtab code.
> 
> I thought that was deprecated code.? Why would we want consistency with 
> deprecated code?