Re: [Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string

2017-04-11 Thread Nicolai Hähnle 

On 07.04.2017 21:45, gregory hainaut wrote:

On Thu, 6 Apr 2017 00:21:19 +0200
gregory hainaut  wrote:


On Wed, 5 Apr 2017 14:22:00 -0400
Ilia Mirkin  wrote:


On Wed, Apr 5, 2017 at 1:12 PM, Gregory Hainaut
 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 = >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


Hello All,

I commented the "Name" field in the struct to see the gcc error message.
I did also some grep to be sure I didn't miss anything.

I didn't find anything that differentiate a NULL pointer from an empty
string. Actually "Name" is used inside printf and to lookup the index/location.
Note: some code doesn't check for NULL pointer.

copy_indirect_accessed_array (prog_parameter_layout.c) will use NULL on
purpose to avoid a double-free (due to pointer memcpy). But it is only
a temporary struct that will be freed by the caller (_mesa_layout_parameters)

Conclusion, it would be enough to add a parameter with
p->Name = strdup(name ? name : "");


I like this variant. It eliminates the need for special NULL-handling 
everywhere.


Thanks for doing those investigations!
Nicolai




Or we can keep the *string_optional function. It is up to you. Tell me
what solution do you prefer and I will redo the patch accordingly.

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 
---
 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, , 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.

Re: [Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string

2017-04-07 Thread gregory hainaut 
On Thu, 6 Apr 2017 00:21:19 +0200
gregory hainaut  wrote:

> On Wed, 5 Apr 2017 14:22:00 -0400
> Ilia Mirkin  wrote:
> 
> > On Wed, Apr 5, 2017 at 1:12 PM, Gregory Hainaut
> >  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 = >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

Hello All,

I commented the "Name" field in the struct to see the gcc error message.
I did also some grep to be sure I didn't miss anything.

I didn't find anything that differentiate a NULL pointer from an empty
string. Actually "Name" is used inside printf and to lookup the index/location.
Note: some code doesn't check for NULL pointer.

copy_indirect_accessed_array (prog_parameter_layout.c) will use NULL on
purpose to avoid a double-free (due to pointer memcpy). But it is only 
a temporary struct that will be freed by the caller (_mesa_layout_parameters)

Conclusion, it would be enough to add a parameter with
p->Name = strdup(name ? name : "");

Or we can keep the *string_optional function. It is up to you. Tell me
what solution do you prefer and I will redo the patch accordingly.

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 
> > > ---
> > >  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, , 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

Re: [Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string

2017-04-05 Thread gregory hainaut 
On Wed, 5 Apr 2017 14:22:00 -0400
Ilia Mirkin  wrote:

> On Wed, Apr 5, 2017 at 1:12 PM, Gregory Hainaut
>  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 = >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 
> > ---
> >  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, , 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);
> >

Re: [Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string

2017-04-05 Thread Ilia Mirkin 
On Wed, Apr 5, 2017 at 1:12 PM, Gregory Hainaut
 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.

>
> 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 
> ---
>  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, , 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 = >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 @@

[Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string

2017-04-05 Thread Gregory Hainaut 
Context:
Nouveau uses NULL strings for unnamed parameter of texture gather
offsets opcode.

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 
---
 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, , 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 = >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