GCC arc port defaults to -fcommon

[Florian Weimer via Gcc]

It seems to me that the arc port still defaults to -fcommon, presumably
due to this in gcc/common/config/arc/arc-common.c:

static void
arc_option_init_struct (struct gcc_options *opts)
{
  opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */

  /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
  arc_cpu = PROCESSOR_NONE;
}

Is that really necessary?  Is -fno-common broken on arc?

I plan to switch glibc to build with -fno-common unconditionally, for
all GCC versions and architectures, and I wonder if that would be a
blocker.

Thanks,
Florian

tree decl stored during LGEN does not map to a symtab_node during WPA

[Erick Ochoa via Gcc]

Hi,

I am saving some tree declarations during LGEN that I will be later
analyzing at WPA time. I am able to read the decl from my summaries
and print it at WPA time. It corresponds to a global variable.
However, whenever I use symtab_node::get (decl) during WPA time I keep
getting NULL.

Does anyone know why that might be the case? Is it possible that other
optimizations are rewriting global variables during LGEN (or prior
WPA)? The variable I am looking at is a static const char typeinfo
name for a class in the program I am analyzing. I don't think this is
an issue since other type info names have an associated symtab_node.

Thanks!

Re: GCC arc port defaults to -fcommon

[Richard Biener via Gcc]

On Wed, Jul 7, 2021 at 11:00 AM Florian Weimer via Gcc  wrote:
>
> It seems to me that the arc port still defaults to -fcommon, presumably
> due to this in gcc/common/config/arc/arc-common.c:
>
> static void
> arc_option_init_struct (struct gcc_options *opts)
> {
>   opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
>
>   /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
>   arc_cpu = PROCESSOR_NONE;
> }
>
> Is that really necessary?  Is -fno-common broken on arc?

It seems arc has -fcommon dependent on !TARGET_NO_SDATA_SET
but it should use global_options_set.x_flag_no_common instead of
such magic value.

> I plan to switch glibc to build with -fno-common unconditionally, for
> all GCC versions and architectures, and I wonder if that would be a
> blocker.
>
> Thanks,
> Florian
>

Re: GCC arc port defaults to -fcommon

[Richard Biener via Gcc]

On Wed, Jul 7, 2021 at 11:56 AM Richard Biener
 wrote:
>
> On Wed, Jul 7, 2021 at 11:00 AM Florian Weimer via Gcc  
> wrote:
> >
> > It seems to me that the arc port still defaults to -fcommon, presumably
> > due to this in gcc/common/config/arc/arc-common.c:
> >
> > static void
> > arc_option_init_struct (struct gcc_options *opts)
> > {
> >   opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
> >
> >   /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
> >   arc_cpu = PROCESSOR_NONE;
> > }
> >
> > Is that really necessary?  Is -fno-common broken on arc?
>
> It seems arc has -fcommon dependent on !TARGET_NO_SDATA_SET
> but it should use global_options_set.x_flag_no_common instead of
> such magic value.

So sth like this (untested):

diff --git a/gcc/common/config/arc/arc-common.c
b/gcc/common/config/arc/arc-common.c
index 6a119029616..c8ac7471744 100644
--- a/gcc/common/config/arc/arc-common.c
+++ b/gcc/common/config/arc/arc-common.c
@@ -32,8 +32,6 @@ along with GCC; see the file COPYING3.  If not see
 static void
 arc_option_init_struct (struct gcc_options *opts)
 {
-  opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
-
   /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
   arc_cpu = PROCESSOR_NONE;
 }
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
index 69f6ae464e1..b9097b11835 100644
--- a/gcc/config/arc/arc.c
+++ b/gcc/config/arc/arc.c
@@ -1440,7 +1440,7 @@ arc_override_options (void)
   if (flag_pic)
 target_flags |= MASK_NO_SDATA_SET;

-  if (flag_no_common == 255)
+  if (!global_options_set.x_flag_no_common)
 flag_no_common = !TARGET_NO_SDATA_SET;

   /* Check for small data option */


> > I plan to switch glibc to build with -fno-common unconditionally, for
> > all GCC versions and architectures, and I wonder if that would be a
> > blocker.
> >
> > Thanks,
> > Florian
> >

Re: GCC arc port defaults to -fcommon

[Florian Weimer via Gcc]

* Richard Biener:

> On Wed, Jul 7, 2021 at 11:56 AM Richard Biener
>  wrote:
>>
>> On Wed, Jul 7, 2021 at 11:00 AM Florian Weimer via Gcc  
>> wrote:
>> >
>> > It seems to me that the arc port still defaults to -fcommon, presumably
>> > due to this in gcc/common/config/arc/arc-common.c:
>> >
>> > static void
>> > arc_option_init_struct (struct gcc_options *opts)
>> > {
>> >   opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
>> >
>> >   /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
>> >   arc_cpu = PROCESSOR_NONE;
>> > }
>> >
>> > Is that really necessary?  Is -fno-common broken on arc?
>>
>> It seems arc has -fcommon dependent on !TARGET_NO_SDATA_SET
>> but it should use global_options_set.x_flag_no_common instead of
>> such magic value.
>
> So sth like this (untested):
>
> diff --git a/gcc/common/config/arc/arc-common.c
> b/gcc/common/config/arc/arc-common.c
> index 6a119029616..c8ac7471744 100644
> --- a/gcc/common/config/arc/arc-common.c
> +++ b/gcc/common/config/arc/arc-common.c
> @@ -32,8 +32,6 @@ along with GCC; see the file COPYING3.  If not see
>  static void
>  arc_option_init_struct (struct gcc_options *opts)
>  {
> -  opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
> -
>/* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
>arc_cpu = PROCESSOR_NONE;
>  }
> diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
> index 69f6ae464e1..b9097b11835 100644
> --- a/gcc/config/arc/arc.c
> +++ b/gcc/config/arc/arc.c
> @@ -1440,7 +1440,7 @@ arc_override_options (void)
>if (flag_pic)
>  target_flags |= MASK_NO_SDATA_SET;
>
> -  if (flag_no_common == 255)
> +  if (!global_options_set.x_flag_no_common)
>  flag_no_common = !TARGET_NO_SDATA_SET;
>
>/* Check for small data option */

But this means that arc still defaults to -fcommon with this change,
right?

Thanks,
Florian

Re: GCC arc port defaults to -fcommon

[Claudiu Zissulescu via Gcc]

The fno-common option is indeed related to the use of small-data. However, the 
small data is only used for baremetal applications, and it shouldn't be used 
for  linux toolchain.

To me Richard's patch looks alright.
Thanks,
Claudiu


From: Richard Biener 
Sent: Wednesday, July 7, 2021 12:58 PM
To: Florian Weimer 
Cc: GCC Development ; Joern Wolfgang Rennecke 
; Claudiu Zissulescu 
Subject: Re: GCC arc port defaults to -fcommon

On Wed, Jul 7, 2021 at 11:56 AM Richard Biener
 wrote:
>
> On Wed, Jul 7, 2021 at 11:00 AM Florian Weimer via Gcc  
> wrote:
> >
> > It seems to me that the arc port still defaults to -fcommon, presumably
> > due to this in gcc/common/config/arc/arc-common.c:
> >
> > static void
> > arc_option_init_struct (struct gcc_options *opts)
> > {
> >   opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
> >
> >   /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
> >   arc_cpu = PROCESSOR_NONE;
> > }
> >
> > Is that really necessary?  Is -fno-common broken on arc?
>
> It seems arc has -fcommon dependent on !TARGET_NO_SDATA_SET
> but it should use global_options_set.x_flag_no_common instead of
> such magic value.

So sth like this (untested):

diff --git a/gcc/common/config/arc/arc-common.c
b/gcc/common/config/arc/arc-common.c
index 6a119029616..c8ac7471744 100644
--- a/gcc/common/config/arc/arc-common.c
+++ b/gcc/common/config/arc/arc-common.c
@@ -32,8 +32,6 @@ along with GCC; see the file COPYING3.  If not see
 static void
 arc_option_init_struct (struct gcc_options *opts)
 {
-  opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
-
   /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
   arc_cpu = PROCESSOR_NONE;
 }
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
index 69f6ae464e1..b9097b11835 100644
--- a/gcc/config/arc/arc.c
+++ b/gcc/config/arc/arc.c
@@ -1440,7 +1440,7 @@ arc_override_options (void)
   if (flag_pic)
 target_flags |= MASK_NO_SDATA_SET;

-  if (flag_no_common == 255)
+  if (!global_options_set.x_flag_no_common)
 flag_no_common = !TARGET_NO_SDATA_SET;

   /* Check for small data option */


> > I plan to switch glibc to build with -fno-common unconditionally, for
> > all GCC versions and architectures, and I wonder if that would be a
> > blocker.
> >
> > Thanks,
> > Florian
> >

Re: GCC arc port defaults to -fcommon

[Claudiu Zissulescu via Gcc]

Hi,

I have quickly tested the next patch, and it looks like everything is alright. 
In the end we don't really need to temper with fcommon.


diff --git a/gcc/common/config/arc/arc-common.c 
b/gcc/common/config/arc/arc-common.c
index 6a1190296167..3b36d09997c7 100644
--- a/gcc/common/config/arc/arc-common.c
+++ b/gcc/common/config/arc/arc-common.c
@@ -30,10 +30,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "flags.h"

 static void
-arc_option_init_struct (struct gcc_options *opts)
+arc_option_init_struct (struct gcc_options *opts ATTRIBUTE_UNUSED)
 {
-  opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
-
   /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
   arc_cpu = PROCESSOR_NONE;
 }
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
index 584783f2fb88..5f1d2ffd3995 100644
--- a/gcc/config/arc/arc.c
+++ b/gcc/config/arc/arc.c
@@ -1449,9 +1449,6 @@ arc_override_options (void)
   target_flags |= MASK_NO_SDATA_SET;
 }

