[Mesa-dev] [PATCH 0/9][RFC] GLSL preprocessor/parser improvements

[Thomas Helland]

2017-05-22 3:41 GMT+02:00 Timothy Arceri <tarceri at itsqueeze.com>:
> I suspect you forgot to test with a debug build, there is issues with the
> string buffer  changes somewhere.
>
> ralloc.c:203: reralloc_size: Assertion `ralloc_parent(ptr) == ctx' failed.
>
> #0  0x00007ffff70eb91f in raise () from /lib64/libc.so.6
> #1  0x00007ffff70ed51a in abort () from /lib64/libc.so.6
> #2  0x00007ffff70e3da7 in __assert_fail_base () from /lib64/libc.so.6
> #3  0x00007ffff70e3e52 in __assert_fail () from /lib64/libc.so.6
> #4  0x00007ffff442c73c in reralloc_size (ctx=ctx at entry=0x7fffd40cb910,
> ptr=<optimized out>, size=<optimized out>) at ralloc.c:203
> #5  0x00007ffff44b519e in glcpp_preprocess
> (ralloc_ctx=ralloc_ctx at entry=0x7fffd40c9da0,
> shader=shader at entry=0x7fffea7cfcc8, info_log=info_log at entry=0x7fffd40ca058,
>     extensions=extensions at entry=0x7ffff43ff070
> <add_builtin_defines(_mesa_glsl_parse_state*, void (*)(glcpp_parser*, char
> const*, int), glcpp_parser*, unsigned int, bool)>,
> state=state at entry=0x7fffd40c9da0,
>     gl_ctx=gl_ctx at entry=0x7fffe85c7010) at glsl/glcpp/pp.c:246
> #6  0x00007ffff4402665 in _mesa_glsl_compile_shader
> (ctx=ctx at entry=0x7fffe85c7010, shader=shader at entry=0x7fffd40c8cc0,
> dump_ast=dump_ast at entry=false, dump_hir=dump_hir at entry=false,
> force_recompile=force_recompile at entry=false)
>     at glsl/glsl_parser_extras.cpp:2065
>
>

Your suspicion is of course correct. Switching back and forth between
profiling and coding and I forgot to switch debug back on. I'll look into
this, plus addressing all of the nice feedback. Thanks for taking a look.

>
> On 22/05/17 06:49, Thomas Helland wrote:
>>
>> This patch series contains some of the work done by Vladislav
>> in the beginning of March, that seems to have been forgotten.
>>
>> For reference, that series, with review comments, can be found here:
>> https://lists.freedesktop.org/archives/mesa-dev/2017-January/139892.html
>>
>> I've adressed some of the review comments, most notably redoing the
>> string buffer implementation quite a bit, and adding tests.
>> The major change is that it is now decoupled from the parser
>> and instead lives in /src/util/ as a utility for easier reuse.
>> I've also closely followed the structure of our hash table and set
>> so that it should be quite familiar for everyone. My implementation
>> is also null terminated, while Vladislav's implementation was not.
>> My reasoning behind this was that it's less fragile if someone where
>> to access the contents of the buffer directly. I've also expanded
>> with some different functions that allow us to do more stuff.
>> Obviously this could be expanded upon further if need be.
>>
>> I've included some of Vladislav's less involved patches that
>> I think we should get in as soon as possible. I've added Ian's
>> r-b on the ones that he reviewed (I hope that's OK). The one
>> patch that is missing from this series is the hand-written
>> custom parser. While this was the thing that gave the biggest
>> speed-up, unfortunately it's also a bit harder to review,
>> so I've left that as a future exercise.
>>
>> I've run it through my complete shader-db and there's no changes
>> or breakages, so that's encouraging. It amounts to about a 3%
>> reduction in runtime and executed instructions on the shader-db run.
>> It should be noted that the i965 backend is the primary bottleneck
>> when running shader-db on my computer, so the numbers don't look all
>> that impressive due to this. I'll see if I can come up with some
>> numbers using the gallium noop driver.
>>
>> I have not done a very thorough job on testing that the output
>> of the preprocessor/parser is the same after this series.
>> However, there's no changes here that I believe should cause
>> any issues, so I feel quite confident that this series should
>> not cause any trouble. The original series did have some issues
>> discovered by running a fuzzer over it, however I expect that
>> was caused by the introduced hand-written fast-path scanner and
>> associated macro substitutaion that are not a part of this series.
>>
>> CC: Vladislav Egorov <vegorov180 at gmail.com>
>> CC: Ian Romanick <idr at freedesktop.com>
>>
>> Thomas Helland (5):
>>    util: Add a string buffer implementation
>>    util: Add tests for the string buffer
>>    glsl: Change the parser to use the string buffer
>>    glcpp: Use string_buffer for line continuation removal
>>    glcpp: Avoid unnecessary call to strlen
>>
>> Vladislav Egorov (4):
>>    glcpp: Avoid unnecessary strcmp()
>>    glcpp: Skip unnecessary line continuations removal
>>    ralloc: Use strnlen() inside of strncat()
>>    glcpp: Use Bloom filter before identifier search
>>
>>   configure.ac                                    |   1 +
>>   src/compiler/glsl/glcpp/glcpp-lex.l             |   3 +-
>>   src/compiler/glsl/glcpp/glcpp-parse.y           | 123 +++++++++-------
>>   src/compiler/glsl/glcpp/glcpp.h                 |  18 ++-
>>   src/compiler/glsl/glcpp/pp.c                    |  73 ++++++----
>>   src/util/Makefile.am                            |   3 +-
>>   src/util/Makefile.sources                       |   2 +
>>   src/util/ralloc.c                               |   7 +-
>>   src/util/string_buffer.c                        | 180
>> ++++++++++++++++++++++++
>>   src/util/string_buffer.h                        |  75 ++++++++++
>>   src/util/tests/string_buffer/Makefile.am        |  34 +++++
>>   src/util/tests/string_buffer/append_and_print.c |  82 +++++++++++
>>   12 files changed, 505 insertions(+), 96 deletions(-)
>>   create mode 100644 src/util/string_buffer.c
>>   create mode 100644 src/util/string_buffer.h
>>   create mode 100644 src/util/tests/string_buffer/Makefile.am
>>   create mode 100644 src/util/tests/string_buffer/append_and_print.c
>>
>