Re: [Mesa-dev] [PATCH 1/2] glsl: add driconf to zero-init unintialized vars

[Eirik Byrkjeflot Anonsen]

Rob Clark  writes:

> On Tue, Jun 28, 2016 at 11:28 AM, Marek Olšák  wrote:
>> On Mon, Jun 27, 2016 at 9:28 PM, Rob Clark  wrote:
>>> On Mon, Jun 27, 2016 at 3:06 PM, Kenneth Graunke  
>>> wrote:
 On Monday, June 27, 2016 11:43:28 AM PDT Matt Turner wrote:
> On Mon, Jun 27, 2016 at 4:44 AM, Rob Clark  wrote:
> > On Mon, Jun 27, 2016 at 7:13 AM, Alan Swanson 
> >  wrote:
> >> On 2016-06-25 13:37, Rob Clark wrote:
> >>>
> >>> Some games are sloppy.. perhaps because it is defined behavior for DX 
> >>> or
> >>> perhaps because nv blob driver defaults things to zero.
> >>>
> >>> So add driconf param to force uninitialized variables to default to 
> >>> zero.
> >>>
> >>> This issue was observed with rust, from steam store.  But has surfaced
> >>> elsewhere in the past.
> >>>
> >>> Signed-off-by: Rob Clark 
> >>> ---
> >>> Note that I left out the drirc bit, since not entirely sure how to
> >>> identify this game.  (I don't actually have the game, just working off
> >>> of an apitrace)
> >>>
> >>> Possibly worth mentioning that for the shaders using uninitialized 
> >>> vars
> >>> having zero-initializers lets constant-propagation get rid of a whole
> >>> lot of instructions.  One shader I saw dropped to less than half of
> >>> it's original instruction count.
> >>
> >>
> >> If the default for uninitialised variables is undefined, then with the
> >> reported shader optimisations why bother with the (DRI) option when
> >> zeroing could still essentially be classed as undefined?
> >>
> >> Cuts the patch down to just the src/compiler/glsl/ast_to_hir.cpp 
> >> change.
> >
> > I did suggest that on #dri-devel, but Jason had a theoretical example
> > where it would hurt.. iirc something like:
> >
> >   float maybe_undef;
> >   for (int i = 0; i < some_uniform_at_least_one; i++)
> >  maybe_undef = ...
> >
> > also, he didn't want to hide shader bugs that app should fix.
> >
> > It would be interesting to rush shaderdb w/ glsl_zero_init=true and
> > see what happens, but I didn't get around to that yet.
>
> Here's what I get on i965. It's not a clear win.
>
> total instructions in shared programs: 5249030 -> 5249002 (-0.00%)
> instructions in affected programs: 28936 -> 28908 (-0.10%)
> helped: 66
> HURT: 132
>
> total cycles in shared programs: 57966694 -> 57956306 (-0.02%)
> cycles in affected programs: 1136118 -> 1125730 (-0.91%)
> helped: 78
> HURT: 106

 I suspect most of the help is because we're missing undef optimizations,
 such as CSE...while zero could be CSE'd.  (I have a patch, but it hurts
 things too...)
>>>
>>> right, I was thinking that treating undef as zero in constant-folding
>>> would have the same effect.. ofc it might make shader bugs less
>>> obvious.
>>>
>>> Btw, does anyone know what fglrx does?  Afaiu nv blob treats undef as
>>> zero.  If fglrx does the same, I suppose that strengthens the argument
>>> for "just do this unconditionally".
>>
>> No idea what fglrx does, but LLVM does eliminate code with undefined
>> inputs. Initializing everything to 0 might make that worse.
>
> hmm, treating as zero does eliminate a lot.. anyway, I guess we'll
> stick w/ driconf.
>
> fwiw, with some help from the reporter, we figured out that this is
> the bit that I need to squash into drirc:
>
> 
> 
> 

Not knowing a lot about drirc, I suspect you should have a double quote
at the end of glsl_zero_init as well?

eirik

> now, if I could talk somebody into a r-b for this and the i965 fix? ;-)
>
> BR,
> -R
> ___
> 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

Re: [Mesa-dev] [PATCH 4/6] gallium/u_queue: add an option to name threads

[Eirik Byrkjeflot Anonsen]

Nicolai Hähnle  writes:

> On 21.06.2016 16:50, Marek Olšák wrote:
>> On Tue, Jun 21, 2016 at 4:40 PM, Nicolai Hähnle  wrote:
>>> On 21.06.2016 14:17, Marek Olšák wrote:

 From: Marek Olšák 

 for debugging
 ---
src/gallium/auxiliary/util/u_queue.c  | 10 ++
src/gallium/auxiliary/util/u_queue.h  |  2 ++
src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c |  2 +-
src/gallium/winsys/radeon/drm/radeon_drm_winsys.c |  2 +-
4 files changed, 14 insertions(+), 2 deletions(-)

 diff --git a/src/gallium/auxiliary/util/u_queue.c
 b/src/gallium/auxiliary/util/u_queue.c
 index d14d850..a0b0317 100644
 --- a/src/gallium/auxiliary/util/u_queue.c
 +++ b/src/gallium/auxiliary/util/u_queue.c
 @@ -26,6 +26,7 @@

#include "u_queue.h"
#include "u_memory.h"
 +#include "u_string.h"
#include "os/os_time.h"

static void
 @@ -61,6 +62,13 @@ static PIPE_THREAD_ROUTINE(util_queue_thread_func,
 input)

   FREE(input);

 +   if (queue->name) {
 +  char name[16] = {0};
 +  util_snprintf(name, sizeof(name) - 1, "%s:%i",
 +queue->name, thread_index);
>>>
>>>
>>> It should be safe to just say util_snprintf(name, sizeof(name), ...) without
>>> initializing name.
>>
>> It's not. snprintf doesn't write '\0' if the output string length is >= size.
>
> Not sure if there was ever some variant that didn't, but both the 
> manpage and a quick test agree that it does.
>

I'm pretty sure snprintf() is safe in this way. I think it is only
strncpy and strncat that are broken.

strncpy() never NUL-terminates anything, but it fills the unused part of
the destination with NULs, so if the source is smaller than the
destination, then the result will "accidentally" be NUL-terminated
anyway.

strncat() on the other hand. If you see it used, it is probably a bug.

Hmm, that makes me curious:

$ git grep strncat
src/compiler/glsl/glcpp/pp.c:   ralloc_strncat(, 
shader,
src/compiler/glsl/glcpp/pp.c:   ralloc_strncat(, shader, 
backslash - shader);
src/gallium/auxiliary/gallivm/lp_bld_printf.c:  util_strncat(format, 
type_fmt, sizeof(format) - strlen(format) - 1);
src/gallium/auxiliary/gallivm/lp_bld_printf.c: util_strncat(format, 
type_fmt, sizeof(format) - strlen(format) - 1);
src/gallium/auxiliary/gallivm/lp_bld_printf.c:   util_strncat(format, "\n", 
sizeof(format) - strlen(format) - 1);
src/gallium/auxiliary/util/u_debug.c:   util_strncat(output, "|", 
sizeof(output) - strlen(output) - 1);
src/gallium/auxiliary/util/u_debug.c:util_strncat(output, names->name, 
sizeof(output) - strlen(output) - 1);
src/gallium/auxiliary/util/u_debug.c:util_strncat(output, "|", 
sizeof(output) - strlen(output) - 1);
src/gallium/auxiliary/util/u_debug.c:  util_strncat(output, rest, 
sizeof(output) - strlen(output) - 1);
src/gallium/auxiliary/util/u_string.h:util_strncat(char *dst, const char *src, 
size_t n)
src/gallium/auxiliary/util/u_string.h:#define util_strncat strncat
src/mesa/main/atifragshader.c:  strncat(ret_str, "|2X", 1024);
src/mesa/main/atifragshader.c:  strncat(ret_str, "|4X", 1024);
src/mesa/main/atifragshader.c:  strncat(ret_str, "|8X", 1024);
src/mesa/main/atifragshader.c:  strncat(ret_str, "|HA", 1024);
src/mesa/main/atifragshader.c:  strncat(ret_str, "|QU", 1024);
src/mesa/main/atifragshader.c:  strncat(ret_str, "|EI", 1024);
src/mesa/main/atifragshader.c:  strncat(ret_str, "|SAT", 1024);
src/mesa/main/atifragshader.c:  strncat(ret_str, "NONE", 1024);
src/util/ralloc.c:/* helper routine for strcat/strncat - n is the exact amount 
to copy */
src/util/ralloc.c:ralloc_strncat(char **dest, const char *str, size_t n)
src/util/ralloc.h:bool ralloc_strncat(char **dest, const char *str, size_t n);

Wow, looks like most of these are actually used correctly. I think
that's the first time I have seen that. But all of the ones in
atifragshader.c are surely wrong.

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] clover: Fix build against clang SVN >= r273191

[Eirik Byrkjeflot Anonsen]

Vedran Miletić  writes:

> On 06/21/2016 03:13 AM, Michel Dänzer wrote:
>> On 21.06.2016 08:17, Vedran Miletić wrote:
[ ... ]
>>
>> P.S. I recommend adding your Signed-off-by for patches you want to be
>> applied.
>>
>
> Will do. Thanks for explaining what that is used for.
>
> Regards,
> Vedran


The signed-off-by tag typically signifies that you agree to have your
contribution be part of the project and subject to whatever licensing
policies the project may have AND (really important!) that you have the
right to make such an agreement.

It was started by the linux kernel people, and they have a very specific
meaning attached to it. See
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=HEAD
part 11.

Other projects could choose a different meaning. I expect most projects
treat it as "kind of like the linux kernel thing" in a somewhat
hand-wavy manner.

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] About tolerance calculation on specific (builtin) functions

[Eirik Byrkjeflot Anonsen]

Andres Gomez  writes:

> Hi,
>
> as part of the work done to "Add FP64 support to the i965 shader
> backends" at:
> https://bugs.freedesktop.org/show_bug.cgi?id=92760
>
> I've been working to add piglit tests that would check the new features
> added by this addition.
>
> Due to this, I've been checking and making modifications into the
> builtin_functions*py module used by some generators. These modules use
> automatic calculation of the tolerance when distance checking the
> result of a function.
>
> As already stated in the good documentation of the module, the
> tolerance is computed following what it is stated in OpenGL's specs:
>
> From the OpenGL 1.4 spec (2.1.1 "Floating-Point Computation"):
>
>   "We require simply that numbers' floating-point parts contain
>   enough bits ... so that individual results of floating-point
>   operations are accurate to about 1 part in 10^5."
>
> Although the text is open to interpretation, and for specific
> operations we take a little bit more flexible approach, basically, the
> tolerance is calculated as:
>
> tolerance =  / 10⁵
>
> This makes sense since the precision of a floating point value gets
> reduced while the number gets bigger[1].
>
> Following this approach, for a number in the order of 40*10⁵, the
> tolerance used is ~40. While this should be OK for most of the
> functions, it seems to me that such a high tolerance should not be used
> with specific functions, if any tolerance should be used at all.
>
> For example, when testing the "sign()" function, seems pretty obvious
> that using a value of 40 in the tolerance of a function that should
> return either 1.0, 0.0 or -1.0 doesn't make much sense.

That's an interesting interpretation. I would assume that a tolerance of
1 part in 10^5 for any of these three values would be somewhat less than
40 :)

Although that does bring up an interesting point: To get things correct,
probably the tolerance has to apply to all values in the chain, both the
inputs, outputs and any intermediates. Consider for example the case of

a = 40 * 10^5 + 1
b = 40 * 10^5 - 1
c = a - b

What is the correct tolerance for c?

eirik

> A similar case is the "trunc" function and probably others, like
> "floor", "ceil", "abs", etc.
>
> My conclusion is that it should be safe to assume no tolerance in this
> functions and I could modify the algorithm used for them in the python
> module but I wanted to have some feedback in case I'm not taking into
> account something that advices against doing these modifications.
>
> Opinions?
>
> [1] https://en.wikipedia.org/wiki/IEEE_floating_point#Basic_and_interch
> ange_formats
> -- 
> Br,
>
> Andres
>
> ___
> 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

Re: [Mesa-dev] [PATCH 03/10] i965: Silence loop counter overflow warning

[Eirik Byrkjeflot Anonsen]

Ian Romanick  writes:

> From: Ian Romanick 
>
> I don't understand why the old code was bad, but the new code is fine.

Probably because the *loop counter* can no longer overflow. Thus the
loop can be optimized. The fact that "i" might overflow has become
irrelevant to the warning.

