[Mesa-dev] [PATCH] i965: fix blorp stage_prog_data->param leak

[Jason Ekstrand]

On Wed, Nov 1, 2017 at 9:24 AM, Tapani Pälli <tapani.palli at intel.com> wrote:

>
>
> On 11/01/2017 04:55 PM, Jason Ekstrand wrote:
>
>> On Wed, Nov 1, 2017 at 7:53 AM, Jason Ekstrand <jason at jlekstrand.net
>> <mailto:jason at jlekstrand.net>> wrote:
>>
>>     On Fri, Oct 27, 2017 at 5:55 AM, Tapani Pälli
>>     <tapani.palli at intel.com <mailto:tapani.palli at intel.com>> wrote:
>>
>>         Patch uses mem_ctx for allocation to ensure param array gets freed
>>         later, in blorp clear case this happens in
>>         blorp_params_get_clear_kernel.
>>
>>         ==6164== 48 bytes in 1 blocks are definitely lost in loss record
>>         61 of 193
>>         ==6164==    at 0x4C2EB6B: malloc (vg_replace_malloc.c:299)
>>         ==6164==    by 0x12E31C6C: ralloc_size (ralloc.c:121)
>>         ==6164==    by 0x130189F1:
>>         fs_visitor::assign_constant_locations() (brw_fs.cpp:2095)
>>         ==6164==    by 0x13022D32: fs_visitor::optimize()
>> (brw_fs.cpp:5715)
>>         ==6164==    by 0x13024D5A: fs_visitor::run_fs(bool, bool)
>>         (brw_fs.cpp:6229)
>>         ==6164==    by 0x1302549A: brw_compile_fs (brw_fs.cpp:6570)
>>         ==6164==    by 0x130C4B07: blorp_compile_fs (blorp.c:194)
>>         ==6164==    by 0x130D384B: blorp_params_get_clear_kernel
>>         (blorp_clear.c:79)
>>         ==6164==    by 0x130D3C56: blorp_fast_clear (blorp_clear.c:332)
>>         ==6164==    by 0x12EFA439: do_single_blorp_clear
>> (brw_blorp.c:1261)
>>         ==6164==    by 0x12EFC4AF: brw_blorp_clear_color
>> (brw_blorp.c:1326)
>>         ==6164==    by 0x12EFF72B: brw_clear (brw_clear.c:297)
>>
>>         Signed-off-by: Tapani Pälli <tapani.palli at intel.com
>>         <mailto:tapani.palli at intel.com>>
>>         ---
>>           src/intel/compiler/brw_fs.cpp | 2 +-
>>           1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>         diff --git a/src/intel/compiler/brw_fs.cpp
>>         b/src/intel/compiler/brw_fs.cpp
>>         index 4616529abc..6b27c38be7 100644
>>         --- a/src/intel/compiler/brw_fs.cpp
>>         +++ b/src/intel/compiler/brw_fs.cpp
>>         @@ -2092,7 +2092,7 @@ fs_visitor::assign_constant_locations()
>>               */
>>              uint32_t *param = stage_prog_data->param;
>>              stage_prog_data->nr_params = num_push_constants;
>>         -   stage_prog_data->param = ralloc_array(NULL, uint32_t,
>>         num_push_constants);
>>         +   stage_prog_data->param = ralloc_array(mem_ctx, uint32_t,
>>         num_push_constants);
>>
>>
>>     Wow, I don't know how I didn't see this pass.  The more correct
>>     answer is that blorp no longer uses push constants, so we can just
>>     delete the whole mess.  I'll send a patch.
>>
>>
>> Gah!  Ignore me.  This is, indeed, correct.
>>
>>              if (num_pull_constants > 0) {
>>                 stage_prog_data->nr_pull_params = num_pull_constants;
>>                 stage_prog_data->pull_param = ralloc_array(NULL, uint32_t,
>>
>>
>> We should be doing it here as well.
>>
>>
> ok, did not catch that as the use case I was running did not use pull
> constants, I can send a separate fix for that one.


As I said in the commit message of the patch I sent, it *almost* doesn't
matter in practice.  It only matters if shader compilation somehow fails
which is not a common case.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171101/882725ea/attachment.html>