-  if (flag_no_common == 255)
-flag_no_common = !TARGET_NO_SDATA_SET;
-
   /* Check for small data option */
   if (!global_options_set.x_g_switch_value && !TARGET_NO_SDATA_SET)
 g_switch_value = TARGET_LL64 ? 8 : 4;




From: Florian Weimer 
Sent: Wednesday, July 7, 2021 1:06 PM
To: Richard Biener 
Cc: GCC Development ; Joern Wolfgang Rennecke 
; Claudiu Zissulescu 
Subject: Re: GCC arc port defaults to -fcommon

* Richard Biener:

> On Wed, Jul 7, 2021 at 11:56 AM Richard Biener
>  wrote:
>>
>> On Wed, Jul 7, 2021 at 11:00 AM Florian Weimer via Gcc  
>> wrote:
>> >
>> > It seems to me that the arc port still defaults to -fcommon, presumably
>> > due to this in gcc/common/config/arc/arc-common.c:
>> >
>> > static void
>> > arc_option_init_struct (struct gcc_options *opts)
>> > {
>> >   opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
>> >
>> >   /* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
>> >   arc_cpu = PROCESSOR_NONE;
>> > }
>> >
>> > Is that really necessary?  Is -fno-common broken on arc?
>>
>> It seems arc has -fcommon dependent on !TARGET_NO_SDATA_SET
>> but it should use global_options_set.x_flag_no_common instead of
>> such magic value.
>
> So sth like this (untested):
>
> diff --git a/gcc/common/config/arc/arc-common.c
> b/gcc/common/config/arc/arc-common.c
> index 6a119029616..c8ac7471744 100644
> --- a/gcc/common/config/arc/arc-common.c
> +++ b/gcc/common/config/arc/arc-common.c
> @@ -32,8 +32,6 @@ along with GCC; see the file COPYING3.  If not see
>  static void
>  arc_option_init_struct (struct gcc_options *opts)
>  {
> -  opts->x_flag_no_common = 255; /* Mark as not user-initialized.  */
> -
>/* Which cpu we're compiling for (ARC600, ARC601, ARC700, ARCv2).  */
>arc_cpu = PROCESSOR_NONE;
>  }
> diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
> index 69f6ae464e1..b9097b11835 100644
> --- a/gcc/config/arc/arc.c
> +++ b/gcc/config/arc/arc.c
> @@ -1440,7 +1440,7 @@ arc_override_options (void)
>if (flag_pic)
>  target_flags |= MASK_NO_SDATA_SET;
>
> -  if (flag_no_common == 255)
> +  if (!global_options_set.x_flag_no_common)
>  flag_no_common = !TARGET_NO_SDATA_SET;
>
>/* Check for small data option */

But this means that arc still defaults to -fcommon with this change,
right?

Thanks,
Florian

Re: daily report on extending static analyzer project [GSoC]

[Ankur Saini via Gcc]

> On 07-Jul-2021, at 4:16 AM, David Malcolm  wrote:
> 
> On Sat, 2021-07-03 at 20:07 +0530, Ankur Saini wrote:
>> AIM for today : 
>> 
>> - update the call_stack to track something else other than supergraph
>> edges
>> 
>> —
>> 
>> PROGRESS :
>> 
>> - After some brainstorming about tracking the callstack, I think one
>> better way to track the call stack is to keep track of source and
>> destination of the call instead of supperedges representing them. 
>> 
>> - so I branched again and updated the call-string to now capture a pair
>> of supernodes ( one representing callee and other representing caller
>> ), like this I was not only able to easily port the entire code to
>> adapt it without much difficulty, but now can also push calls to
>> functions that doesn’t possess a superedge.
>> 
>> - changes done can be seen on the "
>> refs/users/arsenic/heads/call_string_update “ branch. ( currently this
>> branch doesn’t contain other changes I have done till now, as I wanted
>> to test the new call-string representation exclusively and make sure it
>> doesn’t affect the functionality of the base analyser )
> 
> I'm not an expert at git, so it took me a while to figure out how to
> access your branch.
> 
> It's easier for me if you can also use "git format-patch" to generate a
> patch and "git send-email" to send it to this list (and me, please), so
> that the patch content is going to the list.
> 
> The approach in the patch seems reasonable.
> 
> I think you may have a memory leak, though: you're changing call_string
> from:
>  auto_vec m_return_edges;
> to:
>  auto_vec*> m_supernodes;
> 
> and the std:pairs are being dynamically allocated on the heap.
> Ownership gets transferred by call_string::operator=, but if I'm
> reading the patch correctly never get deleted.  This is OK for
> prototyping, but will need fixing before the code can be merged.

