On Wed, 5 Apr 2017 14:22:00 -0400
Ilia Mirkin <imir...@alum.mit.edu> wrote:
> On Wed, Apr 5, 2017 at 1:12 PM, Gregory Hainaut
> <gregory.hain...@gmail.com> wrote:
> > Context:
> > Nouveau uses NULL strings for unnamed parameter of texture gather
> > offsets opcode.
>
> To be clear, this isn't a "nouveau" thing, as it is well downstream of
> all this GLSL stuff. And FWIW llvmpipe/softpipe also support the
> 4-offset TG4 (I think). It's a GLSL IR thing. If it needs changing,
> you can change how GLSL IR represents this stuff, instead of
> necessarily supporting this wart.
>
> Unfortunately I don't understand the problem clearly enough to tell
> what's going on and what the proper solution is.
>
Ilia,
Yes you're right it isn't limited to Nouveau. And actually it could appear
in future glsl code. Actually there is another call to _mesa_add_parameter
in st_init_atifs_prog from state_tracker/st_atifs_to_tgsi.c
If I try to do a summary.
The problem is "what is the expected name of an unnamed parameter ?"
The 2 possibles answer are NULL or emptry (AKA "").
Nicolai raise the valid point that code could handle NULL and empty differently.
I tried to do various grep without success. I don't think there is any
difference
between NULL and "" but easy to say hard to demonstrate.
However I don't think that allowing NULL pointer in name is a good idea.
For example st_nir_lookup_parameter_index (st_glsl_to_nir.cpp) use
this kind of code. I'm afraid that strncmp with a NULL pointer is an
undefined behavior.
for (unsigned i = 0; i < params->NumParameters; i++) {
struct gl_program_parameter *p = ¶ms->Parameters[i];
if ((strncmp(p->Name, name, namelen) == 0) &&
The YOLO solution is appealing. We change _mesa_add_parameter NULL name
parameter to "". Then we run piglit.
Best regards,
Gregory
> >
> > Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau
> >
> > v3: Redo patch based on Nicolai/Timothy feedback
> > Create dedicated blob_write_string_optional/blob_read_string_optional to
> > handle null pointer string.
> > Add an assert in blob_write_string to ease debug
> >
> > Signed-off-by: Gregory Hainaut <gregory.hain...@gmail.com>
> > ---
> > src/compiler/glsl/blob.c | 33 +++++++++++++++++++++++++++++++++
> > src/compiler/glsl/blob.h | 27 +++++++++++++++++++++++++++
> > src/compiler/glsl/shader_cache.cpp | 4 ++--
> > 3 files changed, 62 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c
> > index 769ebf1..62d6bf5 100644
> > --- a/src/compiler/glsl/blob.c
> > +++ b/src/compiler/glsl/blob.c
> > @@ -176,9 +176,29 @@ blob_write_intptr(struct blob *blob, intptr_t value)
> > bool
> > blob_write_string(struct blob *blob, const char *str)
> > {
> > + // Please use blob_write_string_optional instead
> > + assert(str != NULL);
> > +
> > return blob_write_bytes(blob, str, strlen(str) + 1);
> > }
> >
> > +bool
> > +blob_write_string_optional(struct blob *blob, const char *str)
> > +{
> > + bool ret;
> > + const uint8_t flag = str != NULL ? 1 : 0;
> > +
> > + ret = blob_write_bytes(blob, &flag, 1);
> > +
> > + if (!ret)
> > + return false;
> > +
> > + if (str != NULL)
> > + ret = blob_write_string(blob, str);
> > +
> > + return ret;
> > +}
> > +
> > void
> > blob_reader_init(struct blob_reader *blob, uint8_t *data, size_t size)
> > {
> > @@ -321,3 +341,16 @@ blob_read_string(struct blob_reader *blob)
> >
> > return ret;
> > }
> > +
> > +char *
> > +blob_read_string_optional(struct blob_reader *blob)
> > +{
> > + uint8_t *flag;
> > + flag = (uint8_t *)blob_read_bytes(blob, 1);
> > +
> > + if (flag == NULL || *flag == 0) {
> > + return NULL;
> > + }
> > +
> > + return blob_read_string(blob);
> > +}
> > diff --git a/src/compiler/glsl/blob.h b/src/compiler/glsl/blob.h
> > index 940c81e..2f58cc3 100644
> > --- a/src/compiler/glsl/blob.h
> > +++ b/src/compiler/glsl/blob.h
> > @@ -203,6 +203,7 @@ blob_write_intptr(struct blob *blob, intptr_t value);
> >
> > /**
> > * Add a NULL-terminated string to a blob, (including the NULL terminator).
> > + * If str could be NULL, blob_write_string_optional must be used instead.
> > *
> > * \return True unless allocation failed.
> > */
> > @@ -210,6 +211,15 @@ bool
> > blob_write_string(struct blob *blob, const char *str);
> >
> > /**
> > + * Add a flag + a NULL-terminated string to a blob, (including the NULL
> > + * terminator). The flag will be zero if str is NULL.
> > + *
> > + * \return True unless allocation failed.
> > + */
> > +bool
> > +blob_write_string_optional(struct blob *blob, const char *str);
> > +
> > +/**
> > * Start reading a blob, (initializing the contents of \blob for reading).
> > *
> > * After this call, the caller can use the various blob_read_* functions to
> > @@ -294,6 +304,23 @@ blob_read_intptr(struct blob_reader *blob);
> > char *
> > blob_read_string(struct blob_reader *blob);
> >
> > +/**
> > + * Read a NULL-terminated string from the current location, (and update the
> > + * current location to just past this string).
> > + *
> > + * \note Similar as blob_read_string but return NULL if written string was
> > NULL.
> > + *
> > + * \note The memory returned belongs to the data underlying the blob
> > reader. The
> > + * caller must copy the string in order to use the string after the
> > lifetime
> > + * of the data underlying the blob reader.
> > + *
> > + * \return The string read (see note above about memory lifetime).
> > However, if
> > + * there is no NULL byte remaining within the blob, this function returns
> > + * NULL.
> > + */
> > +char *
> > +blob_read_string_optional(struct blob_reader *blob);
> > +
> > #ifdef __cplusplus
> > }
> > #endif
> > diff --git a/src/compiler/glsl/shader_cache.cpp
> > b/src/compiler/glsl/shader_cache.cpp
> > index f5e6a22..e07d2c4 100644
> > --- a/src/compiler/glsl/shader_cache.cpp
> > +++ b/src/compiler/glsl/shader_cache.cpp
> > @@ -1078,7 +1078,7 @@ write_shader_parameters(struct blob *metadata,
> > struct gl_program_parameter *param = ¶ms->Parameters[i];
> >
> > blob_write_uint32(metadata, param->Type);
> > - blob_write_string(metadata, param->Name);
> > + blob_write_string_optional(metadata, param->Name);
> > blob_write_uint32(metadata, param->Size);
> > blob_write_uint32(metadata, param->DataType);
> > blob_write_bytes(metadata, param->StateIndexes,
> > @@ -1104,7 +1104,7 @@ read_shader_parameters(struct blob_reader *metadata,
> > _mesa_reserve_parameter_storage(params, num_parameters);
> > while (i < num_parameters) {
> > gl_register_file type = (gl_register_file)
> > blob_read_uint32(metadata);
> > - const char *name = blob_read_string(metadata);
> > + const char *name = blob_read_string_optional(metadata);
> > unsigned size = blob_read_uint32(metadata);
> > unsigned data_type = blob_read_uint32(metadata);
> > blob_copy_bytes(metadata, (uint8_t *) state_indexes,
> > --
> > 2.1.4
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
