* [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas @ 2019-06-12 18:25 Oliver Browne 2019-07-04 0:02 ` Jeff Law 0 siblings, 1 reply; 4+ messages in thread From: Oliver Browne @ 2019-06-12 18:25 UTC (permalink / raw) To: gcc-patches Patch fixes following PRs: c++/90816 - -finstrument-functions-exclude-function-list improperly handles namespace/class definitions c++/90809 - -finstrument-functions-exclude-function-list mishandles comma escaping Fixes as follows: At flag_instrument_functions_exclude_p [gimplify.c] Using lang_hooks.decl_printable_name (fndecl, 1) to get namespace / class information as part of printable name to allow for inclusion of namespace / class specification when passing symbols to -finstrument-functions-exclude-function-list. Was previously lang_hooks.decl_printable_name (fndecl, 0). At add_comma_separated_to_vector [opts.c] Added writing of a null character to w after primary loop finishes, to account for offset between r and w when r reaches end of passed string. from Oliver Browne <oliverbrowne627@gmail.com> PR c++/90816 PR c++/90809 * gimplify.c (flag_instrument_functions_exclude_p): include namespace information as part of decl name * opts.c (add_comma_separated_to_vector): add null character to correct position in last token added to token vector Index: gimplify.c =================================================================== --- gimplify.c 2019-06-12 19:07:26.872077000 +0100 +++ gimplify.c 2019-06-12 18:55:10.609255000 +0100 @@ -13987,11 +13987,17 @@ flag_instrument_functions_exclude_p (tre { const char *name; - int i; + unsigned int i; char *s; - name = lang_hooks.decl_printable_name (fndecl, 0); - FOR_EACH_VEC_ELT (*v, i, s) + name = lang_hooks.decl_printable_name (fndecl, 1); + for(i = 0; i < v->length(); i++){ + s = (*v)[i]; + if(strstr(name, s) != NULL){ + return(true); + } + } +/* FOR_EACH_VEC_ELT (*v, i, s) if (strstr (name, s) != NULL) - return true; + return true;*/ } @@ -14278,3 +14284,3 @@ gimplify_hasher::equal (const elt_t *p1, return true; -} \ No newline at end of file +} Index: opts.c =================================================================== --- opts.c 2019-06-12 19:10:04.354612000 +0100 +++ opts.c 2019-06-12 18:53:43.675852000 +0100 @@ -263,7 +263,8 @@ add_comma_separated_to_vector (void **pv *w++ = *r++; } - if (*token_start != '\0') + *w = '\0'; + if (*token_start != '\0'){ v->safe_push (token_start); - + } *pvec = v; } @@ -3151,3 +3152,3 @@ option_name (diagnostic_context *context else return NULL; -} \ No newline at end of file +} ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas 2019-06-12 18:25 [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas Oliver Browne @ 2019-07-04 0:02 ` Jeff Law 2019-07-04 8:28 ` Oliver Browne 0 siblings, 1 reply; 4+ messages in thread From: Jeff Law @ 2019-07-04 0:02 UTC (permalink / raw) To: Oliver Browne, gcc-patches On 6/12/19 12:25 PM, Oliver Browne wrote: > Patch fixes following PRs: > c++/90816 - -finstrument-functions-exclude-function-list improperly > handles namespace/class definitions > c++/90809 - -finstrument-functions-exclude-function-list mishandles > comma escaping > > Fixes as follows: > At flag_instrument_functions_exclude_p [gimplify.c] > Using lang_hooks.decl_printable_name (fndecl, 1) to get namespace / > class information as part of printable name to allow for > inclusion of namespace / class specification when passing symbols to > -finstrument-functions-exclude-function-list. Was > previously lang_hooks.decl_printable_name (fndecl, 0). > > At add_comma_separated_to_vector [opts.c] > Added writing of a null character to w after primary loop finishes, to > account for offset between r and w when r reaches end of > passed string. > > from Oliver Browne <oliverbrowne627@gmail.com> > PR c++/90816 > PR c++/90809 > * gimplify.c (flag_instrument_functions_exclude_p): include namespace > information as part of decl name > * opts.c (add_comma_separated_to_vector): add null character to correct > position in last token added to token vector > Index: gimplify.c > =================================================================== > --- gimplify.c 2019-06-12 19:07:26.872077000 +0100 > +++ gimplify.c 2019-06-12 18:55:10.609255000 +0100 > @@ -13987,11 +13987,17 @@ flag_instrument_functions_exclude_p (tre > { > const char *name; > - int i; > + unsigned int i; > char *s; > > - name = lang_hooks.decl_printable_name (fndecl, 0); > - FOR_EACH_VEC_ELT (*v, i, s) > + name = lang_hooks.decl_printable_name (fndecl, 1); > + for(i = 0; i < v->length(); i++){ > + s = (*v)[i]; > + if(strstr(name, s) != NULL){ > + return(true); > + } > + } > +/* FOR_EACH_VEC_ELT (*v, i, s) > if (strstr (name, s) != NULL) > - return true; > + return true;*/ > } So why did you drop the FOR_EACH_VEC_ELT and open-code the loop? I don't see that as being a necessary change. Leaving the FOR_EACH_VEC_ELT in place would also avoid the mis-formatting you've introduced as well as removing clutter of a commented-out hunk of code. > > @@ -14278,3 +14284,3 @@ gimplify_hasher::equal (const elt_t *p1, > > return true; > -} > \ No newline at end of file > +} > Index: opts.c > =================================================================== > --- opts.c 2019-06-12 19:10:04.354612000 +0100 > +++ opts.c 2019-06-12 18:53:43.675852000 +0100 > @@ -263,7 +263,8 @@ add_comma_separated_to_vector (void **pv > *w++ = *r++; > } > - if (*token_start != '\0') > + *w = '\0'; > + if (*token_start != '\0'){ > v->safe_push (token_start); > - > + } > *pvec = v; So why introduce the unnecessary { } scope? And why do it in a way that is different than 99% of the other code in GCC (where the { and } will be on individual lines with 2 spaces of indention? Jeff ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas 2019-07-04 0:02 ` Jeff Law @ 2019-07-04 8:28 ` Oliver Browne 2019-07-24 18:19 ` Jeff Law 0 siblings, 1 reply; 4+ messages in thread From: Oliver Browne @ 2019-07-04 8:28 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches See below for modified patch, indentation and newline for curly braces style applied, and commented out chunk removed. Apologies, indentation and newline for scope are not they way I normally write things, habits got the better of me, and I forgot to remove the commented out chunk before submission. Adding the scope braces and writing the for loop in ordinary C instead of relying on a macro are changes made for the sake of maintainability. This feature is rarely touched (as evidenced by it having the bugs I'm fixing for so long) and so it is more likely any patches submitted to it will be submitted by people who a) are not maintainers of GCC, and so are not familiar with the various macros etc "normally" used and b) don't have the time / patience to familiarize themselves with the code base before fixing just the bit they need to work for whatever they're doing. For people like that (people like me), having the code be written in as obvious a way as possible and clearly marking scope of branches makes making fixes easier (adding a quick printf for debugging is painless, etc), which is important for low usage features. Index: gimplify.c =================================================================== --- gimplify.c 2019-06-12 19:07:26.872077000 +0100 +++ gimplify.c 2019-07-04 08:53:08.768420000 +0100 @@ -13987,11 +13987,16 @@ flag_instrument_functions_exclude_p (tre { const char *name; - int i; + unsigned int i; char *s; - name = lang_hooks.decl_printable_name (fndecl, 0); - FOR_EACH_VEC_ELT (*v, i, s) - if (strstr (name, s) != NULL) - return true; + name = lang_hooks.decl_printable_name (fndecl, 1); + for(i = 0; i < v->length(); i++) + { + s = (*v)[i]; + if(strstr(name, s) != NULL) + { + return(true); + } + } } Index: opts.c =================================================================== --- opts.c 2019-06-12 19:10:04.354612000 +0100 +++ opts.c 2019-07-04 08:55:25.287523000 +0100 @@ -263,7 +263,9 @@ add_comma_separated_to_vector (void **pv *w++ = *r++; } + *w = '\0'; if (*token_start != '\0') + { v->safe_push (token_start); - + } *pvec = v; } On Thu, Jul 4, 2019 at 12:31 AM Jeff Law <law@redhat.com> wrote: > > On 6/12/19 12:25 PM, Oliver Browne wrote: > > Patch fixes following PRs: > > c++/90816 - -finstrument-functions-exclude-function-list improperly > > handles namespace/class definitions > > c++/90809 - -finstrument-functions-exclude-function-list mishandles > > comma escaping > > > > Fixes as follows: > > At flag_instrument_functions_exclude_p [gimplify.c] > > Using lang_hooks.decl_printable_name (fndecl, 1) to get namespace / > > class information as part of printable name to allow for > > inclusion of namespace / class specification when passing symbols to > > -finstrument-functions-exclude-function-list. Was > > previously lang_hooks.decl_printable_name (fndecl, 0). > > > > At add_comma_separated_to_vector [opts.c] > > Added writing of a null character to w after primary loop finishes, to > > account for offset between r and w when r reaches end of > > passed string. > > > > from Oliver Browne <oliverbrowne627@gmail.com> > > PR c++/90816 > > PR c++/90809 > > * gimplify.c (flag_instrument_functions_exclude_p): include namespace > > information as part of decl name > > * opts.c (add_comma_separated_to_vector): add null character to correct > > position in last token added to token vector > > Index: gimplify.c > > =================================================================== > > --- gimplify.c 2019-06-12 19:07:26.872077000 +0100 > > +++ gimplify.c 2019-06-12 18:55:10.609255000 +0100 > > @@ -13987,11 +13987,17 @@ flag_instrument_functions_exclude_p (tre > > { > > const char *name; > > - int i; > > + unsigned int i; > > char *s; > > > > - name = lang_hooks.decl_printable_name (fndecl, 0); > > - FOR_EACH_VEC_ELT (*v, i, s) > > + name = lang_hooks.decl_printable_name (fndecl, 1); > > + for(i = 0; i < v->length(); i++){ > > + s = (*v)[i]; > > + if(strstr(name, s) != NULL){ > > + return(true); > > + } > > + } > > +/* FOR_EACH_VEC_ELT (*v, i, s) > > if (strstr (name, s) != NULL) > > - return true; > > + return true;*/ > > } > So why did you drop the FOR_EACH_VEC_ELT and open-code the loop? I > don't see that as being a necessary change. Leaving the > FOR_EACH_VEC_ELT in place would also avoid the mis-formatting you've > introduced as well as removing clutter of a commented-out hunk of code. > > > > > @@ -14278,3 +14284,3 @@ gimplify_hasher::equal (const elt_t *p1, > > > > return true; > > -} > > \ No newline at end of file > > +} > > Index: opts.c > > =================================================================== > > --- opts.c 2019-06-12 19:10:04.354612000 +0100 > > +++ opts.c 2019-06-12 18:53:43.675852000 +0100 > > @@ -263,7 +263,8 @@ add_comma_separated_to_vector (void **pv > > *w++ = *r++; > > } > > - if (*token_start != '\0') > > + *w = '\0'; > > + if (*token_start != '\0'){ > > v->safe_push (token_start); > > - > > + } > > *pvec = v; > So why introduce the unnecessary { } scope? And why do it in a way that > is different than 99% of the other code in GCC (where the { and } will > be on individual lines with 2 spaces of indention? > > Jeff ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas 2019-07-04 8:28 ` Oliver Browne @ 2019-07-24 18:19 ` Jeff Law 0 siblings, 0 replies; 4+ messages in thread From: Jeff Law @ 2019-07-24 18:19 UTC (permalink / raw) To: Oliver Browne; +Cc: gcc-patches On 7/4/19 2:18 AM, Oliver Browne wrote: > See below for modified patch, indentation and newline for curly braces > style applied, and commented out chunk removed. Apologies, indentation > and newline for scope are not they way I normally write things, habits > got the better of me, and I forgot to remove the commented out chunk > before submission. > > Adding the scope braces and writing the for loop in ordinary C instead > of relying on a macro are changes made for the sake of > maintainability. The right way to do this in GCC is to use the facilities we have designed for these purposes. In some cases there are constraints that those facilities enforce and doing an open-coded implementation will not work the way you want (the immediate use iterators immediately come to mind) and in other cases the facilities we've built may be much faster (the bitmap iterators) and in some cases it's just syntatic sugar, but the consistency with the rest of the codebase is important. When you don't follow conventions, the maintainer looking at your patch has to disentangle the real change from your formatting and convention changes, then apply the fix by hand, possibly getting it wrong in the process (made even more possible by the lack of a test in the patch). THen because they made changes, they'll have to sanity tests to make sure they didn't goof anything with a typo, etc which takes even more of their limited time. If you make it easy to review the patch by following conventions, not making extraneous changes, include tests, indicate that you regression tested your patch, etc, then it becomes very easy for a maintainer to look at the patch and gate it in. Anyway, I've fixed up your patch to follow existing conventions, bootstrapped and regression tested it and installed the patch on the trunk. jeff ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-07-24 18:09 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-12 18:25 [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas Oliver Browne 2019-07-04 0:02 ` Jeff Law 2019-07-04 8:28 ` Oliver Browne 2019-07-24 18:19 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).