Bug 69200 - [Bisected]Piglit glx/glx-multithread-shader-compile aborted
Summary: [Bisected]Piglit glx/glx-multithread-shader-compile aborted
Status: VERIFIED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: high major
Assignee: Intel 3D Bugs Mailing List
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-11 02:20 UTC by lu hua
Modified: 2014-11-03 08:42 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch to add locks to builtin_builder::initialize() (1.78 KB, patch)
2013-09-11 02:22 UTC, Kenneth Graunke
Details | Splinter Review
Patch to add a lock for the builtins singleton (2.56 KB, patch)
2014-02-06 11:27 UTC, Daniel Kurtz
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description lu hua 2013-09-11 02:20:48 UTC
System Environment:
--------------------------
Arch:           x86_64
Platform:       Ironlake/Ivybridge
Libdrm:		(master)libdrm-2.4.46-36-g58d008883165ba35c83041fa9ed84937163d5f76
Mesa:		(master)395b9410860371a64d6b5f2d50679f29eb41729e
Xserver:	(master)xorg-server-1.14.99.1-212-g47ff382d1fce25a8b097d45b79489e891f1f1228
Xf86_video_intel:(master)2.99.902-13-g8ff8eb2b38dc705f5c86f524c1cd74a811a7b04c
Cairo:		(master)8e11a42e3e9b679dce97ac45cd8b47322536a253
Libva:		(staging)29ad40444f3ed68e5f54ddd70dd256ea41e0456f
Libva_intel_driver:(staging)670b14cf273a2b5fe6c77b1ea99e9871c998543f
Kernel:	(drm-intel-nightly) 6d68305fda796a578cd7681d9928b148734b5456

Bug detailed description:
-----------------------------
It aborted on ironlake and ivybridge with mesa master branch, and works well on 9.2 branch. It has Bug 66948 on SNB.
Bisect shows: 76d2f73643f5502d88fdc272447753fde8f6438b is the first bad commit.
commit 76d2f73643f5502d88fdc272447753fde8f6438b
Author:     Kenneth Graunke <kenneth@whitecape.org>
AuthorDate: Sun Sep 1 20:48:45 2013 -0700
Commit:     Kenneth Graunke <kenneth@whitecape.org>
CommitDate: Mon Sep 9 14:42:33 2013 -0700

    glsl: Switch to the new built-in function module.

    All built-ins are now handled by the new code; the old system is dead.

    Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
    Reviewed-by: Matt Turner <mattst88@gmail.com>
    Reviewed-by: Paul Berry <stereotype441@gmail.com>

output:
Failed to compile vertex shader: 0:0(0): error: no matching function for call to `ftransform()'

source:
void main()
{
   gl_Position = ftransform();
   gl_FrontColor = gl_Color;
}
PIGLIT: {'result': 'fail' }
glx-multithread-shader-compile: ../../../src/glsl/ralloc.c:81: get_header: Assertion `info->canary == 0x5A1106' failed.
Aborted (core dumped)

(gdb) bt
#0  push_tail (n=0x7fffec30af58, this=0x7fffec30b598) at ../../../src/glsl/list.h:368
#1  builtin_builder::_atan2 (this=this@entry=0x7ffff3b0cfe0 <builtins>, type=type@entry=0x7ffff3b0df00 <glsl_type::_vec2_type>) at ../../../src/glsl/builtin_functions.cpp:2089
#2  0x00007ffff37db3d2 in builtin_builder::create_builtins (this=this@entry=0x7ffff3b0cfe0 <builtins>) at ../../../src/glsl/builtin_functions.cpp:733
#3  0x00007ffff37e88ab in builtin_builder::initialize (this=this@entry=0x7ffff3b0cfe0 <builtins>) at ../../../src/glsl/builtin_functions.cpp:571
#4  0x00007ffff37e88bc in _mesa_glsl_initialize_builtin_functions () at ../../../src/glsl/builtin_functions.cpp:3527
#5  0x00007ffff37c56f8 in match_function_by_name (state=0x7fffec2d6ed0, actual_parameters=0x7ffff4dfda30, name=0x7fffec2637e0 "ftransform")
    at ../../../src/glsl/ast_function.cpp:406
#6  ast_function_expression::hir (this=0x7fffec2d9520, instructions=0x7fffec2e4ca8, state=0x7fffec2d6ed0) at ../../../src/glsl/ast_function.cpp:1661
#7  0x00007ffff37c8eb2 in ast_expression::hir (this=0x7fffec2d95d0, instructions=0x7fffec2e4ca8, state=0x7fffec2d6ed0) at ../../../src/glsl/ast_to_hir.cpp:1079
#8  0x00007ffff37cacb3 in ast_expression_statement::hir (this=<optimized out>, instructions=<optimized out>, state=<optimized out>) at ../../../src/glsl/ast_to_hir.cpp:1696
#9  0x00007ffff37cacff in ast_compound_statement::hir (this=0x7fffec2d9970, instructions=0x7fffec2e4ca8, state=0x7fffec2d6ed0) at ../../../src/glsl/ast_to_hir.cpp:1712
#10 0x00007ffff37ccd6f in ast_function_definition::hir (this=0x7fffec2d99f0, instructions=<optimized out>, state=0x7fffec2d6ed0) at ../../../src/glsl/ast_to_hir.cpp:3691
#11 0x00007ffff37c7fe0 in _mesa_ast_to_hir (instructions=0x7fffec25c5e0, state=state@entry=0x7fffec2d6ed0) at ../../../src/glsl/ast_to_hir.cpp:93
#12 0x00007ffff37ec61e in _mesa_glsl_compile_shader (ctx=ctx@entry=0x7ffff34e6040, shader=shader@entry=0x7fffec2d6cd0, dump_ast=dump_ast@entry=false,
    dump_hir=dump_hir@entry=false) at ../../../src/glsl/glsl_parser_extras.cpp:1476
#13 0x00007ffff36c5134 in compile_shader (ctx=0x7ffff34e6040, shaderObj=<optimized out>) at ../../../src/mesa/main/shaderapi.c:771
#14 0x00007ffff7d39b46 in stub_glCompileShader (shader=1) at /GFX/Test/Piglit/piglit/tests/util/generated_dispatch.c:3757
#15 0x00007ffff7d1b1b0 in piglit_compile_shader_text (target=35633, text=0x401180 "void main() \n{ \n   gl_Position = ftransform(); \n   gl_FrontColor = gl_Color; \n} \n")
    at /GFX/Test/Piglit/piglit/tests/util/piglit-shader.c:147
#16 0x0000000000400ef1 in thread_func (arg=0x0) at /GFX/Test/Piglit/piglit/tests/glx/glx-multithread-shader-compile.c:78
#17 0x00000033ee807c53 in start_thread () from /usr/lib64/libpthread.so.0
#18 0x00000033ee0f4ecd in clone () from /usr/lib64/libc.so.6

Reproduce steps:
----------------------------
1. xinit
2. ./bin/glx-multithread-shader-compile -auto -fbo
Comment 1 Kenneth Graunke 2013-09-11 02:22:52 UTC
Created attachment 85595 [details] [review]
Patch to add locks to builtin_builder::initialize()

This patch fixes the immediate problem, but I'm concerned that we may need locking at a broader scope...
Comment 2 Kenneth Graunke 2013-09-11 02:23:50 UTC
Also, I have no idea why the new system would exhibit the issue while the old one didn't.  It looks like the old code would have the exact same problem...though, the timing is probebly different.
Comment 3 lu hua 2013-09-11 02:48:07 UTC
It also happens on pineview.

