Re: [Mesa-dev] [PATCH] nv50: use larger arrays to silence warnings
On 01.02.2012 16:21, Brian Paul wrote: > On 02/01/2012 08:14 AM, Christoph Bumiller wrote: >> On 01.02.2012 15:23, Brian Paul wrote: >>> On 02/01/2012 03:40 AM, Jose Fonseca wrote: Silences warnings and fixes potential bugs due to buffer overflow. The nv50 maintainers could benefit from sprinkling a few asserts to catch this early in the future, as it is bound to happen again. >>> >>> Good point. I'll add a couple asserts. >> >> Hm, I don't see any assertions in the commit ... > > Grrr, I forgot to fold them into the commit before pushing. I'll push > them soon. > > >> >> I should really have used something like NV50_MAX_INSN_SRCS in the loop >> in the old code there (nv50_pc_regalloc), but it's scheduled for removal >> anyway ... >> >> As for the optimization pass, I don't see how the compiler can >> statically determine that the array will overflow, > > I was a bit surprised to see the compiler figure that out too. > > >> and it wasn't >> supposed to (maximum store size is 16 bytes, and minimum size of a Value >> (with TGSI input) is 4 bytes, makes 4 Values. >> >> But you're right I should have been more careful with that code, it's >> ... been written at a point where I just wanted to be done with it (but >> still include enough optimizations so as to not produce worse code than >> the old solution). >> >> Thanks for fixing, Christoph >> >> hm, I need to make my compiler spit out better/more warnings ... > > Do you want to cherry-pick these fixes to 8.0? > Yes, seems like the kind of fix that should go into stable. > -Brian > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nv50: use larger arrays to silence warnings
On 02/01/2012 08:14 AM, Christoph Bumiller wrote: On 01.02.2012 15:23, Brian Paul wrote: On 02/01/2012 03:40 AM, Jose Fonseca wrote: Silences warnings and fixes potential bugs due to buffer overflow. The nv50 maintainers could benefit from sprinkling a few asserts to catch this early in the future, as it is bound to happen again. Good point. I'll add a couple asserts. Hm, I don't see any assertions in the commit ... Grrr, I forgot to fold them into the commit before pushing. I'll push them soon. I should really have used something like NV50_MAX_INSN_SRCS in the loop in the old code there (nv50_pc_regalloc), but it's scheduled for removal anyway ... As for the optimization pass, I don't see how the compiler can statically determine that the array will overflow, I was a bit surprised to see the compiler figure that out too. and it wasn't supposed to (maximum store size is 16 bytes, and minimum size of a Value (with TGSI input) is 4 bytes, makes 4 Values. But you're right I should have been more careful with that code, it's ... been written at a point where I just wanted to be done with it (but still include enough optimizations so as to not produce worse code than the old solution). Thanks for fixing, Christoph hm, I need to make my compiler spit out better/more warnings ... Do you want to cherry-pick these fixes to 8.0? -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nv50: use larger arrays to silence warnings
On 01.02.2012 15:23, Brian Paul wrote: > On 02/01/2012 03:40 AM, Jose Fonseca wrote: >> Silences warnings and fixes potential bugs due to buffer overflow. >> >> The nv50 maintainers could benefit from sprinkling a few asserts to >> catch this early in the future, as it is bound to happen again. > > Good point. I'll add a couple asserts. Hm, I don't see any assertions in the commit ... I should really have used something like NV50_MAX_INSN_SRCS in the loop in the old code there (nv50_pc_regalloc), but it's scheduled for removal anyway ... As for the optimization pass, I don't see how the compiler can statically determine that the array will overflow, and it wasn't supposed to (maximum store size is 16 bytes, and minimum size of a Value (with TGSI input) is 4 bytes, makes 4 Values. But you're right I should have been more careful with that code, it's ... been written at a point where I just wanted to be done with it (but still include enough optimizations so as to not produce worse code than the old solution). Thanks for fixing, Christoph hm, I need to make my compiler spit out better/more warnings ... > > -Brian > ___ > 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] nv50: use larger arrays to silence warnings
On 02/01/2012 03:40 AM, Jose Fonseca wrote: Silences warnings and fixes potential bugs due to buffer overflow. The nv50 maintainers could benefit from sprinkling a few asserts to catch this early in the future, as it is bound to happen again. Good point. I'll add a couple asserts. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nv50: use larger arrays to silence warnings
Silences warnings and fixes potential bugs due to buffer overflow.
The nv50 maintainers could benefit from sprinkling a few asserts to catch this
early in the future, as it is bound to happen again.
Jose
- Original Message -
> The warnings were:
> nv50_pc_regalloc.c: In function ‘pass_generate_phi_movs’:
> nv50_pc_regalloc.c:423:41: warning: array subscript is above array
> bounds
> codegen/nv50_ir_peephole.cpp: In member function ‘bool
> nv50_ir::MemoryOpt::replaceStFromSt(nv50_ir::Instruction*,
> nv50_ir::MemoryOpt::Record*)’:
> codegen/nv50_ir_peephole.cpp:1475:18: warning: array subscript is
> above array bounds
> codegen/nv50_ir_peephole.cpp:1475:18: warning: array subscript is
> above array bounds
> codegen/nv50_ir_peephole.cpp:1475:18: warning: array subscript is
> above array bounds
> codegen/nv50_ir_peephole.cpp:1475:18: warning: array subscript is
> above array bounds
> ---
> .../drivers/nv50/codegen/nv50_ir_peephole.cpp |2 +-
> src/gallium/drivers/nv50/nv50_pc.h |2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/drivers/nv50/codegen/nv50_ir_peephole.cpp
> b/src/gallium/drivers/nv50/codegen/nv50_ir_peephole.cpp
> index fc025d8..fb4041f 100644
> --- a/src/gallium/drivers/nv50/codegen/nv50_ir_peephole.cpp
> +++ b/src/gallium/drivers/nv50/codegen/nv50_ir_peephole.cpp
> @@ -1463,7 +1463,7 @@ MemoryOpt::replaceStFromSt(Instruction
> *restrict st, Record *rec)
> st->takeExtraSources(0, extra);
>
> if (offR < offS) {
> - Value *vals[4];
> + Value *vals[10];
>int s, n;
>int k = 0;
>// get non-replaced sources of ri
> diff --git a/src/gallium/drivers/nv50/nv50_pc.h
> b/src/gallium/drivers/nv50/nv50_pc.h
> index 45804d3..9abefa2 100644
> --- a/src/gallium/drivers/nv50/nv50_pc.h
> +++ b/src/gallium/drivers/nv50/nv50_pc.h
> @@ -234,7 +234,7 @@ struct nv_instruction {
> int serial;
> struct nv_value *def[4];
> struct nv_value *flags_def;
> - struct nv_ref *src[5];
> + struct nv_ref *src[6];
> struct nv_ref *flags_src;
> struct nv_basic_block *bb;
> struct nv_basic_block *target; /* target block of control flow
> insn */
> --
> 1.7.3.4
>
> ___
> 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
[Mesa-dev] [PATCH] nv50: use larger arrays to silence warnings
The warnings were:
nv50_pc_regalloc.c: In function ‘pass_generate_phi_movs’:
nv50_pc_regalloc.c:423:41: warning: array subscript is above array bounds
codegen/nv50_ir_peephole.cpp: In member function ‘bool
nv50_ir::MemoryOpt::replaceStFromSt(nv50_ir::Instruction*,
nv50_ir::MemoryOpt::Record*)’:
codegen/nv50_ir_peephole.cpp:1475:18: warning: array subscript is above array
bounds
codegen/nv50_ir_peephole.cpp:1475:18: warning: array subscript is above array
bounds
codegen/nv50_ir_peephole.cpp:1475:18: warning: array subscript is above array
bounds
codegen/nv50_ir_peephole.cpp:1475:18: warning: array subscript is above array
bounds
---
.../drivers/nv50/codegen/nv50_ir_peephole.cpp |2 +-
src/gallium/drivers/nv50/nv50_pc.h |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/gallium/drivers/nv50/codegen/nv50_ir_peephole.cpp
b/src/gallium/drivers/nv50/codegen/nv50_ir_peephole.cpp
index fc025d8..fb4041f 100644
--- a/src/gallium/drivers/nv50/codegen/nv50_ir_peephole.cpp
+++ b/src/gallium/drivers/nv50/codegen/nv50_ir_peephole.cpp
@@ -1463,7 +1463,7 @@ MemoryOpt::replaceStFromSt(Instruction *restrict st,
Record *rec)
st->takeExtraSources(0, extra);
if (offR < offS) {
- Value *vals[4];
+ Value *vals[10];
int s, n;
int k = 0;
// get non-replaced sources of ri
diff --git a/src/gallium/drivers/nv50/nv50_pc.h
b/src/gallium/drivers/nv50/nv50_pc.h
index 45804d3..9abefa2 100644
--- a/src/gallium/drivers/nv50/nv50_pc.h
+++ b/src/gallium/drivers/nv50/nv50_pc.h
@@ -234,7 +234,7 @@ struct nv_instruction {
int serial;
struct nv_value *def[4];
struct nv_value *flags_def;
- struct nv_ref *src[5];
+ struct nv_ref *src[6];
struct nv_ref *flags_src;
struct nv_basic_block *bb;
struct nv_basic_block *target; /* target block of control flow insn */
--
1.7.3.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
