[Mesa-dev] [PATCH v2] glcpp: Sync line number for macro
Zhaowei Yuan
zhaowei.yuan at samsung.com
Fri Aug 10 06:23:43 UTC 2018
Okay, got it, thank you for your verification
On 8/10/2018 2:15 PM, Tapani Pälli <tapani.palli at intel.com> wrote:
> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> Behalf Of Tapani P?lli
> Sent: Friday, August 10, 2018 2:15 PM
> To: Zhaowei Yuan; mesa-dev at lists.freedesktop.org
> Cc: vlad.golovkin.mail at gmail.com; kenneth at whitecape.org;
> ian.d.romanick at intel.com; tarceri at itsqueeze.com;
> timothy.arceri at collabora.com
> Subject: Re: [Mesa-dev] [PATCH v2] glcpp: Sync line number for macro
> Importance: High
>
> Hi;
>
> On 08/10/2018 09:08 AM, Zhaowei Yuan wrote:
> > Thank you for reminding me of the mistakes, I'll re-commit a PATCH v3,
> > thanks
> > However, I've re-tested those failed cases with latest code on master
> branch
> > and got the same result. I don't why you didn't see anything wrong on your
> > board,
> > it doesn't make sense.
>
> Sorry for confusion, what I meant is that I did not see any regressions
> that would occur from these changes. Only change was that they fix those
> preprocessor tests mentioned.
>
> > On 08/07/2018 5:58 PM, Tapani Pälli <tapani.palli at intel.com> wrote:
> >> On 07/22/2018 11:36 PM, zhaowei yuan wrote:
> >>> Line number of a predefined macro should be set
> >>> as where it is referenced rather than declared
> >>>
> >>> Patch v2 does nothing more except ccing more maintainers
> >>>
> >>> Signed-off-by: zhaowei yuan <zhaowei.yuan at samsung.com>
> >>> Bugzilla: https://patchwork.freedesktop.org/patch/225818/
> >>
> >> Probably a copy-paste mistake here, this should be the bug url instead
> >> of patchwork one, https://bugs.freedesktop.org/show_bug.cgi?id=99730.
> >>
> >> IMO commit message should also mention the tests that it fixes, which
> are:
> >>
> >> dEQP-
> >>
> GLES2.functional.shaders.preprocessor.predefined_macros.line_2_vertex
> >> dEQP-
> >>
> GLES2.functional.shaders.preprocessor.predefined_macros.line_2_fragment
> >>
> >> (same tests exist also in dEQP-GLES3)
> >>
> >> I've run this through Intel CI and did not see any failures, I also run
> >> some dEQP preprocessor tests with valgrind and did not see anything
> >> reported;
> >>
> >> Tested-by: Tapani Pälli <tapani.palli at intel.com>
> >>
> >> some small coding style issues noted below:
> >>
> >>> ---
> >>> src/compiler/glsl/glcpp/glcpp-lex.l | 1 +
> >>> src/compiler/glsl/glcpp/glcpp-parse.y | 55
> >> ++++++++++++++++++++++++++++++-----
> >>> src/compiler/glsl/glcpp/glcpp.h | 4 ++-
> >>> src/compiler/glsl/glcpp/pp.c | 3 +-
> >>> 4 files changed, 54 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/src/compiler/glsl/glcpp/glcpp-lex.l
> >> b/src/compiler/glsl/glcpp/glcpp-lex.l
> >>> index 9cfcc12..86b82c2 100644
> >>> --- a/src/compiler/glsl/glcpp/glcpp-lex.l
> >>> +++ b/src/compiler/glsl/glcpp/glcpp-lex.l
> >>> @@ -50,6 +50,7 @@ void glcpp_set_column (int column_no , yyscan_t
> >> yyscanner);
> >>> yylloc->first_line = yylloc->last_line = yylineno; \
> >>> yycolumn += yyleng; \
> >>> yylloc->last_column = yycolumn + 1; \
> >>> + yylloc->position = (yytext -
> >> YY_CURRENT_BUFFER_LVALUE->yy_ch_buf); \
> >>> parser->has_new_line_number = 0; \
> >>> parser->has_new_source_number = 0;
> >> \
> >>> } while(0);
> >>> diff --git a/src/compiler/glsl/glcpp/glcpp-parse.y
> >> b/src/compiler/glsl/glcpp/glcpp-parse.y
> >>> index ccb3aa1..68f8477 100644
> >>> --- a/src/compiler/glsl/glcpp/glcpp-parse.y
> >>> +++ b/src/compiler/glsl/glcpp/glcpp-parse.y
> >>> @@ -1021,7 +1021,7 @@ _token_list_append_list(token_list_t *list,
> >> token_list_t *tail)
> >>> }
> >>>
> >>> static token_list_t *
> >>> -_token_list_copy(glcpp_parser_t *parser, token_list_t *other)
> >>> +_token_list_copy(glcpp_parser_t *parser, token_list_t *other,
> >> token_node_t *macro_node)
> >>> {
> >>> token_list_t *copy;
> >>> token_node_t *node;
> >>> @@ -1033,6 +1033,12 @@ _token_list_copy(glcpp_parser_t *parser,
> >> token_list_t *other)
> >>> for (node = other->head; node; node = node->next) {
> >>> token_t *new_token = linear_alloc_child(parser->linalloc,
> >> sizeof(token_t));
> >>> *new_token = *node->token;
> >>> +
> >>> + if(macro_node) {
> >>
> >> should have space after if
> >>
> >>> + new_token->location.first_line =
> >> macro_node->token->location.first_line;
> >>> + new_token->location.last_line =
> >> macro_node->token->location.last_line;
> >>> + }
> >>> +
> >>> _token_list_append (parser, copy, new_token);
> >>> }
> >>>
> >>> @@ -1349,7 +1355,7 @@ add_builtin_define(glcpp_parser_t *parser,
> const
> >> char *name, int value)
> >>>
> >>> glcpp_parser_t *
> >>> glcpp_parser_create(const struct gl_extensions *extension_list,
> >>> - glcpp_extension_iterator extensions, void *state,
> >> gl_api api)
> >>> + glcpp_extension_iterator extensions, void *state,
> >> gl_api api, const char *input)
> >>> {
> >>> glcpp_parser_t *parser;
> >>>
> >>> @@ -1377,6 +1383,11 @@ glcpp_parser_create(const struct
> gl_extensions
> >> *extension_list,
> >>> parser->lex_from_list = NULL;
> >>> parser->lex_from_node = NULL;
> >>>
> >>> + parser->input = _mesa_string_buffer_create(parser, strlen(input) +
> > 1);
> >>> + strcpy(parser->input->buf, input);
> >>> + parser->input->buf[strlen(input)] = '\0';
> >>> + parser->input->length = strlen(input);
> >>> +
> >>> parser->output = _mesa_string_buffer_create(parser,
> >>>
> >> INITIAL_PP_OUTPUT_BUF_SIZE);
> >>> parser->info_log = _mesa_string_buffer_create(parser,
> >>> @@ -1441,7 +1452,7 @@ typedef enum function_status
> >>> static function_status_t
> >>> _arguments_parse(glcpp_parser_t *parser,
> >>> argument_list_t *arguments, token_node_t *node,
> >>> - token_node_t **last)
> >>> + token_node_t **last, int *end_position)
> >>> {
> >>> token_list_t *argument;
> >>> int paren_count;
> >>> @@ -1465,8 +1476,10 @@ _arguments_parse(glcpp_parser_t *parser,
> >>> paren_count++;
> >>> } else if (node->token->type == ')') {
> >>> paren_count--;
> >>> - if (paren_count == 0)
> >>> + if (paren_count == 0) {
> >>> + *end_position = node->token->location.position;
> >>> break;
> >>> + }
> >>> }
> >>>
> >>> if (node->token->type == ',' && paren_count == 1) {
> >>> @@ -1702,6 +1715,28 @@ _glcpp_parser_apply_pastes(glcpp_parser_t
> >> *parser,
> >> token_list_t *list)
> >>> list->non_space_tail = list->tail;
> >>> }
> >>>
> >>> +static int
> >>> +_glcpp_parser_get_line(glcpp_parser_t *parser, int offset)
> >>> +{
> >>> + int line = 1;
> >>> + int i;
> >>> +
> >>> + for(i = 0; parser->input->buf[i] && i <= offset; i++) {
> >>
> >> should have space after for
> >>
> >>> + if(parser->input->buf[i] == '\n' || parser->input->buf[i] ==
> > '\r')
> >>
> >> should have space after if
> >>
> >>> + line++;
> >>> + }
> >>> +
> >>> + return line;
> >>> +}
> >>> +
> >>> +static void
> >>> +_glcpp_sync_location(glcpp_parser_t *parser, token_t *token,
> token_t
> >> *macro_token)
> >>> +{
> >>> + token->location.source = macro_token->location.source;
> >>> + token->location.first_line = _glcpp_parser_get_line(parser,
> >> token->location.position);
> >>> + token->location.last_line = token->location.first_line;
> >>> +}
> >>> +
> >>> /* This is a helper function that's essentially part of the
> >>> * implementation of _glcpp_parser_expand_node. It shouldn't be
> called
> >>> * except for by that function.
> >>> @@ -1732,7 +1767,9 @@
> _glcpp_parser_expand_function(glcpp_parser_t
> >> *parser, token_node_t *node,
> >>> argument_list_t *arguments;
> >>> function_status_t status;
> >>> token_list_t *substituted;
> >>> + token_node_t *macro_node = node;
> >>> int parameter_index;
> >>> + int end_position;
> >>>
> >>> identifier = node->token->value.str;
> >>>
> >>> @@ -1742,7 +1779,7 @@
> _glcpp_parser_expand_function(glcpp_parser_t
> >> *parser, token_node_t *node,
> >>> assert(macro->is_function);
> >>>
> >>> arguments = _argument_list_create(parser);
> >>> - status = _arguments_parse(parser, arguments, node, last);
> >>> + status = _arguments_parse(parser, arguments, node, last,
> >> &end_position);
> >>>
> >>> switch (status) {
> >>> case FUNCTION_STATUS_SUCCESS:
> >>> @@ -1784,7 +1821,8 @@
> _glcpp_parser_expand_function(glcpp_parser_t
> >> *parser, token_node_t *node,
> >>> * placeholder token for an empty argument. */
> >>> if (argument->head) {
> >>> token_list_t *expanded_argument;
> >>> - expanded_argument = _token_list_copy(parser, argument);
> >>> + expanded_argument = _token_list_copy(parser, argument,
> > NULL);
> >>> + _glcpp_sync_location(parser,
> > expanded_argument->head->token,
> >> macro_node->token);
> >>> _glcpp_parser_expand_token_list(parser, expanded_argument,
> >> mode);
> >>> _token_list_append_list(substituted, expanded_argument);
> >>> } else {
> >>> @@ -1795,6 +1833,8 @@
> _glcpp_parser_expand_function(glcpp_parser_t
> >> *parser, token_node_t *node,
> >>> _token_list_append(parser, substituted, new_token);
> >>> }
> >>> } else {
> >>> + node->token->location.position = end_position;
> >>> + _glcpp_sync_location(parser, node->token, macro_node->token);
> >>> _token_list_append(parser, substituted, node->token);
> >>> }
> >>> }
> >>> @@ -1835,6 +1875,7 @@ _glcpp_parser_expand_node(glcpp_parser_t
> >> *parser,
> >> token_node_t *node,
> >>> const char *identifier;
> >>> struct hash_entry *entry;
> >>> macro_t *macro;
> >>> + token_node_t *macro_node = node;
> >>>
> >>> /* We only expand identifiers */
> >>> if (token->type != IDENTIFIER) {
> >>> @@ -1887,7 +1928,7 @@ _glcpp_parser_expand_node(glcpp_parser_t
> >> *parser,
> >> token_node_t *node,
> >>> if (macro->replacements == NULL)
> >>> return _token_list_create_with_one_space(parser);
> >>>
> >>> - replacement = _token_list_copy(parser, macro->replacements);
> >>> + replacement = _token_list_copy(parser, macro->replacements,
> >> macro_node);
> >>> _glcpp_parser_apply_pastes(parser, replacement);
> >>> return replacement;
> >>> }
> >>> diff --git a/src/compiler/glsl/glcpp/glcpp.h
> >> b/src/compiler/glsl/glcpp/glcpp.h
> >>> index c7e382e..eed6923 100644
> >>> --- a/src/compiler/glsl/glcpp/glcpp.h
> >>> +++ b/src/compiler/glsl/glcpp/glcpp.h
> >>> @@ -78,6 +78,7 @@ typedef struct YYLTYPE {
> >>> int first_column;
> >>> int last_line;
> >>> int last_column;
> >>> + int position;
> >>> unsigned source;
> >>> } YYLTYPE;
> >>> # define YYLTYPE_IS_DECLARED 1
> >>> @@ -204,6 +205,7 @@ struct glcpp_parser {
> >>> token_list_t *lex_from_list;
> >>> token_node_t *lex_from_node;
> >>> struct _mesa_string_buffer *output;
> >>> + struct _mesa_string_buffer *input;
> >>> struct _mesa_string_buffer *info_log;
> >>> int error;
> >>> glcpp_extension_iterator extensions;
> >>> @@ -229,7 +231,7 @@ struct glcpp_parser {
> >>>
> >>> glcpp_parser_t *
> >>> glcpp_parser_create(const struct gl_extensions *extension_list,
> >>> - glcpp_extension_iterator extensions, void *state,
> >> gl_api api);
> >>> + glcpp_extension_iterator extensions, void *state,
> >> gl_api api, const char *input);
> >>>
> >>> int
> >>> glcpp_parser_parse (glcpp_parser_t *parser);
> >>> diff --git a/src/compiler/glsl/glcpp/pp.c b/src/compiler/glsl/glcpp/pp.c
> >>> index 32dee11..1ac733f 100644
> >>> --- a/src/compiler/glsl/glcpp/pp.c
> >>> +++ b/src/compiler/glsl/glcpp/pp.c
> >>> @@ -228,7 +228,7 @@ glcpp_preprocess(void *ralloc_ctx, const char
> >> **shader, char **info_log,
> >>> {
> >>> int errors;
> >>> glcpp_parser_t *parser =
> >>> - glcpp_parser_create(&gl_ctx->Extensions, extensions, state,
> >> gl_ctx->API);
> >>> + glcpp_parser_create(&gl_ctx->Extensions, extensions, state,
> >> gl_ctx->API, *shader);
> >>>
> >>> if (! gl_ctx->Const.DisableGLSLLineContinuations)
> >>> *shader = remove_line_continuations(parser, *shader);
> >>> @@ -247,6 +247,7 @@ glcpp_preprocess(void *ralloc_ctx, const char
> >> **shader, char **info_log,
> >>> /* Crimp the buffer first, to conserve memory */
> >>> _mesa_string_buffer_crimp_to_fit(parser->output);
> >>>
> >>> + ralloc_steal(ralloc_ctx, parser->input->buf);
> >>> ralloc_steal(ralloc_ctx, parser->output->buf);
> >>> *shader = parser->output->buf;
> >>>
> >>>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list