> 
> It's probably simplest to get rid of the indirection and allocation in
> m_supernodes and have the std::pair be stored by value, rather than by
> pointer, i.e.:
>  auto_vec > m_supernodes;
> 
> Does that work? (or is there a problem I'm not thinking of).

yes, I noticed that while creating, was thinking to empty the vector and delete 
the all the memory in the destructor of the call-string ( or make them unique 
pointers and let them destroy themselves ) but looks like storing the values of 
the pairs would make more sense.

> 
> If that's a problem, I think you might be able to get away with
> dropping the "first" from the pair, and simply storing the supernode to
> return to; I think the only places that "first" gets used are in dumps
> and in validation.  But "first" is probably helpful for debugging, so
> given that you've got it working with that field, better to keep it.

yes, I see that too, but my idea is to keep it as is for now ( maybe it might 
turn out to be helpful in future ). I will change it back if it proves to be 
useless and we get time at the end.

> 
> Hope this is helpful
> Dave
> 
>> 
>> - now hopefully all that is left for tomorrow is to update the analyzer
>> to finally see, call and return from the function aclled via the
>> function pointer. 
>> 
>> STATUS AT THE END OF THE DAY :- 
>> 
>> - update the call_stack to track something else other than supergraph
>> edges ( done )
>> 
>> Thank you
>> - Ankur

———

> On 07-Jul-2021, at 4:20 AM, David Malcolm  wrote:
> 
> On Tue, 2021-07-06 at 18:46 -0400, David Malcolm wrote:
>> On Sat, 2021-07-03 at 20:07 +0530, Ankur Saini wrote:
>>> AIM for today : 
>>> 
>>> - update the call_stack to track something else other than
>>> supergraph
>>> edges
>>> 
>>> —
>>> 
>>> PROGRESS :
>>> 
>>> - After some brainstorming about tracking the callstack, I think
>>> one
>>> better way to track the call stack is to keep track of source and
>>> destination of the call instead of supperedges representing them. 
>>> 
>>> - so I branched again and updated the call-string to now capture a
>>> pair
>>> of supernodes ( one representing callee and other representing
>>> caller
>>> ), like this I was not only able to easily port the entire code to
>>> adapt it without much difficulty, but now can also push calls to
>>> functions that doesn’t possess a superedge.
>>> 
>>> - changes done can be seen on the "
>>> refs/users/arsenic/heads/call_string_update “ branch. ( currently
>>> this
>>> branch doesn’t contain other changes I have done till now, as I
>>> wanted
>>> to test the new call-string representation exclusively and make
>>> sure it
>>> doesn’t affect the functionality of the base analyser )
>> 
>> I'm not an expert at git, so it took me a while to figure out how to
>> access your branch.
>> 
>> It's easier for me if you can also use "git format-patch" to generate
>> a
>> patch and "git send-email" to send it to this list (and me, please),
>> so
>> that the patch content is going to the list.
>> 
>> The approach in the patch seems reasonable.
>> 
>> I think you may have a memory leak, though: you're chang

Re: daily report on extending static analyzer project [GSoC]

[David Malcolm via Gcc]

On Wed, 2021-07-07 at 19:22 +0530, Ankur Saini wrote:
> 
> 
> > On 07-Jul-2021, at 4:16 AM, David Malcolm 
> > wrote:
> > 
> > On Sat, 2021-07-03 at 20:07 +0530, Ankur Saini wrote:
> > > AIM for today : 
> > > 
> > > - update the call_stack to track something else other than
> > > supergraph
> > > edges
> > > 
> > > —
> > > 
> > > PROGRESS :
> > > 
> > > - After some brainstorming about tracking the callstack, I think
> > > one
> > > better way to track the call stack is to keep track of source and
> > > destination of the call instead of supperedges representing them.
> > > 
> > > - so I branched again and updated the call-string to now capture
> > > a pair
> > > of supernodes ( one representing callee and other representing
> > > caller
> > > ), like this I was not only able to easily port the entire code
> > > to
> > > adapt it without much difficulty, but now can also push calls to
> > > functions that doesn’t possess a superedge.
> > > 
> > > - changes done can be seen on the "
> > > refs/users/arsenic/heads/call_string_update “ branch. ( currently
> > > this
> > > branch doesn’t contain other changes I have done till now, as I
> > > wanted
> > > to test the new call-string representation exclusively and make
> > > sure it
> > > doesn’t affect the functionality of the base analyser )
> > 
> > I'm not an expert at git, so it took me a while to figure out how
> > to
> > access your branch.
> > 
> > It's easier for me if you can also use "git format-patch" to
> > generate a
> > patch and "git send-email" to send it to this list (and me,
> > please), so
> > that the patch content is going to the list.
> > 
> > The approach in the patch seems reasonable.
> > 
> > I think you may have a memory leak, though: you're changing
> > call_string
> > from:
> >  auto_vec m_return_edges;
> > to:
> >  auto_vec*>
> > m_supernodes;
> > 
> > and the std:pairs are being dynamically allocated on the heap.
> > Ownership gets transferred by call_string::operator=, but if I'm
> > reading the patch correctly never get deleted.  This is OK for
> > prototyping, but will need fixing before the code can be merged.
> 
> > 
> > It's probably simplest to get rid of the indirection and allocation
> > in
> > m_supernodes and have the std::pair be stored by value, rather than
> > by
> > pointer, i.e.:
> >  auto_vec >
> > m_supernodes;
> > 
> > Does that work? (or is there a problem I'm not thinking of).
> 
> yes, I noticed that while creating, was thinking to empty the vector
> and delete the all the memory in the destructor of the call-string (
> or make them unique pointers and let them destroy themselves ) but
> looks like storing the values of the pairs would make more sense.

Yes, just storing the std::pair rather than new/delete is much simpler.

There's also an auto_delete_vec which stores (T *) as the elements
and deletes all of the elements in its dtor, but the assignment
operator/copy-ctor/move-assign/move-ctor probably don't work properly,
and the overhead of new/delete probably isn't needed.

> 
> > 
> > If that's a problem, I think you might be able to get away with
> > dropping the "first" from the pair, and simply storing the
> > supernode to
> > return to; I think the only places that "first" gets used are in
> > dumps
> > and in validation.  But "first" is probably helpful for debugging,
> > so
> > given that you've got it working with that field, better to keep
> > it.
> 
> yes, I see that too, but my idea is to keep it as is for now ( maybe
> it might turn out to be helpful in future ). I will change it back if
> it proves to be useless and we get time at the end.

Yes; my suggestion was just in case there were issues with fixing the
auto_vec.   It's better for debugging to have both of the pointers in
the element.

[...snip...]

> > > 
> > > I think you may have a memory leak, though: you're changing
> > > call_string
> > > from:
> > >   auto_vec m_return_edges;
> > > to:
> > >   auto_vec*>
> > > m_supernodes;
> > 
> > One other, unrelated idea that just occurred to me: those lines get
> > very long, so maybe introduce a typedef e.g. 
> >  typedef std::pair element_t;
> > so that you can refer to the pairs as call_string::element_t, and
> > just
> > element_t when you're in call_string scope, and just have a:
> > 
> >  auto_vec m_supernodes;
> > 
> > or
> > 
> >  auto_vec m_elements; 
> > 
> > within the call_string, if that makes sense.  Does that simplify
> > things?
> 
> Yes, this is a nice idea, I will update the call-stack with next
> update to the analyzer, or should I update it and send a patch to the
> mailing list with this call_string changes for review first and then
> work on the other changes ?

I prefer reviewing code via emails to the mailing list, rather than
looking at it in the repository.  One benefit is that other list
subscribers (and archive readers) can easily see the code we're
discussing; this will become more significant as we go into the ipa-
devirt code which wasn't written by me.

That sa

Re: where is PRnnnn required again?

[Martin Sebor via Gcc]

On 7/6/21 4:09 PM, Jonathan Wakely wrote:



On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc, > wrote:


On 7/6/21 3:36 PM, Marek Polacek wrote:
 > On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via Gcc wrote:
 >> I came away from the recent discussion of ChangeLogs requirements
 >> with the impression that the PR bit should be in the subject
 >> (first) line and also above the ChangeLog part but doesn't need
 >> to be repeated again in the ChangeLog entries.  But my commit
 >> below was rejected last Friday with the subsequent error.  Adding
 >> PR middle-end/98871 to the ChangeLog entry let me push the change:
 >>
 >> https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
 >>
 >> I just had the same error happen now, again with what seems like
 >> a valid commit message.  Did I misunderstand something or has
 >> something changed recently?
 >>
 >> Martin
 >>
 >> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD -> master)
 >> Author: Martin Sebor mailto:mse...@redhat.com>>
 >> Date:   Fri Jul 2 16:16:31 2021 -0600
 >>
 >>      Improve warning suppression for inlined functions [PR98512].
 >>
 >>      Resolves:
 >>      PR middle-end/98871 - Cannot silence -Wmaybe-uninitialized at
 >> declaration si
 >> te
 >>      PR middle-end/98512 - #pragma GCC diagnostic ignored
ineffective in
 >> conjunct
 >> ion with alias attribute
 >
 > This should be just
 >
 >       PR middle-end/98871
 >       PR middle-end/98512
 >
 > , no?

Does it matter if there's text after the PR ...?



Yes. With extra text the whole line is just treated as arbitrary text, 
not a "PR component/" string. So with the extra text it won't be 
added to the ChangeLog file, and won't match the PR in the subject line.


   I managed to push

https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html

that uses the same style earlier today


But will it add the PR numbers to the ChangeLog? I think the answer is 
no (in which case you could edit the ChangeLog tomorrow if you want them 
to be in there).


It updated Bugzilla but it didn't add the PR numbers to the ChangeLog
entries.  I still don't (obviously) understand the rules the hook uses
for what to update or the rationale for them.  It seems as though
the PR in the subject is used to update only Bugzilla but not also
update the ChangeLogs (why not?)  The PR component/ part that's
supposed to come before the ChangeLog is used to update ChangeLog
entries but seems to be ignored if it's followed by any text (why?)
These unnecessary gotchas aren't documented anywhere.

Martin

Re: where is PRnnnn required again?

[Jakub Jelinek via Gcc]

On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via Gcc wrote:
> I came away from the recent discussion of ChangeLogs requirements
> with the impression that the PR bit should be in the subject
> (first) line and also above the ChangeLog part but doesn't need
> to be repeated again in the ChangeLog entries.  But my commit
> below was rejected last Friday with the subsequent error.  Adding
> PR middle-end/98871 to the ChangeLog entry let me push the change:
> 
> https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
> 
> I just had the same error happen now, again with what seems like
> a valid commit message.  Did I misunderstand something or has
> something changed recently?
> 
> Martin
> 
> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD -> master)
> Author: Martin Sebor 
> Date:   Fri Jul 2 16:16:31 2021 -0600
> 
> Improve warning suppression for inlined functions [PR98512].