(And from that perspective, it isn't equivalent. If "i" overflows in the
original code, you would get an infinite loop.)

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/tests: fix build with clang compiler

[Eirik Byrkjeflot Anonsen]

Samuel Pitoiset  writes:

> Nested functions are supported as an extension in GNU C, but Clang
> don't support them.
>
> This fixes compilation errors when (manually) building compute.c,
> or by setting --enable-gallium-tests to the configure script.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75165
>
> Signed-off-by: Samuel Pitoiset 
> ---
>  src/gallium/tests/trivial/compute.c | 643 
> +---
>  1 file changed, 370 insertions(+), 273 deletions(-)
>
> diff --git a/src/gallium/tests/trivial/compute.c 
> b/src/gallium/tests/trivial/compute.c
> index bcdfb11..4cb32e5 100644
> --- a/src/gallium/tests/trivial/compute.c
> +++ b/src/gallium/tests/trivial/compute.c
> @@ -428,6 +428,35 @@ static void launch_grid(struct context *ctx, const uint 
> *block_layout,
>  pipe->launch_grid(pipe, block_layout, grid_layout, pc, input);
>  }
>  
> +/* test_system_values */
> +static void test_system_values_init(void *p, int s, int x, int y)
> +{
> +*(uint32_t *)p = 0xdeadbeef;
> +}
> +
> +static void test_sytem_values_expect(void *p, int s, int x, int y)

s/sytem/system/, I assume?

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] util: implement strndup for WIN32

[Eirik Byrkjeflot Anonsen]

Samuel Iglesias Gonsalvez  writes:

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92124
> Cc: Jose Fonseca 
> ---
>
> I tested it on MSVC but not MinGW. I hope I did not something wrong.
>
>  src/mesa/main/shader_query.cpp |  1 +
>  src/util/Makefile.sources  |  1 +
>  src/util/strndup.c | 47 
> ++
>  src/util/strndup.h | 28 +
>  4 files changed, 77 insertions(+)
>  create mode 100644 src/util/strndup.c
>  create mode 100644 src/util/strndup.h
>
> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
> index e020dce..0cada50 100644
> --- a/src/mesa/main/shader_query.cpp
> +++ b/src/mesa/main/shader_query.cpp
> @@ -37,6 +37,7 @@
>  #include "../glsl/program.h"
>  #include "uniforms.h"
>  #include "main/enums.h"
> +#include "util/strndup.h"
>  
>  extern "C" {
>  #include "shaderapi.h"
> diff --git a/src/util/Makefile.sources b/src/util/Makefile.sources
> index afdd0cb..10f2c02 100644
> --- a/src/util/Makefile.sources
> +++ b/src/util/Makefile.sources
> @@ -17,6 +17,7 @@ MESA_UTIL_FILES :=  \
>   set.c \
>   set.h \
>   simple_list.h \
> + strndup.c \
>   strtod.c \
>   strtod.h \
>   texcompress_rgtc_tmp.h \
> diff --git a/src/util/strndup.c b/src/util/strndup.c
> new file mode 100644
> index 000..6e5bb22
> --- /dev/null
> +++ b/src/util/strndup.c
> @@ -0,0 +1,47 @@
> +/*
> + * Copyright (c) 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#if defined(_WIN32)
> +#include 
> +#include "strndup.h"
> +
> +char *
> +strndup(const char *str, size_t max)
> +{
> +   size_t n;
> +   char *ptr;
> +
> +   if (str == NULL)
> +  return NULL;
> +
> +   n = strlen(str);
> +   if (n > max)
> +  n = max;
> +
> +   ptr = (char *) malloc((n + 1) * sizeof(char));

In C++, I'm pretty sure sizeof(char) is always 1 (a 'char' is defined to
be of 'byte' size, and sizeof() returns size in 'bytes'. On the other
hand, the size of a 'byte' is not specified by C++.) But it could be
that C is of a different opinion.

However, there is a possibility for the calculation of the size to
overflow, which would most likely end up allocating a small buffer and
then overflow that buffer in the following memcpy. Of course, if
sizeof(char) is 1, that would probably never do anything worse than
crashing :)

eirik

> +   memcpy(ptr, str, n);
> +   ptr[n] = '\0';
> +   return ptr;
> +}
> +
> +#endif
> diff --git a/src/util/strndup.h b/src/util/strndup.h
> new file mode 100644
> index 000..dc8cdd2
> --- /dev/null
> +++ b/src/util/strndup.h
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (c) 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 

Re: [Mesa-dev] [PATCH v3] i915: fixing driver crashes if too few vertices are submitted

[Eirik Byrkjeflot Anonsen]

Ilia Mirkin  writes:

> On Fri, Sep 11, 2015 at 10:45 PM, Ian Romanick  wrote:
>> On 09/11/2015 10:55 AM, Ilia Mirkin wrote:
>>> On Fri, Sep 11, 2015 at 12:36 PM, Marius Predut  
>>> wrote:
 Comparison with a signed expression and unsigned value
 is converted to unsigned value, reason for minus value is interpreted
 as a big unsigned value. For this case the "for" loop
 is going into unexpected behavior.

 v1: Brian Paul: code style fix.
 v2: Ian Romanick: glDrawArrays(GL_QUADS, 0, (n * 4) + k) fail , k < 4.

 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
 Signed-off-by: Marius Predut 
 ---
  src/mesa/tnl_dd/t_dd_dmatmp.h | 7 +++
  1 file changed, 7 insertions(+)

 diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
 index 7be3954..f99d977 100644
 --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
 +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
 @@ -627,6 +627,13 @@ static void TAG(render_quads_verts)( struct 
 gl_context *ctx,
LOCAL_VARS;
GLuint j;

 +  /* Page 18 (page 32 of the PDF) of the OpenGL 2.1 spec says:
 +   * The total number of vertices between Begin and End is 4n + k,
 +   * where 0 ≤ k ≤ 3; if k is not zero, the final k vertices are  
 ignored.
 +   */
 +  count = (count / 4) * 4;
>>>
>>> Might be just me, but I'd find
>>>
>>> count &= ~0x3
>>>
>>> to be a lot clearer. Don't know if the compiler can make such an
>>> optimization.
>>
>> I think it can if count is unsigned.  Of course, GLsizei is not
>> unsigned.  It is already invalid for count < 0, so your optimization is
>> safe.
>
> Actually count is a GLuint, so you're probably right that the compiler
> can work it out. I definitely have to think about what it's doing
> though, whereas with something like & ~3 it's pretty obvious. Perhaps
> I've been in bit-land too long.
>
>>
>>> However this seems wrong... you're supposed to draw
>>> start..count, so that's the value that has to be div-by-4. Further up,
>>> when there's native quad support, the logic does:
>>
>> I don't think that's right.  Count is the number of vertices, not the
>> index of the last vertex.  Calling
>>
>> glDrawArrays(GL_QUADS, 47000, 4);
>>
>> still draws one quad.
>>
>> Look at the pseudocode on page 28 (page 42 of the PDF) of the OpenGL 2.1
>> spec:
>>
>> "The command
>>
>> void DrawArrays(enum mode, int first, sizei count);
>>
>> constructs a sequence of geometric primitives using elements first
>> through first + count − 1 of each enabled array. mode specifies
>> what kind of primitives are constructed; it accepts the same token
>> values as the mode parameter of the Begin command. The effect of
>>
>> DrawArrays(mode, first, count);
>>
>> is the same as the effect of the command sequence
>>
>> if (mode or count is invalid)
>> generate appropriate error
>> else {
>> Begin(mode);
>> for (int i = 0; i < count; i++)
>> ArrayElement(first + i);
>> End();
>> }"
>>
>> Combining that with the language previously quoted, I think this change
>> is right.
>
> Well, the code in question is
>
>   for (j = start; j < count-3; j += 4) {
>  void *tmp = ALLOC_VERTS( 6 );
>  /* Send v0, v1, v3
>   */
>  tmp = EMIT_VERTS(ctx, j, 2, tmp);
>  tmp = EMIT_VERTS(ctx, j + 3, 1, tmp);
>  /* Send v1, v2, v3
>   */
>  tmp = EMIT_VERTS(ctx, j + 1, 3, tmp);
>  (void) tmp;
>   }
>
> If count worked the way you're suggesting, then this would never work
> for start != 0. I think "count" is really "end" in this case. Here is
> one of the callers of this function:
>
> tab[prim & PRIM_MODE_MASK]( ctx, start, start + length, prim );
>
> The fact that the variable is called 'count' is actively misleading of
> course, but that doesn't make the code any more right. The HAVE_QUADS
> and HAVE_ELTS cases both have:
>
>   count -= (count-start)%4;
>
> which I believe further confirms my analysis.

So we have three main branches and one failure branch, and all three
main branches tries to do the exact same thing (looping from start to
count in steps of 4). Maybe the common stuff should be moved out of the
branches?

(Actually, the first two also differ in their style of setting dmasz and
currentsz to a multiple of 4. Maybe they should be made consistent for
easier reading?)

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v1] i915: fixing driver crashes if too few vertices are submitted

