Re: [Piglit] [PATCH 2/2] shader_runner: Fix get_ints on 32-bit systems.

[Andres Gomez]

On Mon, 2016-06-06 at 16:02 -0700, Kenneth Graunke wrote:
> On Monday, June 6, 2016 3:56:09 PM PDT Mark Janes wrote:
> > 
> > Kenneth Graunke  writes:
> > 
> > > 
> > > The new ARB_vertex_attrib_64bit tests specify integer uniform
> > > values
> > > as hex, such as 0xc21620c5.  As an integer value, this is beyond
> > > LONG_MAX
> > > on 32-bit systems.  The intent is to parse it as an unsigned hex
> > > value and
> > > bitcast it.
> > > 
> > > However, we still need to handle parsing values with negative
> > > signs.
> > > 
> > > Using strtoll and truncating works.
> > > 
> > > Signed-off-by: Kenneth Graunke 
> > > ---
> > >  tests/shaders/shader_runner.c | 2 +-
> > >  tests/util/piglit-vbo.cpp | 4 ++--
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tests/shaders/shader_runner.c
> > > b/tests/shaders/shader_runner.c
> > > index 94c7826..56fd97c 100644
> > > --- a/tests/shaders/shader_runner.c
> > > +++ b/tests/shaders/shader_runner.c
> > > @@ -1398,7 +1398,7 @@ get_ints(const char *line, int *ints,
> > > unsigned count)
> > >   unsigned i;
> > >  
> > >   for (i = 0; i < count; i++)
> > > - ints[i] = strtol(line, (char **) , 0);
> > > + ints[i] = strtoll(line, (char **) , 0);
> > >  }
> > >  
> > >  
> > > diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-
> > > vbo.cpp
> > > index fd7e72a..50e6731 100644
> > > --- a/tests/util/piglit-vbo.cpp
> > > +++ b/tests/util/piglit-vbo.cpp
> > > @@ -387,8 +387,8 @@ vertex_attrib_description::parse_datum(const
> > > char **text, void *data) const
> > >   break;
> > >   }
> > >   case GL_INT: {
> > > - long value = (long) strtoll(*text, , 0);
> > > - if (errno == ERANGE) {
> > > + long long value = strtoll(*text, , 0);
> > > + if (errno == ERANGE || (unsigned long long)
> > > value > 0xull) {
> > ^^^
> > ^^
> > with this check removed, the series corrects all 32 bit failures
> > introduced by b7eb469, and is
> > 
> > Tested-by: Mark Janes 
> Right, the value > 0xull bit is bogus - the sign bit gets
> extended.  I've just dropped this locally.  It removes a bit of
> sanity
> checking for bogus values written in .shader_test files, but we don't
> sanity check most values, either.

Ugh! Sorry for this bug.

With that change, it LGTM.

I'm still working in these tests so I will come with a follow-up patch
to handle the correct parsing of the values with negative sign.

Reviewed-by: Andres Gomez 
-- 
-- 
Br,

Andres


___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/2] shader_runner: Fix get_ints on 32-bit systems.

[Antía Puentes]

On lun, 2016-06-06 at 16:02 -0700, Kenneth Graunke wrote:
> On Monday, June 6, 2016 3:56:09 PM PDT Mark Janes wrote:
> > 
> > Kenneth Graunke  writes:
> > 
> > > 
> > > The new ARB_vertex_attrib_64bit tests specify integer uniform
> > > values
> > > as hex, such as 0xc21620c5.  As an integer value, this is beyond
> > > LONG_MAX
> > > on 32-bit systems.  The intent is to parse it as an unsigned hex
> > > value and
> > > bitcast it.
> > > 
> > > However, we still need to handle parsing values with negative
> > > signs.
> > > 
> > > Using strtoll and truncating works.
> > > 
> > > Signed-off-by: Kenneth Graunke 
> > > ---
> > >  tests/shaders/shader_runner.c | 2 +-
> > >  tests/util/piglit-vbo.cpp | 4 ++--
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tests/shaders/shader_runner.c
> > > b/tests/shaders/shader_runner.c
> > > index 94c7826..56fd97c 100644
> > > --- a/tests/shaders/shader_runner.c
> > > +++ b/tests/shaders/shader_runner.c
> > > @@ -1398,7 +1398,7 @@ get_ints(const char *line, int *ints,
> > > unsigned count)
> > >   unsigned i;
> > >  
> > >   for (i = 0; i < count; i++)
> > > - ints[i] = strtol(line, (char **) , 0);
> > > + ints[i] = strtoll(line, (char **) , 0);
> > >  }
> > >  
> > >  
> > > diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-
> > > vbo.cpp
> > > index fd7e72a..50e6731 100644
> > > --- a/tests/util/piglit-vbo.cpp
> > > +++ b/tests/util/piglit-vbo.cpp
> > > @@ -387,8 +387,8 @@ vertex_attrib_description::parse_datum(const
> > > char **text, void *data) const
> > >   break;
> > >   }
> > >   case GL_INT: {
> > > - long value = (long) strtoll(*text, , 0);
> > > - if (errno == ERANGE) {
> > > + long long value = strtoll(*text, , 0);
> > > + if (errno == ERANGE || (unsigned long long)
> > > value > 0xull) {
> > ^^^
> > ^^
> > with this check removed, the series corrects all 32 bit failures
> > introduced by b7eb469, and is
> > 
> > Tested-by: Mark Janes 
> Right, the value > 0xull bit is bogus - the sign bit gets
> extended.  I've just dropped this locally.  It removes a bit of
> sanity
> checking for bogus values written in .shader_test files, but we don't
> sanity check most values, either.

Thanks for fixing this. Patches 1-2 are:

Reviewed-by: Antia Puentes 

___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/2] shader_runner: Fix get_ints on 32-bit systems.

[Kenneth Graunke]

On Monday, June 6, 2016 3:56:09 PM PDT Mark Janes wrote:
> Kenneth Graunke  writes:
> 
> > The new ARB_vertex_attrib_64bit tests specify integer uniform values
> > as hex, such as 0xc21620c5.  As an integer value, this is beyond LONG_MAX
> > on 32-bit systems.  The intent is to parse it as an unsigned hex value and
> > bitcast it.
> >
> > However, we still need to handle parsing values with negative signs.
> >
> > Using strtoll and truncating works.
> >
> > Signed-off-by: Kenneth Graunke 
> > ---
> >  tests/shaders/shader_runner.c | 2 +-
> >  tests/util/piglit-vbo.cpp | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> > index 94c7826..56fd97c 100644
> > --- a/tests/shaders/shader_runner.c
> > +++ b/tests/shaders/shader_runner.c
> > @@ -1398,7 +1398,7 @@ get_ints(const char *line, int *ints, unsigned count)
> > unsigned i;
> >  
> > for (i = 0; i < count; i++)
> > -   ints[i] = strtol(line, (char **) , 0);
> > +   ints[i] = strtoll(line, (char **) , 0);
> >  }
> >  
> >  
> > diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-vbo.cpp
> > index fd7e72a..50e6731 100644
> > --- a/tests/util/piglit-vbo.cpp
> > +++ b/tests/util/piglit-vbo.cpp
> > @@ -387,8 +387,8 @@ vertex_attrib_description::parse_datum(const char 
> > **text, void *data) const
> > break;
> > }
> > case GL_INT: {
> > -   long value = (long) strtoll(*text, , 0);
> > -   if (errno == ERANGE) {
> > +   long long value = strtoll(*text, , 0);
> > +   if (errno == ERANGE || (unsigned long long) value > 
> > 0xull) {
> ^
> with this check removed, the series corrects all 32 bit failures
> introduced by b7eb469, and is
> 
> Tested-by: Mark Janes 

Right, the value > 0xull bit is bogus - the sign bit gets
extended.  I've just dropped this locally.  It removes a bit of sanity
checking for bogus values written in .shader_test files, but we don't
sanity check most values, either.

--Ken


signature.asc
Description: This is a digitally signed message part.
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Re: [Piglit] [PATCH 2/2] shader_runner: Fix get_ints on 32-bit systems.

[Mark Janes]

Kenneth Graunke  writes:

> The new ARB_vertex_attrib_64bit tests specify integer uniform values
> as hex, such as 0xc21620c5.  As an integer value, this is beyond LONG_MAX
> on 32-bit systems.  The intent is to parse it as an unsigned hex value and
> bitcast it.
>
> However, we still need to handle parsing values with negative signs.
>
> Using strtoll and truncating works.
>
> Signed-off-by: Kenneth Graunke 
> ---
>  tests/shaders/shader_runner.c | 2 +-
>  tests/util/piglit-vbo.cpp | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
> index 94c7826..56fd97c 100644
> --- a/tests/shaders/shader_runner.c
> +++ b/tests/shaders/shader_runner.c
> @@ -1398,7 +1398,7 @@ get_ints(const char *line, int *ints, unsigned count)
>   unsigned i;
>  
>   for (i = 0; i < count; i++)
> - ints[i] = strtol(line, (char **) , 0);
> + ints[i] = strtoll(line, (char **) , 0);
>  }
>  
>  
> diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-vbo.cpp
> index fd7e72a..50e6731 100644
> --- a/tests/util/piglit-vbo.cpp
> +++ b/tests/util/piglit-vbo.cpp
> @@ -387,8 +387,8 @@ vertex_attrib_description::parse_datum(const char **text, 
> void *data) const
>   break;
>   }
>   case GL_INT: {
> - long value = (long) strtoll(*text, , 0);
> - if (errno == ERANGE) {
> + long long value = strtoll(*text, , 0);
> + if (errno == ERANGE || (unsigned long long) value > 
> 0xull) {
^
with this check removed, the series corrects all 32 bit failures
introduced by b7eb469, and is

Tested-by: Mark Janes 

>   printf("Could not parse as signed integer\n");
>   return false;
>   }
> -- 
> 2.8.3
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

[Piglit] [PATCH 2/2] shader_runner: Fix get_ints on 32-bit systems.

[Kenneth Graunke]

The new ARB_vertex_attrib_64bit tests specify integer uniform values
as hex, such as 0xc21620c5.  As an integer value, this is beyond LONG_MAX
on 32-bit systems.  The intent is to parse it as an unsigned hex value and
bitcast it.

However, we still need to handle parsing values with negative signs.

Using strtoll and truncating works.

Signed-off-by: Kenneth Graunke 
---
 tests/shaders/shader_runner.c | 2 +-
 tests/util/piglit-vbo.cpp | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
index 94c7826..56fd97c 100644
--- a/tests/shaders/shader_runner.c
+++ b/tests/shaders/shader_runner.c
@@ -1398,7 +1398,7 @@ get_ints(const char *line, int *ints, unsigned count)
unsigned i;
 
for (i = 0; i < count; i++)
-   ints[i] = strtol(line, (char **) , 0);
+   ints[i] = strtoll(line, (char **) , 0);
 }
 
 
diff --git a/tests/util/piglit-vbo.cpp b/tests/util/piglit-vbo.cpp
index fd7e72a..50e6731 100644
--- a/tests/util/piglit-vbo.cpp
+++ b/tests/util/piglit-vbo.cpp
@@ -387,8 +387,8 @@ vertex_attrib_description::parse_datum(const char **text, 
void *data) const
break;
}
case GL_INT: {
-   long value = (long) strtoll(*text, , 0);
-   if (errno == ERANGE) {
+   long long value = strtoll(*text, , 0);
+   if (errno == ERANGE || (unsigned long long) value > 
0xull) {
printf("Could not parse as signed integer\n");
return false;
}
-- 
2.8.3

___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit