On Fri, Dec 12, 2014 at 2:37 PM, Carl Worth <cwo...@cworth.org> wrote:
>
> On Thu, Dec 11 2014, Jason Ekstrand wrote:
> > Yeah, this looks nifty.
>
> Thanks!
>
> > I've got some comments on the implementation though.
>
> I really appreciate the careful review. I obviously didn't pay quite as
> much attention to detail as I would like to when writing this patch. I'm
> glad I didn't bury this code in a giant "here's the shader cache" series
> where this review wouldn't have happened.
>
No problem. It was a fun little review.
>
> >> + * Permission is hereby granted, free of charge, to any person
> obtaining
> > a
>
> [Incidentally, your email to me appears to have been word-wrapped after
> the quote prefixes were added, (and those prefixes pushing some lines
> past 80 columns). That made it a bit harder to read. I haven't seen this
> failure mode commonly before. Part of that may be due to the
> mesa/.dir-locals.el file which is making my emacs wrap my code at 78
> columns rather than its default of 70. Maybe that should be toned down a
> bit.]
>
You can probably blame it on G-Mail
>
> >> + if (blob->allocated == 0)
> >> + to_allocate = BLOB_INITIAL_SIZE;
> >> + else
> >> + to_allocate = blob->allocated * 2;
> >
> > What happens if additional is bigger than blob->size? Maybe iterate (if
> > you want to keep power-of-two) or take a max here.
>
> Good catch. I'm not particularly concerned about a power-of-two size. So
> I've changed the code to take a max here.
>
> >> +static bool
> >> +align_blob (struct blob *blob, size_t alignment)
> >> +{
> >> + size_t new_size;
> >> +
> >> + new_size = ALIGN(blob->size, alignment);
> >
> > Do we what to align the pointer or the size? Aligning the size only
> makes
> > sense if the pointer is already aligned.
>
> Note that the size we're aligning here (blob->size) is the size of all
> data that's already written to the blob, (not the size of the object
> about to be written that is imposing an alignment constraint).
>
> It's not clear to me if your question was based on a misreading of that.
>
Right, I think I misread something. We don't really have a pointer to
align here. My point was that if the pointer is only 16-aligned then it's
no good to 32-align the size because ptr + size will still only be
16-aligned. However, given that it's coming out of malloc and we only care
about 64bit things, it should be fine.
>
> > At the very least, we should assert that the requested alignment is
> > less than or equal to the known pointer alignment for the start of the
> > blob.
>
> The start of the blob's data is a pointer returned by malloc(), (or
> realloc()), so the known pointer alignment there should be adequate for
> anything we're writing to it.
>
Sure. It's probably fine then.
--Jason
>
> >> +static void
> >> +align_blob_reader (struct blob_reader *blob, size_t alignment)
> >> +{
> >> + blob->current = blob->data + ALIGN(blob->current - blob->data,
> > alignment);
> >
> > Again, pointer or size? Also, why is this way up here when the other
> > reader stuff comes later.
>
> I guess I was putting all of the allocation and alignment code in one
> place, (which wasn't obvious since the blob_reader code doesn't have any
> allocation). Obviously, that's easy to move. I'll do that.
>
> >> +void
> >> +blob_reader_init (struct blob_reader *blob, uint8_t *data, size_t size)
> >
> > Calling it blob is a bit disconcerting, but probably not a big deal.
>
> Yes. I hesitated a bit when doing that. This came about probably because
> I had earlier toyed with having a single, shared data structure for both
> purposes, but I decided that they really are different. At the very
> least this "struct blob_reader as 'blob'" naming is contained to just
> this file and obviously doesn't leak into any callers or anything.
>
> >> +void *
> >> +blob_read_bytes (struct blob_reader *blob, size_t size)
> >> +{
> >> + void *ret;
> >> +
> >> + if (blob->current > blob->end || blob->end - blob->current < size)
> >> + return NULL;
> >
> > Why isn't this a call to ensure_can_read? Without this, the overrun flag
> > never gets set.
>
> Thanks for the catch.
>
> > We also don't get overrun set for out-of-bounds copies and
> > string reads. If that's intentional, it should be documented.
>
> That was not intentional. Fixing blob_read_bytes() above takes care of
> out-of-bounds copies. For blob_read_string I've just added:
>
> blob->overrun = true;
>
> to the case that handles "no 0 byte appears in the remainder of the
> blob's data".
>
> > Also, you reversed the logic wrong. It should be current >= end.
>
> I don't think so. Checking for "current >= end" is a check to see
> whether a previous operation has overrun. But what we want to check here
> is whether the current operation would overrun, (whether or not the
> previous one had).
>
> But it doesn't matter since I just deleted that code and replaced it
> with a call to ensure_can_read().
>
> >> +blob_read_string (struct blob_reader *blob)
> >> +{
> >> + int size;
> >> + char *ret;
> >> + uint8_t *nul;
> >> +
> >> + /* If there is no NULL terminator, the reader gets nothing. */
> >> + nul = memchr (blob->current, 0, blob->end - blob->current);
> >
> > Should we check for current >= end first?...
>
> Yes. That's another good fix, (for the case of trying to read a string
> immediately after having read the entire blob's data).
>
>
> >> + if (! ensure_can_read(blob, size))
> >> + return 0;
> >
> > ...If we do, this does nothing.
>
> How about for my own comfort, I leave that in as:
>
> assert(ensure_can_read(blob, size));
>
> > We may want to set the overrun flag if there is no null.
>
> As I mentioned above, I did add this.
>
> >> +bool
> >> +blob_overwrite_bytes (struct blob *blob,
> >> + size_t offset,
> >> + const void *bytes,
> >> + size_t to_write);
> >
> > This function isn't implemented.
>
> Thanks. As you mentioned in your other email, I moved this chunk to the
> second patch.
>
> I'll follow up with a second revision of this patch incorporating all of
> the changes described here.
>
> -Carl
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