[Eirik Byrkjeflot Anonsen]

Marius Predut  writes:

> Comparison with a signed expression and unsigned value
> is converted to unsigned value, reason for minus value is interpreted
> as a big unsigned value. For this case the "for" loop
> is going into unexpected behavior.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38109
> Signed-off-by: Marius Predut 
> ---
>  src/mesa/tnl_dd/t_dd_dmatmp.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/tnl_dd/t_dd_dmatmp.h b/src/mesa/tnl_dd/t_dd_dmatmp.h
> index 7be3954..88ecc78 100644
> --- a/src/mesa/tnl_dd/t_dd_dmatmp.h
> +++ b/src/mesa/tnl_dd/t_dd_dmatmp.h
> @@ -627,6 +627,8 @@ static void TAG(render_quads_verts)( struct gl_context 
> *ctx,
>LOCAL_VARS;
>GLuint j;
>  
> +  if(count%4 != 0) return;
> +

Seems to me that does a bit more than just fixing the crash. I would
think

if (count < 4)

would fix the crash only. (And then quite possibly you won't need the
next hunk?) But I have no idea whether the side effect is desired.

(Actually, what if count is 0? Or is that impossible?)

eirik

>INIT(GL_TRIANGLES);
>  
>for (j = start; j < count-3; j += 4) {
> @@ -1248,7 +1250,7 @@ static GLboolean TAG(validate_render)( struct 
> gl_context *ctx,
>   ok = (GLint) count < GET_SUBSEQUENT_VB_MAX_ELTS();
>}
>else {
> - ok = HAVE_TRIANGLES; /* flatshading is ok. */
> + ok = HAVE_TRIANGLES && (count%4 == 0); /* flatshading is ok. */
>}
>break;
>default:
> -- 
> 1.9.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Don't check for draw-time errors that cannot occur in core profile

[Eirik Byrkjeflot Anonsen]

Ilia Mirkin  writes:

> On Tue, Sep 1, 2015 at 12:15 PM, Ian Romanick  wrote:
>> For a bunch of the small changes, I don't care too much what the
>> difference is.  I just want to know whether after is better than before.
>
> And that gets back to my comment that you can't *measure* the impact
> of a change. Not with something where the outcome is a random
> variable. It can't be done.
>
> All you can do is answer the question "is X's mean more than N higher
> than Y's mean". And you change the number of trials in an experiment
> depending on N. (There's also more advanced concepts like 'power' and
> whatnot, I've done just fine without fully understanding them, I
> suspect you can too.)

Power is (IIRC) just an opposite of the p-value. That is, the p-value
gives you the probability of false positives and the power is the
probability of false negatives (or rather 1-power, but whatever). The
complication is that you usually choose the p-value (typically 0.05)
while the power has to be calculated.

> As an aside, increasing the number of trials until you get a
> significant result is a great way to arrive at incorrect decisions,
> due to the multi-look problem (95% CI means 1/20 gives you bad
> results). The proper way is to decide beforehand "I care about changes
>>0.1%, which means I need to run 5000 trial runs"

The trick could be to run a sequence of tests until you find how many
trials are needed for significance. Then you can check if you get
repeatable results with that many trials, in which case you are safe.

The key word is of course "repeatable". If you have a correctly executed
test that gives repeatable "significant difference", it usually doesn't
matter too much how you figured out which parameters were needed.
("usually", because you could run into a choice of parameters that
invalidates the whole test. But just increasing the number of trials
shouldn't do that.)

Which brings us to the clear value of multiple people running similar
tests and getting similar results. That strengthens the conclusion
significantly.

> (based on the
> assumption that 50 runs gets you 1%). After doing the 5k runs, your CI
> width should be ~0.1% and you should then be able to see if the delta
> in means is higher or lower than that. If it's higher, then you've
> detected a significant change. If it's not, that btw doesn't mean "no
> change", just not statistically significant. There's also a procedure
> for the null hypothesis (i.e. is a change's impact <1%) which is
> basically the same thing but involves doing a few more runs (like 50%
> more? I forget the details).

Hmm, you could just formulate your null hypothesis as "the change is
greater than 1%" and then test that normally.

> Anyways, I'm sure I've bored everyone to death with these pedantic
> explanations, but IME statistics is one of the most misunderstood
> areas of math, especially among us engineers.
>
>   -ilia

What, statistics boring? No way! :)

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Don't check for draw-time errors that cannot occur in core profile

[Eirik Byrkjeflot Anonsen]

Ilia Mirkin <imir...@alum.mit.edu> writes:

> On Tue, Sep 1, 2015 at 1:48 AM, Eirik Byrkjeflot Anonsen
> <ei...@eirikba.org> wrote:
>> Ian Romanick <i...@freedesktop.org> writes:
>>
>>> ping. :)
>>>
>>> On 08/10/2015 11:48 AM, Matt Turner wrote:
>>>> On Mon, Aug 10, 2015 at 10:12 AM, Ian Romanick <i...@freedesktop.org> 
>>>> wrote:
>>>>> From: Ian Romanick <ian.d.roman...@intel.com>
>>>>>
>>>>> On many CPU-limited applications, this is *the* hot path.  The idea is
>>>>> to generate per-API versions of brw_draw_prims that elide some checks.
>>>>> This patch removes render-mode and "is everything in VBOs" checks from
>>>>> core-profile contexts.
>>>>>
>>>>> On my IVB laptop (which may have experienced thermal throttling):
>>>>>
>>>>> Gl32Batch7: 3.70955% +/- 1.11344%
>>>>
>>>> I'm getting 3.18414% +/- 0.587956% (n=113) on my IVB, , which probably
>>>> matches your numbers depending on your value of n.
>>>>
>>>>> OglBatch7:  1.04398% +/- 0.772788%
>>>>
>>>> I'm getting 1.15377% +/- 1.05898% (n=34) on my IVB, which probably
>>>> matches your numbers depending on your value of n.
>>>
>>> This is another thing that make me feel a little uncomfortable with the
>>> way we've done performance measurements in the past.  If I run my test
>>> before and after this patch for 121 iterations, which I have done, I can
>>> cut the data at any point and oscillate between "no difference" or X%
>>> +/- some-large-fraction-of-X%.  Since the before and after code for the
>>> compatibility profile path should be identical, "no difference" is the
>>> only believable result.
>>
>> That's pretty much expected, I believe. In essence, you are running 121
>> tests, each with a 95% confidence interval and so should expect
>> somewhere around 5 "significant difference" results. That's not entirely
>> true of course, since these are not 121 *independent* tests, but the
>> basic problem remains.
>
> (more stats rants follow)

:)

> While my job title has never been 'statistician', I've been around a
> bunch of them. Just want to correct this... let's forget about these
> tests, but instead think about coin flips (of a potentially unfair
> coin). What you're doing is flipping the coin 100 times, and then
> looking at the number of times it came up heads and tails. From that
> you're inferring the mean of the distribution.
[...]

(I have a background in mathematics with a small amount of both
probability theory and statistics, but I haven't really worked with it,
so your background may make you more of a "statistician" than me :) )

I think what Ian was saying was that he was flipping the coin 100 times
and then after every flip checking whether the result so far suggested a
50/50 result (fair coin) or not. And he found that sometimes during the
run he would get a "fair coin" result, which he thought was in conflict
with the final "loaded coin" result. Thus he was questioning whether the
final "loaded coin" result was correct.

I was simplifying heavily to point out the problem with this particular
way of looking at the result.

Now, if I understood Ian's comment correctly, his main reason for
doubting the "loaded coin" result was that he thought there was no code
differences to explain the result. Which is a very good reason to
suspect a problem somewhere. I'm just saying that looking at partial
results of a full test run doesn't invalidate the result of the full
test run. More importantly, looking at partial results of a test run
does not provide any more information on the "truth" than the result of
the full test run (again a simplification, but only if you really know
what you're doing).

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: Don't check for draw-time errors that cannot occur in core profile

[Eirik Byrkjeflot Anonsen]

Ian Romanick  writes:

> ping. :)
>
> On 08/10/2015 11:48 AM, Matt Turner wrote:
>> On Mon, Aug 10, 2015 at 10:12 AM, Ian Romanick  wrote:
>>> From: Ian Romanick 
>>>
>>> On many CPU-limited applications, this is *the* hot path.  The idea is
>>> to generate per-API versions of brw_draw_prims that elide some checks.
>>> This patch removes render-mode and "is everything in VBOs" checks from
>>> core-profile contexts.
>>>
>>> On my IVB laptop (which may have experienced thermal throttling):
>>>
>>> Gl32Batch7: 3.70955% +/- 1.11344%
>> 
>> I'm getting 3.18414% +/- 0.587956% (n=113) on my IVB, , which probably
>> matches your numbers depending on your value of n.
>> 
>>> OglBatch7:  1.04398% +/- 0.772788%
>> 
>> I'm getting 1.15377% +/- 1.05898% (n=34) on my IVB, which probably
>> matches your numbers depending on your value of n.
>
> This is another thing that make me feel a little uncomfortable with the
> way we've done performance measurements in the past.  If I run my test
> before and after this patch for 121 iterations, which I have done, I can
> cut the data at any point and oscillate between "no difference" or X%
> +/- some-large-fraction-of-X%.  Since the before and after code for the
> compatibility profile path should be identical, "no difference" is the
> only believable result.

That's pretty much expected, I believe. In essence, you are running 121
tests, each with a 95% confidence interval and so should expect
somewhere around 5 "significant difference" results. That's not entirely
true of course, since these are not 121 *independent* tests, but the
basic problem remains.

You need to decide up front how many iterations you will run before
looking at the result. And you will still sometimes get the "wrong"
result. That's statistics for you.

Now, as Ilia said, this depends on the distribution to be normal, which
it is emphatically not. But figuring out what effect that has on your
results will take a proper statistician, which I am not :)

Of course, an obvious improvement would be to always run the test twice
and if any of those gives you a close-to-no-difference result, run it
another time. If both (or all three) runs give you very similar results,
you would have much better confidence in your result.

