[Mesa-dev] [PATCH 1/2] Allow for GLSL shaders to have #extension directive anywhere in source file.
Rogovin, Kevin
kevin.rogovin at intel.com
Tue Dec 3 01:00:25 PST 2013
Hi,
I appreciate the feedback, however I am having difficulty in figuring out a -good- way to break up the first patch. The basic beans is that all of it's parts are needed together for it to even make sense. To be precise, the changes consist of:
1) implementation of linked list of #extension directives (glcpp_extras.[ch] )
2) addition to glsl_parser_extras.cpp (and .h) using it
3) changes to glcpp-parse.y and glcpp-lex.l, checking for the new field and using it.
Until it is active (i.e. the changes in glsl_parser_extras.cpp passing it to the glcpp), all the code
added is more or less dead code; in particular if the patch is broken up, the patch that turned it
on would appear to be the guilty party but when in fact it was a previous patch.
What is a good way to break up the patch so that the code added is not dead and so that
finding a bug in the added code can be done on a patch by patch basis?
Best Regards
-Kevin Rogovin
________________________________________
From: Kenneth Graunke [kenneth at whitecape.org]
Sent: Tuesday, December 03, 2013 10:49 AM
To: Rogovin, Kevin; mesa-dev at lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH 1/2] Allow for GLSL shaders to have #extension directive anywhere in source file.
On 12/03/2013 12:00 AM, Kevin Rogovin wrote:
> Allow for GLSL shaders to have #extension directive
> anywhere in source file; during preprocessor, #extension
> directives are saved to a list rather than being added
> to the pre-processed string. That list is played back
> before parsing of pre-processed text takes place.
>
> ---
> src/glsl/Makefile.sources | 3 +-
> src/glsl/glcpp/glcpp-lex.l | 19 +++++-
> src/glsl/glcpp/glcpp-parse.y | 31 +++++++++-
> src/glsl/glcpp/glcpp.c | 2 +-
> src/glsl/glcpp/glcpp.h | 17 +++++-
> src/glsl/glcpp/glcpp_extras.c | 126 ++++++++++++++++++++++++++++++++++++++++
> src/glsl/glcpp/glcpp_extras.h | 97 +++++++++++++++++++++++++++++++
> src/glsl/glcpp/pp.c | 5 +-
> src/glsl/glsl_parser_extras.cpp | 55 +++++++++++++++++-
> src/glsl/glsl_parser_extras.h | 14 ++++-
> 10 files changed, 358 insertions(+), 11 deletions(-)
> create mode 100644 src/glsl/glcpp/glcpp_extras.c
> create mode 100644 src/glsl/glcpp/glcpp_extras.h
Hi Kevin,
This is a single patch that performs a ton of refactoring as well as a
functional change. It would be great if you could break it down into
smaller parts. For example, perhaps you could begin parsing and
tracking extension directives, but not change how they're actually
processed. Then, you could change it to handle them in glcpp, but still
require them to come first. Finally, you could change the behavior to
accept them anywhere.
Submitting smaller patches makes it easier to review them. It also
makes bisecting more accurate, for tracking down any bugs. Finally, it
means that if you have a contentious part that might require some
rework, you can get review and possibly land the rest of your series,
and not have to respin it.
A couple of other comments:
Please follow the style of the existing glcpp code. Annoyingly, glcpp
currently uses Carl's style, with is different than the rest of Mesa.
However, it's internally very consistent and clean, and I'd prefer to
keep it that way.
If you use vim, you can accomplish the appropriate indent settings by
putting this in your .vimrc file:
if has("autocmd")
au BufNewFile,BufRead */mesa/src/glsl/glcpp/* set noexpandtab
tabstop=8 softtabstop=8 shiftwidth=8
au BufNewFile,BufRead */mesa/* set expandtab tabstop=8
softtabstop=3 shiftwidth=3
endif
(I'm sure there's appropriate magic for Emacs or other editors as well.)
Other than indentation, a couple of nits:
/* Normal multiline comments should have
* stars at the beginning of lines, even in the middle
*/
/**
* Doxygen should look like this.
*
* with the double star at the top and stars in the middle.
*/
Please put spaces around operators and between if/while/for and parens.
For example:
while(current!=NULL) { /* wrong, vs. */
while (current != NULL) { /* correct */
cnt=parser->directive_ext_list->count; /* wrong */
cnt = parser->directive_ext_list->count; /* correct */
for(current=state->ext_directive_list->head; current!=NULL;
current=current->next) { /* wrong */
Also, this is the first use of MALLOC, FREE, and so on in the compiler
code. These are stupid #defines for malloc, free, and so on. Please
just use the standard lowercase C library functions.
Until this patch series is split up and in a more consistent style, I
don't plan to review it further.
--Ken
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index 2e81ded..90f57ad 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -7,7 +7,8 @@ GLSL_BUILDDIR = $(top_builddir)/src/glsl
>
> LIBGLCPP_FILES = \
> $(GLSL_SRCDIR)/ralloc.c \
> - $(GLSL_SRCDIR)/glcpp/pp.c
> + $(GLSL_SRCDIR)/glcpp/pp.c \
> + $(GLSL_SRCDIR)/glcpp/glcpp_extras.c
>
> LIBGLCPP_GENERATED_FILES = \
> $(GLSL_BUILDDIR)/glcpp/glcpp-lex.c \
> diff --git a/src/glsl/glcpp/glcpp-lex.l b/src/glsl/glcpp/glcpp-lex.l
> index a029f62..31b246f 100644
> --- a/src/glsl/glcpp/glcpp-lex.l
> +++ b/src/glsl/glcpp/glcpp-lex.l
> @@ -126,15 +126,30 @@ HEXADECIMAL_INTEGER 0[xX][0-9a-fA-F]+[uU]?
> return HASH_VERSION;
> }
>
> - /* glcpp doesn't handle #extension, #version, or #pragma directives.
> + /* glcpp doesn't handle #pragma directives.
> * Simply pass them through to the main compiler's lexer/parser. */
> -{HASH}(extension|pragma)[^\n]+ {
> +{HASH}(pragma)[^\n]+ {
> yylval->str = ralloc_strdup (yyextra, yytext);
> yylineno++;
> yycolumn = 0;
> return OTHER;
> }
>
> + /* glcpp will handle the #extension directive if
> + * and only if parser->directive_ext_list is non-NULL
> + * otherwise it will just send that off to the parser */
> +{HASH}(extension)[^\n]+ {
> + yylval->str = ralloc_strdup (yyextra, yytext);
> + yylineno++;
> + yycolumn = 0;
> + if(parser->directive_ext_list) {
> + return EXTENSION_DIRECTIVE;
> + } else {
> + return OTHER;
> + }
> +}
> +
> +
> {HASH}line{HSPACE}+ {
> return HASH_LINE;
> }
> diff --git a/src/glsl/glcpp/glcpp-parse.y b/src/glsl/glcpp/glcpp-parse.y
> index 7edc274..d1d58c3 100644
> --- a/src/glsl/glcpp/glcpp-parse.y
> +++ b/src/glsl/glcpp/glcpp-parse.y
> @@ -166,6 +166,7 @@ add_builtin_define(glcpp_parser_t *parser, const char *name, int value);
> %expect 0
> %token COMMA_FINAL DEFINED ELIF_EXPANDED HASH HASH_DEFINE FUNC_IDENTIFIER OBJ_IDENTIFIER HASH_ELIF HASH_ELSE HASH_ENDIF HASH_IF HASH_IFDEF HASH_IFNDEF HASH_LINE HASH_UNDEF HASH_VERSION IDENTIFIER IF_EXPANDED INTEGER INTEGER_STRING LINE_EXPANDED NEWLINE OTHER PLACEHOLDER SPACE
> %token PASTE
> +%token <str> EXTENSION_DIRECTIVE
> %type <ival> expression INTEGER operator SPACE integer_constant
> %type <str> IDENTIFIER FUNC_IDENTIFIER OBJ_IDENTIFIER INTEGER_STRING OTHER
> %type <string_list> identifier_list
> @@ -208,9 +209,34 @@ line:
> ralloc_free ($1);
> }
> | expanded_line
> +| extension_directive
> | HASH non_directive
> ;
>
> +extension_directive:
> + EXTENSION_DIRECTIVE {
> + int cnt;
> +
> + assert(parser->directive_ext_list);
> + /*
> + the lexer increments yylineno when it encounters
> + #extension. If it did not, then the line used by
> + the glsl parser when then be wrong. We get around
> + the issue by decrementing by number of #extension
> + processed so far.
> + */
> + cnt=parser->directive_ext_list->count;
> + add_extension_from_preprocessor($1,
> + @1.first_line - cnt,
> + @1.first_column,
> + @1.last_line - cnt,
> + @1.last_column,
> + @1.source,
> + parser->directive_ext_list);
> + }
> +;
> +
> +
> expanded_line:
> IF_EXPANDED expression NEWLINE {
> _glcpp_parser_skip_stack_push_if (parser, & @1, $2);
> @@ -1148,7 +1174,8 @@ static void add_builtin_define(glcpp_parser_t *parser,
> }
>
> glcpp_parser_t *
> -glcpp_parser_create (const struct gl_extensions *extensions, int api)
> +glcpp_parser_create (const struct gl_extensions *extensions, int api,
> + struct glcpp_extension_directive_list *ext_list)
> {
> glcpp_parser_t *parser;
> int language_version;
> @@ -1182,6 +1209,7 @@ glcpp_parser_create (const struct gl_extensions *extensions, int api)
> parser->new_source_number = 0;
>
> parser->is_gles = false;
> + parser->directive_ext_list = ext_list;
>
> /* Add pre-defined macros. */
> if (api == API_OPENGLES2) {
> @@ -2098,3 +2126,4 @@ _glcpp_parser_handle_version_declaration(glcpp_parser_t *parser, intmax_t versio
> es_identifier ? " " : "",
> es_identifier ? es_identifier : "");
> }
> +
> diff --git a/src/glsl/glcpp/glcpp.c b/src/glsl/glcpp/glcpp.c
> index 6994d7b..bc2ac3c 100644
> --- a/src/glsl/glcpp/glcpp.c
> +++ b/src/glsl/glcpp/glcpp.c
> @@ -164,7 +164,7 @@ main (int argc, char *argv[])
> if (shader == NULL)
> return 1;
>
> - ret = glcpp_preprocess(ctx, &shader, &info_log, NULL, &gl_ctx);
> + ret = glcpp_preprocess(ctx, &shader, &info_log, NULL, &gl_ctx, NULL);
>
> printf("%s", shader);
> fprintf(stderr, "%s", info_log);
> diff --git a/src/glsl/glcpp/glcpp.h b/src/glsl/glcpp/glcpp.h
> index 8aaa551..e7835cf 100644
> --- a/src/glsl/glcpp/glcpp.h
> +++ b/src/glsl/glcpp/glcpp.h
> @@ -28,6 +28,7 @@
> #include <stdbool.h>
>
> #include "main/mtypes.h"
> +#include "glcpp_extras.h"
>
> #include "../ralloc.h"
>
> @@ -185,12 +186,14 @@ struct glcpp_parser {
> bool has_new_source_number;
> int new_source_number;
> bool is_gles;
> + struct glcpp_extension_directive_list *directive_ext_list;
> };
>
> struct gl_extensions;
>
> glcpp_parser_t *
> -glcpp_parser_create (const struct gl_extensions *extensions, int api);
> +glcpp_parser_create (const struct gl_extensions *extensions, int api,
> + struct glcpp_extension_directive_list *ext_list);
>
> int
> glcpp_parser_parse (glcpp_parser_t *parser);
> @@ -198,9 +201,19 @@ glcpp_parser_parse (glcpp_parser_t *parser);
> void
> glcpp_parser_destroy (glcpp_parser_t *parser);
>
> +/**
> + Execute pre-processor on source code provided by *shader
> + and writing output to *shader on success
> + \param ext_list if NON-null indicates that the pre-processor
> + should intercept #extension directives
> + and build a list of the directives. The
> + outputted source code will have those
> + directive missing.
> + */
> int
> glcpp_preprocess(void *ralloc_ctx, const char **shader, char **info_log,
> - const struct gl_extensions *extensions, struct gl_context *g_ctx);
> + const struct gl_extensions *extensions, struct gl_context *g_ctx,
> + struct glcpp_extension_directive_list *ext_list);
>
> /* Functions for writing to the info log */
>
> diff --git a/src/glsl/glcpp/glcpp_extras.c b/src/glsl/glcpp/glcpp_extras.c
> new file mode 100644
> index 0000000..1672f9f
> --- /dev/null
> +++ b/src/glsl/glcpp/glcpp_extras.c
> @@ -0,0 +1,126 @@
> +#include <string.h>
> +#include <ctype.h>
> +#include "glcpp_extras.h"
> +#include "main/imports.h"
> +
> +#define MAX_STRING_LENGTH 200
> +
> +static const char*
> +remove_surrounding_white_space(char *value)
> +{
> + size_t len;
> + char *back;
> +
> + /*
> + remark:
> + -since the return value of remove_surrounding_white_space
> + is to never be freed, we can safely return a non-NULL
> + "empty" string if value was NULL.
> + */
> +
> + if(value==NULL || *value==0) {
> + return "";
> + }
> +
> + while(*value!=0 && isspace(*value)) {
> + ++value;
> + }
> +
> + len=strlen(value);
> + if(len==0) {
> + return value;
> + }
> +
> + for(back = value + len - 1; back!=value && isspace(*back); --back) {
> + *back=0;
> + }
> +
> + return value;
> +}
> +
> +static char*
> +copy_string(const char *src)
> +{
> + char *dst;
> + size_t len;
> +
> + len=strnlen(src, MAX_STRING_LENGTH);
> + dst=MALLOC(len+1);
> + strncpy(dst, src, len);
> +
> + dst[len]=0;
> + return dst;
> +}
> +
> +static
> +void
> +delete_glcpp_extension_directive_entry(struct glcpp_extension_directive_list_element *p)
> +{
> + FREE(p->line_string);
> + FREE(p->strtoked_string);
> + FREE(p);
> +}
> +
> +void
> +init_glcpp_extension_directive_list(struct glcpp_extension_directive_list *p)
> +{
> + assert(p!=NULL);
> + p->head=p->tail=NULL;
> + p->count=0;
> +}
> +
> +void
> +delete_glcpp_extension_directive_list_contents(struct glcpp_extension_directive_list *p)
> +{
> + struct glcpp_extension_directive_list_element *current;
> +
> + assert(p!=NULL);
> + current=p->head;
> + while(current!=NULL) {
> + struct glcpp_extension_directive_list_element *next;
> +
> + next=current->next;
> + delete_glcpp_extension_directive_entry(current);
> + current=next;
> + }
> + p->head=p->tail=NULL;
> + p->count=0;
> +}
> +
> +void
> +add_extension_from_preprocessor(const char *l,
> + int first_line,
> + int first_column,
> + int last_line,
> + int last_column,
> + unsigned source,
> + struct glcpp_extension_directive_list *directive_list)
> +{
> + struct glcpp_extension_directive_list_element *current;
> + char *save_strok_state;
> +
> + current=MALLOC_STRUCT(glcpp_extension_directive_list_element);
> + current->line_string=copy_string(l);
> +
> + current->location.first_line = first_line;
> + current->location.first_column = first_column;
> + current->location.last_line = last_line;
> + current->location.last_column = last_column;
> + current->location.source = source;
> +
> + current->strtoked_string=copy_string(l);
> + strtok_r(current->strtoked_string, " ", &save_strok_state); //get #extension
> + current->name=remove_surrounding_white_space(strtok_r(NULL, ":", &save_strok_state));
> + current->behavior_string=remove_surrounding_white_space(strtok_r(NULL, " ", &save_strok_state));
> + current->next=NULL;
> +
> + if(directive_list->tail==NULL) {
> + assert(directive_list->head==NULL);
> + directive_list->head=current;
> + } else {
> + directive_list->tail->next=current;
> + }
> + directive_list->tail=current;
> + directive_list->count++;
> +}
> +
> diff --git a/src/glsl/glcpp/glcpp_extras.h b/src/glsl/glcpp/glcpp_extras.h
> new file mode 100644
> index 0000000..3c67ea9
> --- /dev/null
> +++ b/src/glsl/glcpp/glcpp_extras.h
> @@ -0,0 +1,97 @@
> +#ifndef GLCPP_EXTRAS_H
> +#define GLCPP_EXTRAS_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * Specifies a single #extension directive picked up
> + * by the pre-processor.
> + */
> +struct glcpp_extension_directive_list_element {
> +
> + char *line_string; /*!< string from source line of GLSL */
> +
> + struct {
> + int first_line;
> + int first_column;
> + int last_line;
> + int last_column;
> + unsigned source;
> + } location; /*!< location of directive in GLSL source */
> +
> + /**
> + name of extension, is a substring of
> + \ref strtoked_string or a substring of
> + a const (compiled) string
> + */
> + const char *name;
> +
> + /**
> + behavior of extension, is a substring of
> + \ref strtoked_string or a substring of
> + a const (compiled) string
> + */
> + const char *behavior_string;
> +
> + /**
> + * next element in linked list
> + */
> + struct glcpp_extension_directive_list_element *next;
> +
> + char *strtoked_string; /*!< original string modified by strtok_r */
> +};
> +
> +/**
> + Convenience wrapper for linked list of elements.
> + */
> +struct glcpp_extension_directive_list {
> + struct glcpp_extension_directive_list_element *head;
> + struct glcpp_extension_directive_list_element *tail;
> + int count;
> +};
> +
> +/**
> + * Initialize the contents of a \ref glcpp_extension_directive_list
> + * \param l struct to initialize, must not be NULL
> + */
> +extern void
> +init_glcpp_extension_directive_list(struct glcpp_extension_directive_list *l);
> +
> +/**
> + Delete the <i>contents</i> of a \ref glcpp_extension_directive_list.
> + Does NOT delete the containing struct.
> + \param l struct to clear, must not be NULL.
> + */
> +extern void
> +delete_glcpp_extension_directive_list_contents(struct glcpp_extension_directive_list *l);
> +
> +/**
> + * Means of allowing #extension directives to come
> + * -after- source code by processing the directives
> + * in the pre-processor.
> + *
> + * \param line_string line of source of the extension directive, string is copied
> + * \param first_line value of first_line field of location of directive
> + * \param first_column value of first_column field of location of directive
> + * \param last_line value of last_line field of location of directive
> + * \param last_column value of field last_column of location of directive
> + * \param source value of source field of location of directive
> + * \param l list to which to append the directive
> + */
> +extern void
> +add_extension_from_preprocessor(const char *line_string,
> + int first_line,
> + int first_column,
> + int last_line,
> + int last_column,
> + unsigned source,
> + struct glcpp_extension_directive_list *l);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +
> +#endif
> diff --git a/src/glsl/glcpp/pp.c b/src/glsl/glcpp/pp.c
> index 7e1b6c6..48176a6 100644
> --- a/src/glsl/glcpp/pp.c
> +++ b/src/glsl/glcpp/pp.c
> @@ -136,10 +136,11 @@ remove_line_continuations(glcpp_parser_t *ctx, const char *shader)
>
> int
> glcpp_preprocess(void *ralloc_ctx, const char **shader, char **info_log,
> - const struct gl_extensions *extensions, struct gl_context *gl_ctx)
> + const struct gl_extensions *extensions, struct gl_context *gl_ctx,
> + struct glcpp_extension_directive_list *ext_list)
> {
> int errors;
> - glcpp_parser_t *parser = glcpp_parser_create (extensions, gl_ctx->API);
> + glcpp_parser_t *parser = glcpp_parser_create (extensions, gl_ctx->API, ext_list);
>
> if (! gl_ctx->Const.DisableGLSLLineContinuations)
> *shader = remove_line_continuations(parser, *shader);
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index d76d94b..75640e1 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -603,6 +603,41 @@ static const _mesa_glsl_extension *find_extension(const char *name)
> return NULL;
> }
>
> +/**
> + * Walks the directive list built by the preprocessor and
> + * sets the parser state to enable them, in addition frees
> + * the elements of the linked list.
> + */
> +static
> +void
> +_mesa_glsl_enable_extensions_from_cpp(struct _mesa_glsl_parse_state *state)
> +{
> + if(state->ext_directive_list) {
> + YYLTYPE location;
> + struct glcpp_extension_directive_list_element *current;
> + bool extension_ok;
> +
> + for(current=state->ext_directive_list->head; current!=NULL; current=current->next) {
> + location.first_line = current->location.first_line;
> + location.first_column = current->location.first_column;
> + location.last_line = current->location.last_line;
> + location.last_column = current->location.last_column;
> + location.source = current->location.source;
> + extension_ok=_mesa_glsl_process_extension(current->name, &location,
> + current->behavior_string, &location,
> + state);
> +
> + if(!extension_ok) {
> + _mesa_glsl_error(&location, state, "%s", current->line_string);
> + }
> +
> + }
> + delete_glcpp_extension_directive_list_contents(state->ext_directive_list);
> + state->ext_directive_list=NULL;
> + }
> +}
> +
> +
>
> bool
> _mesa_glsl_process_extension(const char *name, YYLTYPE *name_locp,
> @@ -1475,15 +1510,33 @@ void
> _mesa_glsl_compile_shader(struct gl_context *ctx, struct gl_shader *shader,
> bool dump_ast, bool dump_hir)
> {
> + struct glcpp_extension_directive_list extension_directive_list;
> struct _mesa_glsl_parse_state *state =
> new(shader) _mesa_glsl_parse_state(ctx, shader->Type, shader);
> const char *source = shader->Source;
>
> + if(1) {
> + state->ext_directive_list=&extension_directive_list;
> + init_glcpp_extension_directive_list(state->ext_directive_list);
> + } else {
> + state->ext_directive_list=NULL;
> + }
> +
> state->error = glcpp_preprocess(state, &source, &state->info_log,
> - &ctx->Extensions, ctx);
> + &ctx->Extensions, ctx,
> + state->ext_directive_list);
>
> +
> if (!state->error) {
> _mesa_glsl_lexer_ctor(state, source);
> +
> + /*
> + before parsing, walk the directive list
> + and enable the extensions.
> + */
> + _mesa_glsl_enable_extensions_from_cpp(state);
> +
> +
> _mesa_glsl_parse(state);
> _mesa_glsl_lexer_dtor(state);
> }
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index d232bb3..c163ae5 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -25,6 +25,9 @@
> #ifndef GLSL_PARSER_EXTRAS_H
> #define GLSL_PARSER_EXTRAS_H
>
> +
> +#include "glcpp/glcpp_extras.h"
> +
> /*
> * Most of the definitions here only apply to C++
> */
> @@ -376,6 +379,14 @@ struct _mesa_glsl_parse_state {
>
> /** Atomic counter offsets by binding */
> unsigned atomic_counter_offsets[MAX_COMBINED_ATOMIC_BUFFERS];
> +
> + /**
> + If non-NULL gives a list of directives picked up
> + by the preprocessor.
> + */
> + struct glcpp_extension_directive_list *ext_directive_list;
> +
> +
> };
>
> # define YYLLOC_DEFAULT(Current, Rhs, N) \
> @@ -450,7 +461,8 @@ extern const char *
> _mesa_glsl_shader_target_name(GLenum type);
>
> extern int glcpp_preprocess(void *ctx, const char **shader, char **info_log,
> - const struct gl_extensions *extensions, struct gl_context *gl_ctx);
> + const struct gl_extensions *extensions, struct gl_context *gl_ctx,
> + struct glcpp_extension_directive_list *ext_list);
>
> extern void _mesa_destroy_shader_compiler(void);
> extern void _mesa_destroy_shader_compiler_caches(void);
>
More information about the mesa-dev
mailing list