spec_glsl-1.10_execution_built-in-functions_fs-length-float also fails on pineview with same bisect commit.
Run: ./bin/shader_runner generated_tests/spec/glsl-1.10/execution/built-in-functions/fs-length-float.shader_test -auto
output:
Probe color at (1,0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 0.000000 0.000000 1.000000
Probe color at (2,0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 0.000000 0.000000 1.000000
Comment 4 Ian Romanick 2013-09-11 14:13:29 UTC
This is a compiler issue.  It's not specific to any driver or hardware.
Comment 5 Daniel Kurtz 2014-02-06 11:13:35 UTC
Comment on attachment 85595 [details] [review]
Patch to add locks to builtin_builder::initialize()

Review of attachment 85595 [details] [review]:
-----------------------------------------------------------------

I think we also want to protect against one thread calling release() while another thread was still in initialize() or find().
So, perhaps we can use a single mutex at file scope to protect all accesses to the builtins singleton?
I've uploaded a patch that implements this.
Comment 6 Daniel Kurtz 2014-02-06 11:27:46 UTC
Created attachment 93519 [details] [review]
Patch to add a lock for the builtins singleton

AFAICT, this bug is still open, since I ran into this same bug when running a fairly recent mesa [0] with very recent piglit [1].

[0] cbecd958a7e36736a4447ebe65e5017e5c0ea4a0
[1] 8eb40d7e829500ff4e22d91745cc7c4f0f6e64d0

I'm not sure what the protocol is for submitting mesa patches or getting them accepted.  If I need to send this to a list, too, please let me know.

To test it, I ran:
/usr/local/piglit/bin/glx-multithread-shader-compile -fbo -auto
Comment 7 lu hua 2014-02-08 06:43:49 UTC
(In reply to comment #6)
> Created attachment 93519 [details] [review] [review]
> Patch to add a lock for the builtins singleton
> 
> AFAICT, this bug is still open, since I ran into this same bug when running
> a fairly recent mesa [0] with very recent piglit [1].
> 
> [0] cbecd958a7e36736a4447ebe65e5017e5c0ea4a0
> [1] 8eb40d7e829500ff4e22d91745cc7c4f0f6e64d0
> 
> I'm not sure what the protocol is for submitting mesa patches or getting
> them accepted.  If I need to send this to a list, too, please let me know.
> 
> To test it, I ran:
> /usr/local/piglit/bin/glx-multithread-shader-compile -fbo -auto

Test this patch:
The 1st run output:
Failed to link: error: uniform `gl_BackLightProduct' declared as type `gl_LightProducts[8]' and type `gl_LightProducts[8]'

PIGLIT: {'result': 'pass' }

The 2nd run output:
PIGLIT: {'result': 'pass' }
Comment 8 Daniel Kurtz 2014-02-10 14:27:40 UTC
I guess that is what Kenneth Graunke meant by "we may need locking at a broader scope"...
Comment 9 Kenneth Graunke 2014-10-30 08:28:48 UTC
In particular, the glsl_type class needs locking in get_record_instance and get_array_instance to ensure that only one flyweight will be created for a particular type, and that the hash tables won't be corrupted.
Comment 10 Kenneth Graunke 2014-10-30 09:30:52 UTC
Probably fixed on master by:

commit 2d64e4ffbab0426317f329e2bae22566dc80b98a
Author: Chia-I Wu <olvaffe@gmail.com>
Date:   Wed Aug 20 14:40:29 2014 +0800

    glsl: protect glsl_type with a mutex
    
    glsl_type has several static hash tables and a static ralloc context.  They
    need to be protected by a mutex as they are not thread-safe.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69200
    Signed-off-by: Chia-I Wu <olv@lunarg.com>
    Reviewed-by: Brian Paul <brianp@vmware.com>
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

It seems to be passing for me pretty consistently, at any rate.
Comment 11 lu hua 2014-11-03 08:42:00 UTC
Test on latest mesa master branch, it works well.


bug/show.html.tmpl processed on Jan 16, 2017 at 21:50:24.
(provided by the Example extension).