In this case you would want to run both/all tests with the same number
of iterations to get comparable numbers. But if those tests give
close-to-no-difference result, it would also be possible to run another
test with more iterations, which should tighten up the confidence
interval (and thus give a more certain result).

> Using a higher confidence threshold (e.g., -c 98) results in "no
> difference" throughout, as expected.  I feel like 90% isn't a tight
> enough confidence interval for a lot of what we do, but I'm unsure how
> to determine what confidence level we should use.  We could
> experimentally determine it by running a test some number of times and
> finding the interval that detects no change in some random partitioning
> of the test results.  Ugh.

But that only attacks half the problem. You also want differences to be
flagged when there really is one. So you would also have to work out
which level of actual, real improvements you are happy about discarding
as "no difference".

Another "obvious" way to improve the results would be to use a better
analysis of the data than what is currently done. But that brings me
back to the "I'm not a statistician" bit.

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] glsls: Modify exec_list to avoid strict-aliasing violations

[Eirik Byrkjeflot Anonsen]

Francisco Jerez curroje...@riseup.net writes:

 Erik Faye-Lund kusmab...@gmail.com writes:

 On Fri, Jun 26, 2015 at 4:01 PM, Francisco Jerez curroje...@riseup.net 
 wrote:
 Davin McCall dav...@davmac.org writes:

 On 26/06/15 14:31, Eirik Byrkjeflot Anonsen wrote:
 Erik Faye-Lund kusmab...@gmail.com writes:

 On Fri, Jun 26, 2015 at 1:23 PM, Davin McCall dav...@davmac.org wrote:
 On 26/06/15 12:03, Davin McCall wrote:
 ... The stored value of 'n' is not accessed by any other type than the
 type of n itself. This value is then cast to a different pointer type. 
 You
 are mistaken if you think that the cast accesses the stored value of 
 n. The
 other stored value access that it occurs in that expression is to the
 object pointed at by the result of the cast. [...]:

 I'm sorry, I think that was phrased somewhat abrasively, which I did not
 intend. Let me try this part again. If we by break up the expression in
 order of evaluation:

 From:
 return ((const struct exec_node **)n)[0]

 In order of evaluation:

 n
 - which accesses the stored value of n, i.e. a value of type 'struct 
 exec
 node *', via n, which is obviously of that type.

 (const struct exec_node **)n
   - which casts that value, after it has been retrieved, to another 
 type. If
 this were an aliasing violation, then casting any pointer variable to
 another type would be an aliasing violation; this is clearly not the 
 case.

 ((const struct exec_node **)n)[0]
 - which de-references the result of the above cast, thereby accessing a
 stored value of type 'exec node *' using a glvalue of type 'exec node 
 *'.
 I think breaking this up is a mistake, because the strict-aliasing
 rules is explicitly about the *combination* of these two things.

 You *are* accessing the underlying memory of 'n' through a different
 type, and this is what strict aliasing is all about. But it takes two
 steps, a single step isn't enough to do so.

 Those other spec-quotes doesn't undo the strict-aliasing definitions;
 knowing how things are laid out in memory doesn't mean the compiler
 cannot assume two differently typed variables doesn't overlap.
 So basically, you're saying that e.g.:

 p-next = a;
 q = exec_node_get_next_const(p);

 is equivalent to:

 exec_node * p1 = p;
 exec_node ** p2 = (exec_node**)p;
 p1-next = a;
 q = p2[0];

 It is, once the patch is applied (or if strict aliasing is disabled).

 And at this point p1 and p2 are different types, so the compiler can
 freely assume that p1 and p2 are non-overlapping.

 p1 and p2 are two separate variables and of course they are
 non-overlapping, but *p1 and **p2 are the same type and so may overlap.

 Also note that even *p1 and *p2 are allowed to overlap even though they
 are of different types because of section 6.5 of C99:

 | 7 An object shall have its stored value accessed only by an lvalue
 |   expression that has one of the following types:
 |[...]
 |- an aggregate or union type that includes one of the aforementioned
 |  types among its members (including, recursively, a member of a
 |  subaggregate or contained union)[...]

 I don't see how this wording legitimates the code above. There's no
 unions involved, so that part is irrelevant.

 Yeah, only the aggregate part is relevant.

 And n isn't an aggregate type, it's a pointer type that happens to
 point to an aggregate type, no? But even if it were, it needs to
 include one of the aforementioned types among its members, which I
 cannot see that it does either.

 Care to explain?

 In the example this was replying to, *p1 was an lvalue of aggregate type
 (struct exec_node), and *p2 (or equivalently p2[0]) was an lvalue of
 type struct exec_node *.  The latter is a type compatible with the
 effective type of the object, the object being the next member of an
 aggregate object of type struct exec_node, hence the text quoted above
 applies and *p1 and *p2 may legitimately alias the same object.

The argument makes sense. As I said, I haven't been carefully thinking
about aliasing issues, and I have a feeling that there is obviously
correct code that probably requires this to work. If my example was
broken, it would probably break stuff that shouldn't break.

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] glsls: Modify exec_list to avoid strict-aliasing violations

[Eirik Byrkjeflot Anonsen]

