There is no opinions about this issue or reviews of the proposed patch after one week.
This is just a reminder in case you have missed it :-) Sam On 21/10/15 12:23, Iago Toral wrote: > Hi, > > The problem is with code like this (see brw_send_indirect_message): > > setup = brw_OR(p, addr, desc, brw_imm_ud(0)); > send = next_insn(p, BRW_OPCODE_SEND); > ... > return setup; > > If next_insn triggers a realloc of the instruction store, then the setup > instruction pointer is no longer valid. Notice that this can happen > anywhere where we keep pointers to previous instructions before creating > new ones (!) > > The patch from Samuel fixes this by special-casing this for SEND > instructions only (since we know that the indirect versions can hit > this, maybe there are more situations though). It does so by trying to > make sure that we never realloc the store with a SEND instruction. For > this, we realloc before we reach the end of the current store (32 > instructions before the limit) as long as the instruction is not a SEND > (so that if it is a SEND we still have up to 32 opportunities to do the > realloc without a different instruction before running out of space in > the store). > > Iago > > On Wed, 2015-10-21 at 09:02 +0200, Samuel Iglesias Gonsalvez wrote: >> Hello, >> >> I have found several invalid memory accesses when running >> dEQP-GLES31.functional.ssbo.* tests on i965 driver (and gen7+). That >> invalid memory accesses were unluckily happening when generating the >> assembly instructions for SSBO stores for different compute shaders. >> >> However it looks like this problem could happen to other shaders and >> situations. Because of that, I am going to explain the problem here: >> >> When generating a untyped surface write/read, i965 driver will end up >> calling brw_send_indirect_message() through >> brw_send_indirect_surface_message(). At brw_send_indirect_message()'s >> 'else' branch, the code generates a load of the indirect descriptor to >> an address register using an OR instruction and it also generates a new >> SEND instruction; if this case happens, the OR instruction is returned. >> brw_send_indirect_surface_message() uses that OR instruction to set mlen >> and rlen's descriptor bits later. >> >> Just to give more context, when generating instructions in fs/vec4 >> generators, i965 driver uses pointers to elements in the 'store' table >> inside struct brw_codegen. That table has an initial size of 1024 but, >> when it's full, it is resized (doubling its size each time, >> see brw_next_insn()). This resize operation ends up calling >> realloc(). However the returned pointer by realloc() could be different >> and the old allocated memory would be free'd as part of the process. >> >> Back to the issue, if the p->store's resize happens when we get the >> pointer to the SEND instruction at brw_send_indirect_message()'s 'else' >> branch, we could have the following problem: >> >> The realloc() returns a new pointer and *free'd* the old allocation, so >> the pointer we previously saved for the OR instruction at >> brw_send_indirect_surface_message() becomes invalid (because it is a >> pointer of the old allocation). Then, we access to that invalid pointer >> when setting up rlen/mlen bits at the end of >> brw_send_indirect_surface_message() and we would have undefined results. >> >> This issue is quite unlikely to happen but it is reproducible on >> ~120 dEQP-GLES31.functional.ssbo.* tests, basically because they have >> the same shaders except the buffer variable's data type. Those tests >> were failing intermittently at different rates but valgrind helped to >> find what was happening. >> >> I would like to expose publicly the problem and analyse possible >> solutions for it along with the community. For the time being, a patch >> is proposed to mitigate this issue, which is sent as a reply to this >> one. >> >> What this work-around patch does is: book enough room for one or more >> consecutive SEND* instructions at the end of the p->store table in order >> to avoid reallocating p->store in the aforementioned case. The 32 value >> was chosen arbitrary because it has low impact in p->store >> (its initial size is 1024) and makes this issue much less likely to >> happen. We could tune this number to a less conservative value if >> needed. If you want to test it, that patch should be applied on top of >> this Curro's patch [0] as it fixes a lot of compute shader compilation >> errors (~700 dEQP-GLES31.functional.ssbo.* tests). I have setup a branch >> with both patches in [1]. >> >> Feel free to comment about other solutions, ideas, opinions, etc. >> >> Thanks, >> >> Sam >> >> [0] See attachment at >> http://lists.freedesktop.org/archives/mesa-dev/2015-October/097183.html >> [1] $ git clone -b dEQP-functional-ssbo-fixes-v1 \ >> https://github.com/Igalia/mesa.git >> >> Samuel Iglesias Gonsalvez (1): >> i965: book space at the end of p->store for SEND opcodes to avoid >> invalid memory access >> >> src/mesa/drivers/dri/i965/brw_eu_emit.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