This states a PR number on the first line
> 
> Resolves:
> PR middle-end/98871 - Cannot silence -Wmaybe-uninitialized at
> declaration si
> te
> PR middle-end/98512 - #pragma GCC diagnostic ignored ineffective in
> conjunct
> ion with alias attribute

but these are intentionally not considered part of the ChangeLog entry,
so it isn't specified anywhere and that is why it has been rejected.

The ChangeLog entry must be in a rigid form so that random text in the middle
of the commit message isn't mistaken for start of the ChangeLog entry.

contrib/gcc-changelog/git_commit.py
has
author_line_regex = \
re.compile(r'^(?P\d{4}-\d{2}-\d{2})\ {2}(?P.*  <.*>)')
changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
pr_regex = re.compile(r'\tPR (?P[a-z+-]+\/)?([0-9]+)$')
dr_regex = re.compile(r'\tDR ([0-9]+)$')
star_prefix_regex = re.compile(r'\t\*(?P\ *)(?P.*)')
and will consider as first line of the ChangeLog part:
if (changelog_regex.match(b) or self.find_changelog_location(b)
or star_prefix_regex.match(b) or pr_regex.match(b)
or dr_regex.match(b) or author_line_regex.match(b)
or b.lower().startswith(CO_AUTHORED_BY_PREFIX)):
self.changes = body[i:]
return
Once something is considered a ChangeLog part, everything after it has
to be a valid ChangeLog entry format.
Matching just PR component/N with random text afterwards at the start
of line or even somewhere in the middle would trigger any time one talks about 
some
PR in the description.

> 
> gcc/ChangeLog:
> 
> * diagnostic.c (get_any_inlining_info): New.
> (update_effective_level_from_pragmas): Handle inlining context.
> (diagnostic_enabled): Same.
> (diagnostic_report_diagnostic): Same.
> * diagnostic.h (struct diagnostic_info): Add ctor.
> (struct diagnostic_context): Add new member.
> * tree-diagnostic.c (set_inlining_locations): New.
> (tree_diagnostics_defaults): Set new callback pointer.

Jakub

Re: where is PRnnnn required again?

[Martin Sebor via Gcc]

On 7/7/21 11:51 AM, Jakub Jelinek wrote:

On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via Gcc wrote:

I came away from the recent discussion of ChangeLogs requirements
with the impression that the PR bit should be in the subject
(first) line and also above the ChangeLog part but doesn't need
to be repeated again in the ChangeLog entries.  But my commit
below was rejected last Friday with the subsequent error.  Adding
PR middle-end/98871 to the ChangeLog entry let me push the change:

https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381

I just had the same error happen now, again with what seems like
a valid commit message.  Did I misunderstand something or has
something changed recently?

Martin

commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD -> master)
Author: Martin Sebor 
Date:   Fri Jul 2 16:16:31 2021 -0600

 Improve warning suppression for inlined functions [PR98512].


This states a PR number on the first line


 Resolves:
 PR middle-end/98871 - Cannot silence -Wmaybe-uninitialized at
declaration si
te
 PR middle-end/98512 - #pragma GCC diagnostic ignored ineffective in
conjunct
ion with alias attribute


but these are intentionally not considered part of the ChangeLog entry,
so it isn't specified anywhere and that is why it has been rejected.


But the PRs are used to update Bugzilla, right?  E.g., this commit:

  https://gcc.gnu.org/g:6d3bab5d5adb3e28ddb16c97b0831efdea23cf7d

updated both of:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98512#c12
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98871#c7

(but not ChangeLogs).  So why can't they also be used to update
the ChangeLog entries?



The ChangeLog entry must be in a rigid form so that random text in the middle
of the commit message isn't mistaken for start of the ChangeLog entry.


If it's okay for the PR lines to be used to update Bugzilla why is
it not also okay to use them to update ChangeLogs?  Or to put it
differently: can we change the script to do both?  I can't be
the only one who finds this onerous and confusing.

Martin



contrib/gcc-changelog/git_commit.py
has
author_line_regex = \
 re.compile(r'^(?P\d{4}-\d{2}-\d{2})\ {2}(?P.*  <.*>)')
changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
pr_regex = re.compile(r'\tPR (?P[a-z+-]+\/)?([0-9]+)$')
dr_regex = re.compile(r'\tDR ([0-9]+)$')
star_prefix_regex = re.compile(r'\t\*(?P\ *)(?P.*)')
and will consider as first line of the ChangeLog part:
 if (changelog_regex.match(b) or self.find_changelog_location(b)
 or star_prefix_regex.match(b) or pr_regex.match(b)
 or dr_regex.match(b) or author_line_regex.match(b)
 or b.lower().startswith(CO_AUTHORED_BY_PREFIX)):
 self.changes = body[i:]
 return
Once something is considered a ChangeLog part, everything after it has
to be a valid ChangeLog entry format.
Matching just PR component/N with random text afterwards at the start
of line or even somewhere in the middle would trigger any time one talks about 
some
PR in the description.



 gcc/ChangeLog:

 * diagnostic.c (get_any_inlining_info): New.
 (update_effective_level_from_pragmas): Handle inlining context.
 (diagnostic_enabled): Same.
 (diagnostic_report_diagnostic): Same.
 * diagnostic.h (struct diagnostic_info): Add ctor.
 (struct diagnostic_context): Add new member.
 * tree-diagnostic.c (set_inlining_locations): New.
 (tree_diagnostics_defaults): Set new callback pointer.


Jakub

Re: where is PRnnnn required again?

[Jonathan Wakely via Gcc]

On Wed, 7 Jul 2021, 17:39 Martin Sebor,  wrote:

> On 7/6/21 4:09 PM, Jonathan Wakely wrote:
> >
> >
> > On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc,  > > wrote:
> >
> > On 7/6/21 3:36 PM, Marek Polacek wrote:
> >  > On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via Gcc
> wrote:
> >  >> I came away from the recent discussion of ChangeLogs requirements
> >  >> with the impression that the PR bit should be in the subject
> >  >> (first) line and also above the ChangeLog part but doesn't need
> >  >> to be repeated again in the ChangeLog entries.  But my commit
> >  >> below was rejected last Friday with the subsequent error.  Adding
> >  >> PR middle-end/98871 to the ChangeLog entry let me push the
> change:
> >  >>
> >  >> https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
> >  >>
> >  >> I just had the same error happen now, again with what seems like
> >  >> a valid commit message.  Did I misunderstand something or has
> >  >> something changed recently?
> >  >>
> >  >> Martin
> >  >>
> >  >> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD -> master)
> >  >> Author: Martin Sebor  mse...@redhat.com>>
> >  >> Date:   Fri Jul 2 16:16:31 2021 -0600
> >  >>
> >  >>  Improve warning suppression for inlined functions [PR98512].
> >  >>
> >  >>  Resolves:
> >  >>  PR middle-end/98871 - Cannot silence -Wmaybe-uninitialized
> at
> >  >> declaration si
> >  >> te
> >  >>  PR middle-end/98512 - #pragma GCC diagnostic ignored
> > ineffective in
> >  >> conjunct
> >  >> ion with alias attribute
> >  >
> >  > This should be just
> >  >
> >  >   PR middle-end/98871
> >  >   PR middle-end/98512
> >  >
> >  > , no?
> >
> > Does it matter if there's text after the PR ...?
> >
> >
> >
> > Yes. With extra text the whole line is just treated as arbitrary text,
> > not a "PR component/" string. So with the extra text it won't be
> > added to the ChangeLog file, and won't match the PR in the subject line.
> >
> >I managed to push
> >
> > https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html
> >
> > that uses the same style earlier today
> >
> >
> > But will it add the PR numbers to the ChangeLog? I think the answer is
> > no (in which case you could edit the ChangeLog tomorrow if you want them
> > to be in there).
>
> It updated Bugzilla but it didn't add the PR numbers to the ChangeLog
> entries.  I still don't (obviously) understand the rules the hook uses
> for what to update or the rationale for them.  It seems as though
> the PR in the subject is used to update only Bugzilla but not also
> update the ChangeLogs (why not?)


Because they are two completely separate processes. Verifying the commit
message format is done by a git hook, and you can run exactly the same
checks locally before pushing a commit.

Updating bugzilla is done by a separate and different process, which has
been in place for years (decades?) before we switched to git.


The PR component/ part that's
> supposed to come before the ChangeLog is used to update ChangeLog
> entries but seems to be ignored if it's followed by any text (why?)
>

See Jakub's reply.


>

Re: where is PRnnnn required again?

[Jason Merrill via Gcc]

On Wed, Jul 7, 2021 at 1:54 PM Jakub Jelinek via Gcc 
wrote:

> On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via Gcc wrote:
> > I came away from the recent discussion of ChangeLogs requirements
> > with the impression that the PR bit should be in the subject
> > (first) line and also above the ChangeLog part but doesn't need
> > to be repeated again in the ChangeLog entries.  But my commit
> > below was rejected last Friday with the subsequent error.  Adding
> > PR middle-end/98871 to the ChangeLog entry let me push the change:
> >
> > https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
> >
> > I just had the same error happen now, again with what seems like
> > a valid commit message.  Did I misunderstand something or has
> > something changed recently?
> >
> > Martin
> >
> > commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD -> master)
> > Author: Martin Sebor 
> > Date:   Fri Jul 2 16:16:31 2021 -0600
> >
> > Improve warning suppression for inlined functions [PR98512].
>
> This states a PR number on the first line
> >
> > Resolves:
> > PR middle-end/98871 - Cannot silence -Wmaybe-uninitialized at
> > declaration si
> > te
> > PR middle-end/98512 - #pragma GCC diagnostic ignored ineffective in
> > conjunct
> > ion with alias attribute
>
> but these are intentionally not considered part of the ChangeLog entry,
> so it isn't specified anywhere and that is why it has been rejected.
>
> The ChangeLog entry must be in a rigid form so that random text in the
> middle
> of the commit message isn't mistaken for start of the ChangeLog entry.
>
> contrib/gcc-changelog/git_commit.py
> has
> author_line_regex = \
> re.compile(r'^(?P\d{4}-\d{2}-\d{2})\ {2}(?P.*
> <.*>)')
> changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
> pr_regex = re.compile(r'\tPR (?P[a-z+-]+\/)?([0-9]+)$')
> dr_regex = re.compile(r'\tDR ([0-9]+)$')
> star_prefix_regex = re.compile(r'\t\*(?P\ *)(?P.*)')
> and will consider as first line of the ChangeLog part:
> if (changelog_regex.match(b) or self.find_changelog_location(b)
> or star_prefix_regex.match(b) or pr_regex.match(b)
> or dr_regex.match(b) or author_line_regex.match(b)
> or b.lower().startswith(CO_AUTHORED_BY_PREFIX)):
> self.changes = body[i:]
> return
> Once something is considered a ChangeLog part, everything after it has
> to be a valid ChangeLog entry format.
> Matching just PR component/N with random text afterwards at the start
> of line or even somewhere in the middle would trigger any time one talks
> about some
> PR in the description.
>

But this *could* accept ^PR comp/N - descr$ , it just doesn't currently.

There's certainly a lot of precedent for including the description in the
PR line in ChangeLog files.

I agree we don't want to match a PR number in the middle of the line, or
without the dash.

Jason

Re: where is PRnnnn required again?

[Martin Sebor via Gcc]

On 7/7/21 2:42 PM, Jonathan Wakely wrote:



On Wed, 7 Jul 2021, 17:39 Martin Sebor, > wrote:


On 7/6/21 4:09 PM, Jonathan Wakely wrote:
 >
 >
 > On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc, mailto:gcc@gcc.gnu.org>
 > >> wrote:
 >
 >     On 7/6/21 3:36 PM, Marek Polacek wrote:
 >      > On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via
Gcc wrote:
 >      >> I came away from the recent discussion of ChangeLogs
requirements
 >      >> with the impression that the PR bit should be in the
subject
 >      >> (first) line and also above the ChangeLog part but
doesn't need
 >      >> to be repeated again in the ChangeLog entries.  But my commit
 >      >> below was rejected last Friday with the subsequent
error.  Adding
 >      >> PR middle-end/98871 to the ChangeLog entry let me push
the change:
 >      >>
 >      >>
https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
 >      >>
 >      >> I just had the same error happen now, again with what
seems like
 >      >> a valid commit message.  Did I misunderstand something or has
 >      >> something changed recently?
 >      >>
 >      >> Martin
 >      >>
 >      >> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD ->
master)
 >      >> Author: Martin Sebor mailto:mse...@redhat.com> >>
 >      >> Date:   Fri Jul 2 16:16:31 2021 -0600
 >      >>
 >      >>      Improve warning suppression for inlined functions
[PR98512].
 >      >>
 >      >>      Resolves:
 >      >>      PR middle-end/98871 - Cannot silence
-Wmaybe-uninitialized at
 >      >> declaration si
 >      >> te
 >      >>      PR middle-end/98512 - #pragma GCC diagnostic ignored
 >     ineffective in
 >      >> conjunct
 >      >> ion with alias attribute
 >      >
 >      > This should be just
 >      >
 >      >       PR middle-end/98871
 >      >       PR middle-end/98512
 >      >
 >      > , no?
 >
 >     Does it matter if there's text after the PR ...?
 >
 >
 >
 > Yes. With extra text the whole line is just treated as arbitrary
text,
 > not a "PR component/" string. So with the extra text it won't be
 > added to the ChangeLog file, and won't match the PR in the
subject line.
 >
 >        I managed to push
 >
 > https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html
 >
 >     that uses the same style earlier today
 >
 >
 > But will it add the PR numbers to the ChangeLog? I think the
answer is
 > no (in which case you could edit the ChangeLog tomorrow if you
want them
 > to be in there).

It updated Bugzilla but it didn't add the PR numbers to the ChangeLog
entries.  I still don't (obviously) understand the rules the hook uses
for what to update or the rationale for them.  It seems as though
the PR in the subject is used to update only Bugzilla but not also
update the ChangeLogs (why not?) 



Because they are two completely separate processes. Verifying the commit 
message format is done by a git hook, and you can run exactly the same 
checks locally before pushing a commit.


Updating bugzilla is done by a separate and different process, which has 
been in place for years (decades?) before we switched to git.


I don't mean to turn this into a contentious back and forth but
"because this is how it works" or "because this is how it's been
done for eons" aren't a rationale, at least not a satisfying one.

Do you not agree that it would be better to be able to mention
the PR or PRs just once and have all our scripts work with it?
If you do then is something keeping us from making those changes?

Martin

PS To be clear, I'm suggesting that all these work the same and
update Bugzilla as well as ChangeLogs, both with and without
a space after PR and both with and without a component name after
the PR.

1) PR only in title.
  Fix foobar [PR12345]

  gcc/ChangeLog:
* foo.c (bar): Fix it.

2) PR (with or without additional text after it) after title and
   before ChageLogs.
  Fix foobar.

  PR12345 - foobar broken

  gcc/ChangeLog:
* foo.c (bar): Fix it.

3) PR only in ChangeLogs.
  Fix foobar.

  gcc/ChangeLog:
PR 12345
* foo.c (bar): Fix it.




The PR component/ part that's
supposed to come before the ChangeLog is used to update ChangeLog
entries but seems to be ignored if it's followed by any text (why?)


See Jakub's reply.

Re: where is PRnnnn required again?

[Marek Polacek via Gcc]

On Wed, Jul 07, 2021 at 03:35:35PM -0600, Martin Sebor wrote:
> On 7/7/21 2:42 PM, Jonathan Wakely wrote:
> > 
> > 
> > On Wed, 7 Jul 2021, 17:39 Martin Sebor,  > > wrote:
> > 
> > On 7/6/21 4:09 PM, Jonathan Wakely wrote:
> >  >
> >  >
> >  > On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc,  > 
> >  > >> wrote:
> >  >
> >  >     On 7/6/21 3:36 PM, Marek Polacek wrote:
> >  >      > On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via
> > Gcc wrote:
> >  >      >> I came away from the recent discussion of ChangeLogs
> > requirements
> >  >      >> with the impression that the PR bit should be in the
> > subject
> >  >      >> (first) line and also above the ChangeLog part but
> > doesn't need
> >  >      >> to be repeated again in the ChangeLog entries.  But my commit
> >  >      >> below was rejected last Friday with the subsequent
> > error.  Adding
> >  >      >> PR middle-end/98871 to the ChangeLog entry let me push
> > the change:
> >  >      >>
> >  >      >>
> > https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
> >  >      >>
> >  >      >> I just had the same error happen now, again with what
> > seems like
> >  >      >> a valid commit message.  Did I misunderstand something or has
> >  >      >> something changed recently?
> >  >      >>
> >  >      >> Martin
> >  >      >>
> >  >      >> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD ->
> > master)
> >  >      >> Author: Martin Sebor  >   > >>
> >  >      >> Date:   Fri Jul 2 16:16:31 2021 -0600
> >  >      >>
> >  >      >>      Improve warning suppression for inlined functions
> > [PR98512].
> >  >      >>
> >  >      >>      Resolves:
> >  >      >>      PR middle-end/98871 - Cannot silence
> > -Wmaybe-uninitialized at
> >  >      >> declaration si
> >  >      >> te
> >  >      >>      PR middle-end/98512 - #pragma GCC diagnostic ignored
> >  >     ineffective in
> >  >      >> conjunct
> >  >      >> ion with alias attribute
> >  >      >
> >  >      > This should be just
> >  >      >
> >  >      >       PR middle-end/98871
> >  >      >       PR middle-end/98512
> >  >      >
> >  >      > , no?
> >  >
> >  >     Does it matter if there's text after the PR ...?
> >  >
> >  >
> >  >
> >  > Yes. With extra text the whole line is just treated as arbitrary
> > text,
> >  > not a "PR component/" string. So with the extra text it won't be
> >  > added to the ChangeLog file, and won't match the PR in the
> > subject line.
> >  >
> >  >        I managed to push
> >  >
> >  > https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html
> >  >
> >  >     that uses the same style earlier today
> >  >
> >  >
> >  > But will it add the PR numbers to the ChangeLog? I think the
> > answer is
> >  > no (in which case you could edit the ChangeLog tomorrow if you
> > want them
> >  > to be in there).
> > 
> > It updated Bugzilla but it didn't add the PR numbers to the ChangeLog
> > entries.  I still don't (obviously) understand the rules the hook uses
> > for what to update or the rationale for them.  It seems as though
> > the PR in the subject is used to update only Bugzilla but not also
> > update the ChangeLogs (why not?)
> > 
> > 
> > Because they are two completely separate processes. Verifying the commit
> > message format is done by a git hook, and you can run exactly the same
> > checks locally before pushing a commit.
> > 
> > Updating bugzilla is done by a separate and different process, which has
> > been in place for years (decades?) before we switched to git.
> 
> I don't mean to turn this into a contentious back and forth but
> "because this is how it works" or "because this is how it's been
> done for eons" aren't a rationale, at least not a satisfying one.
> 
> Do you not agree that it would be better to be able to mention
> the PR or PRs just once and have all our scripts work with it?
> If you do then is something keeping us from making those changes?
> 
> Martin
> 
> PS To be clear, I'm suggesting that all these work the same and
> update Bugzilla as well as ChangeLogs, both with and without
> a space after PR and both with and without a component name after
> the PR.
> 
> 1) PR only in title.
>   Fix foobar [PR12345]
> 
>   gcc/ChangeLog:
> * foo.c (bar): Fix it.

The script would have to derive the component name from the PR number. 
That might a complication.

> 2) PR (with or without additional text after it) after title and
>before ChageLogs.
>   Fix foobar.
> 
>   PR12345 - foobar b

Re: where is PRnnnn required again?

[Jonathan Wakely via Gcc]

On Wed, 7 Jul 2021, 22:35 Martin Sebor,  wrote:

> On 7/7/21 2:42 PM, Jonathan Wakely wrote:
> >
> >
> > On Wed, 7 Jul 2021, 17:39 Martin Sebor,  > > wrote:
> >
> > On 7/6/21 4:09 PM, Jonathan Wakely wrote:
> >  >
> >  >
> >  > On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc,  > 
> >  > >> wrote:
> >  >
> >  > On 7/6/21 3:36 PM, Marek Polacek wrote:
> >  >  > On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via
> > Gcc wrote:
> >  >  >> I came away from the recent discussion of ChangeLogs
> > requirements
> >  >  >> with the impression that the PR bit should be in the
> > subject
> >  >  >> (first) line and also above the ChangeLog part but
> > doesn't need
> >  >  >> to be repeated again in the ChangeLog entries.  But my
> commit
> >  >  >> below was rejected last Friday with the subsequent
> > error.  Adding
> >  >  >> PR middle-end/98871 to the ChangeLog entry let me push
> > the change:
> >  >  >>
> >  >  >>
> > https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
> >  >  >>
> >  >  >> I just had the same error happen now, again with what
> > seems like
> >  >  >> a valid commit message.  Did I misunderstand something or
> has
> >  >  >> something changed recently?
> >  >  >>
> >  >  >> Martin
> >  >  >>
> >  >  >> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD ->
> > master)
> >  >  >> Author: Martin Sebor  >   > >>
> >  >  >> Date:   Fri Jul 2 16:16:31 2021 -0600
> >  >  >>
> >  >  >>  Improve warning suppression for inlined functions
> > [PR98512].
> >  >  >>
> >  >  >>  Resolves:
> >  >  >>  PR middle-end/98871 - Cannot silence
> > -Wmaybe-uninitialized at
> >  >  >> declaration si
> >  >  >> te
> >  >  >>  PR middle-end/98512 - #pragma GCC diagnostic ignored
> >  > ineffective in
> >  >  >> conjunct
> >  >  >> ion with alias attribute
> >  >  >
> >  >  > This should be just
> >  >  >
> >  >  >   PR middle-end/98871
> >  >  >   PR middle-end/98512
> >  >  >
> >  >  > , no?
> >  >
> >  > Does it matter if there's text after the PR ...?
> >  >
> >  >
> >  >
> >  > Yes. With extra text the whole line is just treated as arbitrary
> > text,
> >  > not a "PR component/" string. So with the extra text it won't
> be
> >  > added to the ChangeLog file, and won't match the PR in the
> > subject line.
> >  >
> >  >I managed to push
> >  >
> >  > https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html
> >  >
> >  > that uses the same style earlier today
> >  >
> >  >
> >  > But will it add the PR numbers to the ChangeLog? I think the
> > answer is
> >  > no (in which case you could edit the ChangeLog tomorrow if you
> > want them
> >  > to be in there).
> >
> > It updated Bugzilla but it didn't add the PR numbers to the ChangeLog
> > entries.  I still don't (obviously) understand the rules the hook
> uses
> > for what to update or the rationale for them.  It seems as though
> > the PR in the subject is used to update only Bugzilla but not also
> > update the ChangeLogs (why not?)
> >
> >
> > Because they are two completely separate processes. Verifying the commit
> > message format is done by a git hook, and you can run exactly the same
> > checks locally before pushing a commit.
> >
> > Updating bugzilla is done by a separate and different process, which has
> > been in place for years (decades?) before we switched to git.
>
> I don't mean to turn this into a contentious back and forth but
> "because this is how it works" or "because this is how it's been
> done for eons" aren't a rationale, at least not a satisfying one.
>

You didn't ask for rationale, you asked about the rules used and why you
got that result. The reason is that "find the PR for the ChangeLog" and
"find the PR for bugzilla" are done separately by different rules.

And that's for good reasons. We want to update bugzilla for any mention of
a PR in the commit message, but we only want to list a PR in the ChangeLog
if it's actually what the commit directly addresses.

So we need to be able to distinguish  "mentions it in some way, add a
comment to bugzilla" from "really addresses this PR, use it in the
ChangeLog".

For example, in a commit log I might say "as discussed in the comments of
PR 5678, this does blah blah" and that deserves to add a comment to that
PR, but not to use that PR in the ChangeLog file. So 

Re: where is PRnnnn required again?

[Martin Sebor via Gcc]

On 7/7/21 3:53 PM, Marek Polacek wrote:

On Wed, Jul 07, 2021 at 03:35:35PM -0600, Martin Sebor wrote:

On 7/7/21 2:42 PM, Jonathan Wakely wrote:



On Wed, 7 Jul 2021, 17:39 Martin Sebor, mailto:mse...@gmail.com>> wrote:

 On 7/6/21 4:09 PM, Jonathan Wakely wrote:
  >
  >
  > On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc, mailto:gcc@gcc.gnu.org>
  > >> wrote:
  >
  >     On 7/6/21 3:36 PM, Marek Polacek wrote:
  >      > On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin Sebor via
 Gcc wrote:
  >      >> I came away from the recent discussion of ChangeLogs
 requirements
  >      >> with the impression that the PR bit should be in the
 subject
  >      >> (first) line and also above the ChangeLog part but
 doesn't need
  >      >> to be repeated again in the ChangeLog entries.  But my commit
  >      >> below was rejected last Friday with the subsequent
 error.  Adding
  >      >> PR middle-end/98871 to the ChangeLog entry let me push
 the change:
  >      >>
  >      >>
 https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
  >      >>
  >      >> I just had the same error happen now, again with what
 seems like
  >      >> a valid commit message.  Did I misunderstand something or has
  >      >> something changed recently?
  >      >>
  >      >> Martin
  >      >>
  >      >> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780 (HEAD ->
 master)
  >      >> Author: Martin Sebor mailto:mse...@redhat.com> >>
  >      >> Date:   Fri Jul 2 16:16:31 2021 -0600
  >      >>
  >      >>      Improve warning suppression for inlined functions
 [PR98512].
  >      >>
  >      >>      Resolves:
  >      >>      PR middle-end/98871 - Cannot silence
 -Wmaybe-uninitialized at
  >      >> declaration si
  >      >> te
  >      >>      PR middle-end/98512 - #pragma GCC diagnostic ignored
  >     ineffective in
  >      >> conjunct
  >      >> ion with alias attribute
  >      >
  >      > This should be just
  >      >
  >      >       PR middle-end/98871
  >      >       PR middle-end/98512
  >      >
  >      > , no?
  >
  >     Does it matter if there's text after the PR ...?
  >
  >
  >
  > Yes. With extra text the whole line is just treated as arbitrary
 text,
  > not a "PR component/" string. So with the extra text it won't be
  > added to the ChangeLog file, and won't match the PR in the
 subject line.
  >
  >        I managed to push
  >
  > https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html
  >
  >     that uses the same style earlier today
  >
  >
  > But will it add the PR numbers to the ChangeLog? I think the
 answer is
  > no (in which case you could edit the ChangeLog tomorrow if you
 want them
  > to be in there).

 It updated Bugzilla but it didn't add the PR numbers to the ChangeLog
 entries.  I still don't (obviously) understand the rules the hook uses
 for what to update or the rationale for them.  It seems as though
 the PR in the subject is used to update only Bugzilla but not also
 update the ChangeLogs (why not?)


Because they are two completely separate processes. Verifying the commit
message format is done by a git hook, and you can run exactly the same
checks locally before pushing a commit.

Updating bugzilla is done by a separate and different process, which has
been in place for years (decades?) before we switched to git.


I don't mean to turn this into a contentious back and forth but
"because this is how it works" or "because this is how it's been
done for eons" aren't a rationale, at least not a satisfying one.

Do you not agree that it would be better to be able to mention
the PR or PRs just once and have all our scripts work with it?
If you do then is something keeping us from making those changes?

Martin

PS To be clear, I'm suggesting that all these work the same and
update Bugzilla as well as ChangeLogs, both with and without
a space after PR and both with and without a component name after
the PR.

1) PR only in title.
   Fix foobar [PR12345]

   gcc/ChangeLog:
 * foo.c (bar): Fix it.


The script would have to derive the component name from the PR number.
That might a complication.


Right, it would have to get from Bugzilla.  The mklog.py script
has an option to do that (get both the PR title and component).




2) PR (with or without additional text after it) after title and
before ChageLogs.
   Fix foobar.

   PR12345 - foobar broken

   gcc/ChangeLog:
 * foo.c (bar): Fix it.