Davin McCall dav...@davmac.org writes:

 On 26/06/15 14:31, Eirik Byrkjeflot Anonsen wrote:
 Erik Faye-Lund kusmab...@gmail.com writes:

 On Fri, Jun 26, 2015 at 1:23 PM, Davin McCall dav...@davmac.org wrote:
 On 26/06/15 12:03, Davin McCall wrote:
 ... The stored value of 'n' is not accessed by any other type than the
 type of n itself. This value is then cast to a different pointer type. You
 are mistaken if you think that the cast accesses the stored value of n. 
 The
 other stored value access that it occurs in that expression is to the
 object pointed at by the result of the cast. [...]:

 I'm sorry, I think that was phrased somewhat abrasively, which I did not
 intend. Let me try this part again. If we by break up the expression in
 order of evaluation:

 From:
 return ((const struct exec_node **)n)[0]

 In order of evaluation:

 n
 - which accesses the stored value of n, i.e. a value of type 'struct exec
 node *', via n, which is obviously of that type.

 (const struct exec_node **)n
   - which casts that value, after it has been retrieved, to another type. 
 If
 this were an aliasing violation, then casting any pointer variable to
 another type would be an aliasing violation; this is clearly not the case.

 ((const struct exec_node **)n)[0]
 - which de-references the result of the above cast, thereby accessing a
 stored value of type 'exec node *' using a glvalue of type 'exec node *'.
 I think breaking this up is a mistake, because the strict-aliasing
 rules is explicitly about the *combination* of these two things.

 You *are* accessing the underlying memory of 'n' through a different
 type, and this is what strict aliasing is all about. But it takes two
 steps, a single step isn't enough to do so.

 Those other spec-quotes doesn't undo the strict-aliasing definitions;
 knowing how things are laid out in memory doesn't mean the compiler
 cannot assume two differently typed variables doesn't overlap.
 So basically, you're saying that e.g.:

 p-next = a;
 q = exec_node_get_next_const(p);

 is equivalent to:

 exec_node * p1 = p;
 exec_node ** p2 = (exec_node**)p;
 p1-next = a;
 q = p2[0];

 It is, once the patch is applied (or if strict aliasing is disabled).

 And at this point p1 and p2 are different types, so the compiler can
 freely assume that p1 and p2 are non-overlapping.

 p1 and p2 are two separate variables and of course they are 
 non-overlapping, but *p1 and **p2 are the same type and so may overlap.

Hmm, yes unclear language on my side. I meant that *p1 and *p2 are
known to be non-overlapping due to the type difference.

   Thus the two
 assignments can be safely reordered. Sounds plausible to me.

 The assignments are to 'p1-next' and 'p2[0]', which *are* the same type 
 (struct exec_node *) and therefore the assignments *cannot* be 
 reordered. It is exactly this that I rely on in my patch to resolve the 
 aliasing issue with the current code.

But, on the other hand, if *p1 and *p2 are known to be non-overlapping,
it would mean that p1-next and p2[0] are non-overlapping. Thus the two
statements can be reordered. Though I'd actually have to carefully read
the spec to check whether a compiler would be allowed to do that.

 And note that casting via void* won't help. p == (void*)p compares a
 variable of type exec_node* to a variable of type void*, and thus
 there's no strict-aliasing problem. But p == (exec_node**)(void*)p
 compares an exec_node* to an exec_node** and thus the compiler can
 assume that they are not the same.

 The compiler cannot assume pointers are not the same based on their 
 type, unless the pointers are de-referenced, which they are not in the 
 example you just gave. Strictly speaking C99 doesn't even allow the 
 comparison of 'p == (exec_node**)(void*)p', but all the compilers I know 
 of allow it, and AFAIK give the same result as if one operand were cast 
 to the same type as the other.

 Davin

Oops, yes. Really badly worded example :)

I did intend some form of dereference there to make sure that the fact
that p and (exec_node**)(void*)p are aliased pointers of different
type would lead to a strict-aliasing violation.

However, I'm assuming the intent of the rule is that two valid pointers
of different types can never point to the same address. In other words,
when the compiler sees two pointers of different types, it can assume
they point to different memory locations.

Thus, if the compiler thinks:

   p-next = a  becomes  write a to p+0
   b = q[0] becomes  read b from q+0

With p and q being known different values (due to having different
types), it is clear that the read statement can not be affected by the
write statement.

Again, I'm not sure that's what the spec actually says, but I suspect
strongly that this was the intention of the strict aliasing rules. And
that precisely these optimizations is what compilers do when they use
the strict aliasing rules.

eirik
___
mesa

Re: [Mesa-dev] [PATCH v2] glsls: Modify exec_list to avoid strict-aliasing violations

[Eirik Byrkjeflot Anonsen]

Erik Faye-Lund kusmab...@gmail.com writes:

 On Fri, Jun 26, 2015 at 1:23 PM, Davin McCall dav...@davmac.org wrote:
 On 26/06/15 12:03, Davin McCall wrote:

 ... The stored value of 'n' is not accessed by any other type than the
 type of n itself. This value is then cast to a different pointer type. You
 are mistaken if you think that the cast accesses the stored value of n. The
 other stored value access that it occurs in that expression is to the
 object pointed at by the result of the cast. [...]:


 I'm sorry, I think that was phrased somewhat abrasively, which I did not
 intend. Let me try this part again. If we by break up the expression in
 order of evaluation:

 From:
return ((const struct exec_node **)n)[0]

 In order of evaluation:

 n
 - which accesses the stored value of n, i.e. a value of type 'struct exec
 node *', via n, which is obviously of that type.

 (const struct exec_node **)n
  - which casts that value, after it has been retrieved, to another type. If
 this were an aliasing violation, then casting any pointer variable to
 another type would be an aliasing violation; this is clearly not the case.

 ((const struct exec_node **)n)[0]
 - which de-references the result of the above cast, thereby accessing a
 stored value of type 'exec node *' using a glvalue of type 'exec node *'.

 I think breaking this up is a mistake, because the strict-aliasing
 rules is explicitly about the *combination* of these two things.

 You *are* accessing the underlying memory of 'n' through a different
 type, and this is what strict aliasing is all about. But it takes two
 steps, a single step isn't enough to do so.

 Those other spec-quotes doesn't undo the strict-aliasing definitions;
 knowing how things are laid out in memory doesn't mean the compiler
 cannot assume two differently typed variables doesn't overlap.

So basically, you're saying that e.g.:

   p-next = a;
   q = exec_node_get_next_const(p);

is equivalent to:

   exec_node * p1 = p;
   exec_node ** p2 = (exec_node**)p;
   p1-next = a;
   q = p2[0];

And at this point p1 and p2 are different types, so the compiler can
freely assume that p1 and p2 are non-overlapping. Thus the two
assignments can be safely reordered. Sounds plausible to me.

And note that casting via void* won't help. p == (void*)p compares a
variable of type exec_node* to a variable of type void*, and thus
there's no strict-aliasing problem. But p == (exec_node**)(void*)p
compares an exec_node* to an exec_node** and thus the compiler can
assume that they are not the same.

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] abundance of branches in mesa.git

[Eirik Byrkjeflot Anonsen]

Ilia Mirkin imir...@alum.mit.edu writes:

 Hello,

 There are a *ton* of branches in the upstream mesa git. Here is a full list:

[...]
 is there
 any reason to keep these around with the exception of:

 master
 $version (i.e. 9.0, 10.0, mesa_7_7_branch, etc)

