[PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas

[PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas

[Oliver Browne]

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
+}

Re: [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas

[Jeff Law]

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

Re: [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas

[Oliver Browne]

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

Re: [PATH] Patch to fix -finstrument-functions-exclude-function-list handling of namespaces and escaped commas

[Jeff Law]

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