Looks like the best variant to me (I agree that enabling "- "
after the PR number would be good).
  

3) PR only in ChangeLogs.
   Fix foob

Re: where is PRnnnn required again?

[Jonathan Wakely via Gcc]

On Wed, 7 Jul 2021, 23:18 Martin Sebor,  wrote:

> On 7/7/21 3:53 PM, Marek Polacek wrote:
> > I'm not sure why you keep hitting so many issues; git addlog takes care
> of
> > this stuff for me and I've had no trouble pushing my patches.  Is there
> > a reason you don't use it also?
>
> I probably have a completely different workflow.  Git addlog isn't
> a git command (is it some sort of a GCC extension?), and what I put
> in the subject of my emails is almost never the same thing as what
> I put in the commit message.


Why not? Why is it useful to write two different explanations of the patch?

Re: where is PRnnnn required again?

[Martin Sebor via Gcc]

On 7/7/21 4:24 PM, Jonathan Wakely wrote:



On Wed, 7 Jul 2021, 23:18 Martin Sebor, > wrote:


On 7/7/21 3:53 PM, Marek Polacek wrote:
 > I'm not sure why you keep hitting so many issues; git addlog
takes care of
 > this stuff for me and I've had no trouble pushing my patches.  Is
there
 > a reason you don't use it also?

I probably have a completely different workflow.  Git addlog isn't
a git command (is it some sort of a GCC extension?), and what I put
in the subject of my emails is almost never the same thing as what
I put in the commit message. 



Why not? Why is it useful to write two different explanations of the patch?


Sometimes, maybe.  I don't really think about it too much.  I'm not
the only one who does it.  But what bearing does what we put in
the subject of our patch submissions have on this discussion?

You may have one way of doing things and others another.  Yours may
even be better/more streamlined, I don't know.  That doesn't mean
our tooling should make things more difficult for the the rest of us.

Martin

Re: where is PRnnnn required again?

[David Malcolm via Gcc]

On Wed, 2021-07-07 at 16:58 -0600, Martin Sebor via Gcc wrote:
> On 7/7/21 4:24 PM, Jonathan Wakely wrote:
> > 
> > 
> > On Wed, 7 Jul 2021, 23:18 Martin Sebor,  > > wrote:
> > 
> >     On 7/7/21 3:53 PM, Marek Polacek wrote:
> >  > I'm not sure why you keep hitting so many issues; git addlog
> >     takes care of
> >  > this stuff for me and I've had no trouble pushing my
> > patches.  Is
> >     there
> >  > a reason you don't use it also?
> > 
> >     I probably have a completely different workflow.  Git addlog
> > isn't
> >     a git command (is it some sort of a GCC extension?), and what I
> > put
> >     in the subject of my emails is almost never the same thing as
> > what
> >     I put in the commit message. 
> > 
> > 
> > Why not? Why is it useful to write two different explanations of
> > the patch?
> 
> Sometimes, maybe.  I don't really think about it too much.  I'm not
> the only one who does it.  But what bearing does what we put in
> the subject of our patch submissions have on this discussion?

FWIW if you use a different subject line for the email as for commit
message, it makes it harder to find discussion about the patch in the
list archives.

> You may have one way of doing things and others another.  Yours may
> even be better/more streamlined, I don't know.  That doesn't mean
> our tooling should make things more difficult for the the rest of us.
> 
> Martin
> 

Re: where is PRnnnn required again?

[Martin Sebor via Gcc]

On 7/7/21 4:15 PM, Jonathan Wakely wrote:



On Wed, 7 Jul 2021, 22:35 Martin Sebor, > wrote:


On 7/7/21 2:42 PM, Jonathan Wakely wrote:
 >
 >
 > On Wed, 7 Jul 2021, 17:39 Martin Sebor, mailto:mse...@gmail.com>
 > >> wrote:
 >
 >     On 7/6/21 4:09 PM, Jonathan Wakely wrote:
 >      >
 >      >
 >      > On Tue, 6 Jul 2021, 22:45 Martin Sebor via Gcc,
mailto:gcc@gcc.gnu.org>
 >     >
 >      > 
      >
 >      >     On 7/6/21 3:36 PM, Marek Polacek wrote:
 >      >      > On Tue, Jul 06, 2021 at 03:20:26PM -0600, Martin
Sebor via
 >     Gcc wrote:
 >      >      >> I came away from the recent discussion of ChangeLogs
 >     requirements
 >      >      >> with the impression that the PR bit should be
in the
 >     subject
 >      >      >> (first) line and also above the ChangeLog part but
 >     doesn't need
 >      >      >> to be repeated again in the ChangeLog entries. 
But my commit

 >      >      >> below was rejected last Friday with the subsequent
 >     error.  Adding
 >      >      >> PR middle-end/98871 to the ChangeLog entry let me push
 >     the change:
 >      >      >>
 >      >      >>
 > https://gcc.gnu.org/g:6feb628a706e86eb3f303aff388c74bdb29e7381
 >      >      >>
 >      >      >> I just had the same error happen now, again with what
 >     seems like
 >      >      >> a valid commit message.  Did I misunderstand
something or has
 >      >      >> something changed recently?
 >      >      >>
 >      >      >> Martin
 >      >      >>
 >      >      >> commit 8a6d08bb49c2b9585c2a2adbb3121f6d9347b780
(HEAD ->
 >     master)
 >      >      >> Author: Martin Sebor mailto:mse...@redhat.com>
 >     >

 >           >      >> Date:   Fri Jul 2 16:16:31 2021 -0600
 >      >      >>
 >      >      >>      Improve warning suppression for inlined functions
 >     [PR98512].
 >      >      >>
 >      >      >>      Resolves:
 >      >      >>      PR middle-end/98871 - Cannot silence
 >     -Wmaybe-uninitialized at
 >      >      >> declaration si
 >      >      >> te
 >      >      >>      PR middle-end/98512 - #pragma GCC diagnostic
ignored
 >      >     ineffective in
 >      >      >> conjunct
 >      >      >> ion with alias attribute
 >      >      >
 >      >      > This should be just
 >      >      >
 >      >      >       PR middle-end/98871
 >      >      >       PR middle-end/98512
 >      >      >
 >      >      > , no?
 >      >
 >      >     Does it matter if there's text after the PR ...?
 >      >
 >      >
 >      >
 >      > Yes. With extra text the whole line is just treated as
arbitrary
 >     text,
 >      > not a "PR component/" string. So with the extra text
it won't be
 >      > added to the ChangeLog file, and won't match the PR in the
 >     subject line.
 >      >
 >      >        I managed to push
 >      >
 >      > https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350316.html
 >      >
 >      >     that uses the same style earlier today
 >      >
 >      >
 >      > But will it add the PR numbers to the ChangeLog? I think the
 >     answer is
 >      > no (in which case you could edit the ChangeLog tomorrow if you
 >     want them
 >      > to be in there).
 >
 >     It updated Bugzilla but it didn't add the PR numbers to the
ChangeLog
 >     entries.  I still don't (obviously) understand the rules the
hook uses
 >     for what to update or the rationale for them.  It seems as though
 >     the PR in the subject is used to update only Bugzilla but not
also
 >     update the ChangeLogs (why not?)
 >
 >
 > Because they are two completely separate processes. Verifying the
commit
 > message format is done by a git hook, and you can run exactly the
same
 > checks locally before pushing a commit.
 >
 > Updating bugzilla is done by a separate and different process,
which has
 > been in place for years (decades?) before we switched to git.

I don't mean to turn this into a contentious back and forth but
"because this is how it works" or "because this is how it's been
done for eons" aren't a rationale, at least not a satisfying one.


You didn't ask for rationale, you asked about the rules used and why yo