Instead of outright deleting old branches, it would be possible to set
up an archive repository which mirrors all branches of the main
repository. And then delete obsolete branches only from the main
repository. Ideally, you would want a git hook to refuse to create a new
branch (in the main repository) if a branch by that name already exists
in the archive repository. Possibly with the exception that creating a
same-named branch on the same commit would be allowed.

(And the same for tags, of course)

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/4] nouveau/compiler: init the size and code variables to NULL/0

[Eirik Byrkjeflot Anonsen]

Ilia Mirkin imir...@alum.mit.edu writes:

 Sure. Fix it in GCC :) disable the bogus warning, whatever. I really really
 hate pandering to broken tools, sending the message that it's ok for the
 tools to be broken.

While I largely agree with the sentiment, I'm not sure I agree with what
is a broken tool. If a tool regularly catches real, nasty bugs and
fixing the false positives have negligible cost, then I don't consider
the tool broken. When it comes to the warning about uninitialized data,
that seems to me a typical case of this.

I haven't actually looked at the code in question, so maybe it is really
painfully obvious that the uninitialized data would never be used. But
even so, I think initializing variables is almost always negligible
cost, and I suspect this warning will often find real bugs that can be
really hard to track down.

The other warning about the ambiguity of !a==b I'm more doubtful
about.

eirik

 On Jun 5, 2015 5:20 PM, Martin Peres martin.pe...@linux.intel.com wrote:

 On 05/06/15 17:12, Ilia Mirkin wrote:


 But if codegen fails, the code is never displayed as I recall. Again,
 this I'd going from memory...


 Good memory. It looks ugly though.

 In general, I would still want to fix the warning because I hate needing
 to revert my patches to check if I introduced warnings or not. In this
 particular case it is probably OK because no-one will likely touch this
 file for the foreseeable future :)

 ___
 mesa-dev mailing list
 mesa-dev@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 12/15] egl: add eglCreateImage

[Eirik Byrkjeflot Anonsen]

Marek Olšák mar...@gmail.com writes:

 I don't understand. Using size_t should prevent the integer overflow.
 Is there anything else wrong other than no fail path for malloc? I
 also don't understand how calloc can help here.

 Marek

size * sizeof(int_attribs[0]) may overflow and thus wrap to a small
number. Using calloc, you'd have calloc(size, sizeof(int_attribs[0])),
moving the overflow inside calloc(). So if calloc() does its job
properly, it will protect against it.

eirik


 On Wed, May 27, 2015 at 9:07 PM, Chad Versace chad.vers...@intel.com wrote:
 On Fri 15 May 2015, Emil Velikov wrote:
 On 12/05/15 22:54, Marek Olšák wrote:
  From: Marek Olšák marek.ol...@amd.com
 
  ---
   src/egl/main/eglapi.c | 38 ++
   1 file changed, 38 insertions(+)
 
  diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
  index 6457798..34a113b 100644
  --- a/src/egl/main/eglapi.c
  +++ b/src/egl/main/eglapi.c
  @@ -251,6 +251,30 @@ _eglUnlockDisplay(_EGLDisplay *dpy)
   }
 
 
  +static EGLint *
  +_eglConvertAttribsToInt(const EGLAttrib *attr_list)
  +{
  +   EGLint *int_attribs = NULL;
  +
  +   /* Convert attributes from EGLAttrib[] to EGLint[] */
  +   if (attr_list) {
  +  int i, size = 0;
  +
  +  while (attr_list[size] != EGL_NONE)
  + size += 2;
  +
  +  if (size) {
  + size += 1; /* add space for EGL_NONE */
  + int_attribs = malloc(size * sizeof(int_attribs[0]));
  +
  + for (i = 0; i  size; i++)
  +int_attribs[i] = attr_list[i];

 In the unlikely event that malloc fails, it'll be nice to not crash.

 NAK.

 There is a stack overflow vulnerability here, even when malloc succeeds.
 An attacker can pass a very large but valid `EGLint *attrib_list` into
 an EGL entry point, forcing the size calculation given to malloc to
 overflow to a small positive integer.  Then _eglConvertAttribsToInt will
 blithely copy a portion (perhaps most) of the attacker's attrib list onto
 the stack!

 To prevent the stack overflow, _eglConvertAttribsToInt should use
 calloc() and abort if allocation fails.
 ___
 mesa-dev mailing list
 mesa-dev@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel: Change vendor string to Intel(R) Open Source Technology Center.

[Eirik Byrkjeflot Anonsen]

Kenneth Graunke kenn...@whitecape.org writes:

 On 05/31/2012 04:28 PM, Roland Mainz wrote:
 On Fri, Jun 1, 2012 at 1:19 AM, Kenneth Graunkekenn...@whitecape.org  
 wrote:
 switch (name) {
 case GL_VENDOR:
 -  return (GLubyte *) Tungsten Graphics, Inc;
 +  return (GLubyte *) Intel® Open Source Technology Center;
break;

 Uhm... isn't ® a character outside the ASCII range ? Some compiles
 might choke on this, i.e. the Sun Workshop/Forte/Studio compilers
 require -xcsi (... This option allows the C compiler to accept source
 code written in locales that do not conform to the ISO C source
 character code requirements. These locales include ja_JP.PCK ...) to
 avoid occasional hiccups.

 I'm happy to change it if it's an issue, but I believe that the ®
 character already appears in the source code:

   case PCI_CHIP_GM45_GM:
  chipset = Mobile Intel® GM45 Express Chipset;
  break;

I believe the C89 specification disallows the use of characters outside
the basic character set in the source file.  The basic character set
is a subset of the characters representable by ASCII.  Or alternatively,
that it leaves the effect implementation-defined or maybe even
undefined.  But I don't have the C89 specification lying around to
check.

I expect many compilers will accept such characters, and interpret them
according to the current locale.  So what character you end up with
depends on which locale you compile in, or which locale you run the
application in.  This may lead some compilers to generate warnings about
this situation.

C++03 allows those characters, but their interpretation is
implementation-defined.  C++03 supports the construct '\u00a9', which
represents the unicode character U+00A9.  I doubt C89 does the same.

For string literals the '\xa9' construct can be used, but its
interpretation is also implementation-defined (at least as far as
which character it represents, given that the execution character set is
implementation-defined).  And keep in mind that '\xa9' must not be
followed by a hexadecimal digit.

 and it definitely appears in comments.

That's less of an issue, since the compiler is quite likely to just
ignore those.

eirik
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev