Okay, got it, thank you for your verification On 8/10/2018 2:15 PM, Tapani Pälli <tapani.pa...@intel.com> wrote:
> -----Original Message----- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > Behalf Of Tapani P?lli > Sent: Friday, August 10, 2018 2:15 PM > To: Zhaowei Yuan; mesa-dev@lists.freedesktop.org > Cc: vlad.golovkin.m...@gmail.com; kenn...@whitecape.org; > ian.d.roman...@intel.com; tarc...@itsqueeze.com; > timothy.arc...@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.pa...@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.y...@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.pa...@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@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
