Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.

[Jim Meyering]

On Wed, Sep 14, 2016 at 5:49 AM, Mark Wielaard  wrote:
> On Wed, 2016-09-14 at 05:39 -0700, Ian Lance Taylor wrote:
>> On Wed, Sep 14, 2016 at 1:30 AM, Mark Wielaard  wrote:
>> > On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote:
>> >> I wonder about spelling the options as
>> >> -Wshadow={local,compatible-local} rather than with a dash, but
>> >> otherwise the patch looks fine.
>> >
>> > That is a much nicer way to write the option. But if I do that I would
>> > like to keep the old names as aliases because Google already ships a gcc
>> > that accepts -Wshadow-local and -Wshadow-compatible-local and you can
>> > find programs that already probe for those names in their configure
>> > scripts. Can I make the existing names hidden aliases by marking them
>> > Undocumented in the .opt file? Or is that too contrived/ugly?
>>
>> I don't have any opinion as to what the option names should be, but I
>> don't see the fact that Google's GCC has different option names as a
>> concern.  That GCC is only used within Google
>
> Google did release a gcc with those warning options (I believe as part
> of the NDK) and there are various projects out there (firefox seems one
> of them) that check to see if these options are available before
> enabling/disabling them (I don't know if other compilers implemented
> them). Given that there are public sources out there that already seem
> to use/test for these option names I would prefer to keep the
> compatibility.

Thanks again for working on this.

I have been using these new options (locally patched) to good effect.
While the vast majority of warning-triggering code has been
technically correct, using these has uncovered at least 4 or 5 real
bugs in code I care about.

I see that these new options are not yet on master. Is there anything
I can do to help get this patch accepted?

Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.

[Jim Meyering]

On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager  wrote:
> On 9/11/16, Manuel López-Ibáñez  wrote:
>> On 11/09/16 14:02, Mark Wielaard wrote:
>>>   -Wshadow-local which warns if a local variable shadows another local
>>>   variable or parameter,
>>>
>>>   -Wshadow-compatible-local which warns if a local variable shadows
>>>   another local variable or parameter whose type is compatible with that
>>>   of the shadowing variable.

Hi Mark,
Thank you for doing this!

>> I honestly don't see the need for the second flag. Why not make Wshadow, or 
>> at
>> least Wshadow-local, work in this way by default? Warning for shadowed
>> variables that will nevertheless trigger errors/warnings if used wrongly
>> seems not very useful anyway.
>
> It helps for readability and helps pre-emptively catch those other
> errors/warnings that come from using them wrongly before they occur in
> a more confusing manner. Plus more granularity makes it easier to
> manage the workload of warning-silencing.

The new warnings will be especially welcome in C++ code.
-Wshadow is fine for most C code, but in C++ it is very common to have
method names that shadow class data member names and/or local
variables in any member function definition. Thus, using -Wshadow in
C++ code often forces undesirable (and unscalable) renaming to avoid
such shadowing, while the only important type of shadowing is that
caught by the new options. Many of us want the benefit of the new
options (spotting bad, easily-fixed code), without having to incur the
renaming (often hurting readability/maintainability) required to
enable -Werror=shadow.

adding -Wshadow-local and -Wshadow-compatible-local ?

[Jim Meyering]

Hi Diego,

I noticed this patch that adds support for improved -Wshadow-related options:

  [google] Add two new -Wshadow warnings (issue4452058)
   https://gcc.gnu.org/ml/gcc-patches/2011-04/msg02317.html
   https://codereview.appspot.com/4452058/

Here are the proposed descriptions:

-Wshadow-local which warns if a local variable shadows another local
variable or parameter,

-Wshadow-compatible-local which warns if a local variable shadows another
local variable or parameter whose type is compatible with that of the
shadowing variable.

Yet, I see no further discussion of them, other than Jason's review feedback.
Was this change deemed unsuitable for upstream gcc?

Thanks,
Jim

Re: FYI: 1500+ typos, with suggested fixes

[Jim Meyering]

Arnaud Charlet wrote:

 Also note: the line numbers listed below work for me with yesterday's
 up-to-date trunk, but if you want to use these commands, you should rerun
 the commands above so that the line numbers reflect your actual sources.
 )

 This is just a heads up.
 I'm not volunteering to make these changes.

 FWIW, there are many false positives on the gcc/ada directory (missing
 Ada specific notions and acronyms). A few real typo fixes also of course,
 but the ratio does not look very good.

In any list of that size, at least a few are guaranteed to be false positives.
That's why I wrote this part:

   (Note that these sed commands are just suggestions that do not take
   into account any context ...

The figure of 1500+ was rounded down from the actual total of 1586 suggestions.
Eliminating the FPs matching /TJE|TEH|STPO|withing/i reduces that to 1501.

Re: FYI: 1500+ typos, with suggested fixes

[Jim Meyering]

Joseph S. Myers wrote:
 I've applied this patch to fix what seemed to be some of the more
 straightforward cases covered by gcc/ChangeLog.

Thanks.  I've run the same thing on gettext and reported the results upstream.

Re: FYI: 1500+ typos, with suggested fixes

[Jim Meyering]

Joseph S. Myers wrote:

 On Tue, 29 May 2012, Jim Meyering wrote:

 Running the following command spots over 1500 typos, and suggests fixes:

   $ git ls-files|misspellings -f -|grep -v '^ERROR:'|perl -pe \
   's/^(.*?)\[(\d+)\]: (\w+) - (.*?)$/sed -i '\''${2}s!$3!$4!'\'' $1/'  k

 This command has some bugs in how it produces fixes, fixing them would
 make the output more useful in some cases:

 sed -i '1811s!MISCELANEOUS!Miscellaneous!' gcc/config/mn10300/mn10300.md

 Should preserve the case of all-uppercase words.

I suppose that would be useful.

 sed -i '2102s!hasnt!hasn't!' gcc/config/picochip/picochip.c

 Needs to allow for the ' in the replacement string.

I can do that on the command line, but it may not be worth it.
It's getting pretty ugly:

git ls-files|misspellings -f -|grep -v '^ERROR:'|perl -p \
  -e '/^(.*?)\[(\d+)\]: (\w+) - (.*?)$/ or next;' \
  -e '($file,$N,$L,$R)=($1,$2,$3,$4); $q='\''; $R=~s/$q/$q\\$q$q/g;' \
  -e 's/.*/sed -i $q${N}s!$L!$R!$q $file/'

I.e., it would print this:

sed -i '2102s!hasnt!hasn'\''t!' gcc/config/picochip/picochip.c

Re: [PATCH] genattrtab: avoid NULL-deref on error

[Jim Meyering]

Richard Guenther wrote:
 On Sat, Apr 21, 2012 at 5:53 PM, Jim Meyering j...@meyering.net wrote:
 I see that no one responded when I posted this in February.
 Ok to commit, now?

 Ok.

Thanks.  Pushed.

[PATCH] genattrtab: avoid NULL-deref on error

[Jim Meyering]

I see that no one responded when I posted this in February.
Ok to commit, now?

2012-02-24  Jim Meyering  meyer...@redhat.com

* genattrtab.c (gen_attr): Avoid NULL-deref after diagnosing
absence of an define_enum call.


diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index 4a4c2a2..bfbe3e8 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -1,6 +1,6 @@
 /* Generate code from machine description to compute values of attributes.
Copyright (C) 1991, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000,
-   2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+   2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2012
Free Software Foundation, Inc.
Contributed by Richard Kenner (ken...@vlsi1.ultra.nyu.edu)

@@ -2993,8 +2993,9 @@ gen_attr (rtx exp, int lineno)
   if (!et || !et-md_p)
error_with_line (lineno, No define_enum called `%s' defined,
 attr-name);
-  for (ev = et-values; ev; ev = ev-next)
-   add_attr_value (attr, ev-name);
+  if (et)
+   for (ev = et-values; ev; ev = ev-next)
+ add_attr_value (attr, ev-name);
 }
   else if (*XSTR (exp, 1) == '\0')
 attr-is_numeric = 1;
--
1.7.10.228.g81f95

Re: [PATCH] genmodes: don't truncate a mode name of length = 7

[Jim Meyering]

Richard Guenther wrote:
 On Thu, Apr 19, 2012 at 10:35 PM, Jim Meyering j...@meyering.net wrote:
 Richard Guenther wrote:
 Sure, my point was that the

       if (strlen (m-name) = sizeof buf)
         {
           error (%s:%d:mode name \%s\ is too long,
                  m-file, m-line, m-name);
           continue;
         }

 check does not account for the (conditional) prepending of 'C' and the
 snprintf would silently discard the last character of the name.

 Here's a patch for that.  (passes make bootstrap)
 Incidentally it removes the name-length limitation of 7 and it hoists
 the xmalloc, which induces the single added free-before-continue.
 There was some clean-up along the way: q was easy to eliminate, and
 I pulled the two identical snprintf calls out of the if block and
 replaced them with the buf[0] assignment and memcpy.

 Ok.

Thanks for the review.  Committed.

[PATCH] genmodes: remove misleading use of strncpy

[Jim Meyering]

Found by inspection.
Seeing this strncpy use without the usual following NUL-termination,
my reflex was that it was a buffer overrun candidate, but then I
realized this is gcc, so that's not very likely.
Looking a little harder, I saw the preceding strlen = sizeof buf test,
which means there is no risk of overrun.

That preceding test means the use of strncpy is misleading, since
with that there is no risk of the name being too long for the buffer,
and hence no reason to use strncpy.
One way to make the code clearer is to use strcpy, as done below.
An alternative is to use memcpy (buf, m-name, strlen (m-name) + 1).

Ok to commit?

2012-04-19  Jim Meyering  meyer...@redhat.com

genmodes: remove misleading use of strncpy
* genmodes.c (make_complex_modes): Avoid unnecessary use of strncpy.
We verified above that the string(including trailing NUL) fits in buf,
so just use strcpy.


diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index 8b6f5bc..f786258 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -1,15 +1,15 @@
 /* Generate the machine mode enumeration and associated tables.
-   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2010
+   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2010, 2012
Free Software Foundation, Inc.

 This file is part of GCC.

 GCC is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free
 Software Foundation; either version 3, or (at your option) any later
 version.

 GCC is distributed in the hope that it will be useful, but WITHOUT ANY
 WARRANTY; without even the implied warranty of MERCHANTABILITY or
 FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 for more details.
@@ -442,27 +442,27 @@ make_complex_modes (enum mode_class cl,
   if (strlen (m-name) = sizeof buf)
{
  error (%s:%d:mode name \%s\ is too long,
 m-file, m-line, m-name);
  continue;
}

   /* Float complex modes are named SCmode, etc.
 Int complex modes are named CSImode, etc.
  This inconsistency should be eliminated.  */
   if (cl == MODE_FLOAT)
{
  char *p, *q = 0;
- strncpy (buf, m-name, sizeof buf);
+ strcpy (buf, m-name);
  p = strchr (buf, 'F');
  if (p == 0)
q = strchr (buf, 'D');
  if (p == 0  q == 0)
{
  error (%s:%d: float mode \%s\ has no 'F' or 'D',
 m-file, m-line, m-name);
  continue;
}

  if (p != 0)
*p = 'C';
  else
--
1.7.10.208.gb4267

Re: [PATCH] genmodes: remove misleading use of strncpy

[Jim Meyering]

Richard Guenther wrote:
 On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering j...@meyering.net wrote:
 Found by inspection.
 Seeing this strncpy use without the usual following NUL-termination,
 my reflex was that it was a buffer overrun candidate, but then I
 realized this is gcc, so that's not very likely.
 Looking a little harder, I saw the preceding strlen = sizeof buf test,
 which means there is no risk of overrun.

 That preceding test means the use of strncpy is misleading, since
 with that there is no risk of the name being too long for the buffer,
 and hence no reason to use strncpy.
 One way to make the code clearer is to use strcpy, as done below.
 An alternative is to use memcpy (buf, m-name, strlen (m-name) + 1).

Thanks for the quick feedback.

 Without a comment before the strcpy this isn't any more clear than

Good point.
How about with this folded in?

diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index f786258..3833017 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -452,6 +452,7 @@ make_complex_modes (enum mode_class cl,
   if (cl == MODE_FLOAT)
{
  char *p, *q = 0;
+ /* We verified above that m-name+NUL fits in buf.  */
  strcpy (buf, m-name);
  p = strchr (buf, 'F');
  if (p == 0)

 when using strncpy.  Also your issue with the terminating NUL is
 still present for the snprintf call done later (in fact the code looks like
 it can be optimized, avoiding the strcpy in the path to the snprintf).

Isn't it different with snprintf?
While strncpy does *NOT* necessarily NUL-terminate,
snprintf explicitly does, always.

IMHO, if you'd like to avoid the duplication in the strcpy/snprintf
path, that deserves to be a different change.

Re: [PATCH] genmodes: remove misleading use of strncpy

[Jim Meyering]

Richard Guenther wrote:
 On Thu, Apr 19, 2012 at 2:13 PM, Jim Meyering j...@meyering.net wrote:
 Richard Guenther wrote:
 On Thu, Apr 19, 2012 at 12:42 PM, Jim Meyering j...@meyering.net wrote:
 Found by inspection.
 Seeing this strncpy use without the usual following NUL-termination,
 my reflex was that it was a buffer overrun candidate, but then I
 realized this is gcc, so that's not very likely.
 Looking a little harder, I saw the preceding strlen = sizeof buf test,
 which means there is no risk of overrun.

 That preceding test means the use of strncpy is misleading, since
 with that there is no risk of the name being too long for the buffer,
 and hence no reason to use strncpy.
 One way to make the code clearer is to use strcpy, as done below.
 An alternative is to use memcpy (buf, m-name, strlen (m-name) + 1).

 Thanks for the quick feedback.

 Without a comment before the strcpy this isn't any more clear than

 Good point.
 How about with this folded in?

 diff --git a/gcc/genmodes.c b/gcc/genmodes.c
 index f786258..3833017 100644
 --- a/gcc/genmodes.c
 +++ b/gcc/genmodes.c
 @@ -452,6 +452,7 @@ make_complex_modes (enum mode_class cl,
       if (cl == MODE_FLOAT)
        {
          char *p, *q = 0;
 +         /* We verified above that m-name+NUL fits in buf.  */
          strcpy (buf, m-name);
          p = strchr (buf, 'F');
          if (p == 0)

 That's better.  Or even cache the strlen result and use memcpy here
 as Jakub suggested.

 when using strncpy.  Also your issue with the terminating NUL is
 still present for the snprintf call done later (in fact the code looks like
 it can be optimized, avoiding the strcpy in the path to the snprintf).

 Isn't it different with snprintf?
 While strncpy does *NOT* necessarily NUL-terminate,
 snprintf explicitly does, always.

 Sure, my point was that the

   if (strlen (m-name) = sizeof buf)
 {
   error (%s:%d:mode name \%s\ is too long,
  m-file, m-line, m-name);
   continue;
 }

 check does not account for the (conditional) prepending of 'C' and the
 snprintf would silently discard the last character of the name.

Yes, you're right.
It's good to avoid snprintf and its truncation, too.

 IMHO, if you'd like to avoid the duplication in the strcpy/snprintf
 path, that deserves to be a different change.

 Sure.

 The patch is ok with caching strlen and using memcpy.

Like this, I presume:
[alternatively, declare and compute m_len on a separate line,
 just before the comparison:
   size_t m_len = strlen (m-name);
 I'd actually prefer that, but don't know if decl-after-stmt is ok here.
 ]

2012-04-19  Jim Meyering  meyer...@redhat.com

genmodes: remove misleading use of strncpy
* genmodes.c (make_complex_modes): Avoid unnecessary use of strncpy.
We verified above that the string(including trailing NUL) fits in buf,
so just use memcpy.

diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index 8b6f5bc..484a6ac 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -1,5 +1,5 @@
 /* Generate the machine mode enumeration and associated tables.
-   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2010
+   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2010, 2012
Free Software Foundation, Inc.

 This file is part of GCC.
@@ -435,11 +435,13 @@ make_complex_modes (enum mode_class cl,

   for (m = modes[cl]; m; m = m-next)
 {
+  size_t m_len;
+
   /* Skip BImode.  FIXME: BImode probably shouldn't be MODE_INT.  */
   if (m-precision == 1)
continue;

-  if (strlen (m-name) = sizeof buf)
+  if ((m_len = strlen (m-name)) = sizeof buf)
{
  error (%s:%d:mode name \%s\ is too long,
 m-file, m-line, m-name);
@@ -452,7 +454,8 @@ make_complex_modes (enum mode_class cl,
   if (cl == MODE_FLOAT)
{
  char *p, *q = 0;
- strncpy (buf, m-name, sizeof buf);
+ /* We verified above that m-name+NUL fits in buf.  */
+ memcpy (buf, m-name, m_len + 1);
  p = strchr (buf, 'F');
  if (p == 0)
q = strchr (buf, 'D');
--
1.7.10.208.gb4267

Re: [PATCH] genmodes: remove misleading use of strncpy

[Jim Meyering]

Richard Guenther wrote:
...
 The patch is ok with caching strlen and using memcpy.

 Like this, I presume:
 [alternatively, declare and compute m_len on a separate line,
  just before the comparison:
   size_t m_len = strlen (m-name);
  I'd actually prefer that, but don't know if decl-after-stmt is ok here.
  ]

 Works for me with ...
...
 -      if (strlen (m-name) = sizeof buf)
 +      if ((m_len = strlen (m-name)) = sizeof buf)

 ... the strlen in a separate stmt like

 m_len = strlen (m-name);
 if (m_len = sizeof (buf))

Thanks.
Adjusted and committed.

[PATCH] genmodes: don't truncate a mode name of length = 7

[Jim Meyering]

Richard Guenther wrote:
 Sure, my point was that the

   if (strlen (m-name) = sizeof buf)
 {
   error (%s:%d:mode name \%s\ is too long,
  m-file, m-line, m-name);
   continue;
 }

 check does not account for the (conditional) prepending of 'C' and the
 snprintf would silently discard the last character of the name.

Here's a patch for that.  (passes make bootstrap)
Incidentally it removes the name-length limitation of 7 and it hoists
the xmalloc, which induces the single added free-before-continue.
There was some clean-up along the way: q was easy to eliminate, and
I pulled the two identical snprintf calls out of the if block and
replaced them with the buf[0] assignment and memcpy.


2012-04-19  Jim Meyering  meyer...@redhat.com

* genmodes.c (make_complex_modes): Don't truncate a mode name of
length 7 or more when prepending a C.  Suggested by Richard Guenther.


 gcc/genmodes.c |   32 +---
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index 6bbeb6f..84517b9 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -427,7 +427,6 @@ make_complex_modes (enum mode_class cl,
 {
   struct mode_data *m;
   struct mode_data *c;
-  char buf[8];
   enum mode_class cclass = complex_class (cl);

   if (cclass == MODE_RANDOM)
@@ -435,6 +434,7 @@ make_complex_modes (enum mode_class cl,

   for (m = modes[cl]; m; m = m-next)
 {
+  char *p, *buf;
   size_t m_len;

   /* Skip BImode.  FIXME: BImode probably shouldn't be MODE_INT.  */
@@ -442,40 +442,34 @@ make_complex_modes (enum mode_class cl,
continue;

   m_len = strlen (m-name);
-  if (m_len = sizeof buf)
-   {
- error (%s:%d:mode name \%s\ is too long,
-m-file, m-line, m-name);
- continue;
-   }
+  /* The leading 1 + is in case we prepend a C below.  */
+  buf = (char *) xmalloc (1 + m_len + 1);

   /* Float complex modes are named SCmode, etc.
 Int complex modes are named CSImode, etc.
  This inconsistency should be eliminated.  */
+  p = 0;
   if (cl == MODE_FLOAT)
{
- char *p, *q = 0;
- /* We verified above that m-name+NUL fits in buf.  */
  memcpy (buf, m-name, m_len + 1);
  p = strchr (buf, 'F');
- if (p == 0)
-   q = strchr (buf, 'D');
- if (p == 0  q == 0)
+ if (p == 0  strchr (buf, 'D') == 0)
{
  error (%s:%d: float mode \%s\ has no 'F' or 'D',
 m-file, m-line, m-name);
+ free (buf);
  continue;
}
-
- if (p != 0)
-   *p = 'C';
- else
-   snprintf (buf, sizeof buf, C%s, m-name);
}
+  if (p != 0)
+   *p = 'C';
   else
-   snprintf (buf, sizeof buf, C%s, m-name);
+   {
+ buf[0] = 'C';
+ memcpy (buf + 1, m-name, m_len + 1);
+   }

-  c = new_mode (cclass, xstrdup (buf), file, line);
+  c = new_mode (cclass, buf, file, line);
   c-component = m;
 }
 }
--
1.7.10.208.gb4267

[PATCH] genattrtab: avoid NULL-deref on error

[Jim Meyering]

This fixes a coverity-spotted issue.
A NULL et could be dereferenced after the diagnostic is issued.


2012-02-24  Jim Meyering  meyer...@redhat.com

genattrtab: avoid NULL-deref on error
* genattrtab.c (gen_attr): Avoid NULL-deref after diagnosing
absence of an defin_enum call.

diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index 4a4c2a2..bfbe3e8 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -1,6 +1,6 @@
 /* Generate code from machine description to compute values of attributes.
Copyright (C) 1991, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000,
-   2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010
+   2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2012
Free Software Foundation, Inc.
Contributed by Richard Kenner (ken...@vlsi1.ultra.nyu.edu)

@@ -2993,8 +2993,9 @@ gen_attr (rtx exp, int lineno)
   if (!et || !et-md_p)
error_with_line (lineno, No define_enum called `%s' defined,
 attr-name);
-  for (ev = et-values; ev; ev = ev-next)
-   add_attr_value (attr, ev-name);
+  if (et)
+   for (ev = et-values; ev; ev = ev-next)
+ add_attr_value (attr, ev-name);
 }
   else if (*XSTR (exp, 1) == '\0')
 attr-is_numeric = 1;
--
1.7.9.2.263.g9be8b7

Re: doubled words

[Jim Meyering]

Mike Stump wrote:
 On Apr 10, 2011, at 2:07 PM, Jim Meyering wrote:
 $ git ls-files .|xargs perl -0777 -n -e 'while (/\b(it)\s+\1\b/gms)'
 -e '{$n=($` =~ tr/\n/\n/ + 1); ($v=$)=~s/\n/\\n/g; print
 $ARGV:$n:$v\n}'|grep -v ChangeLog
 gcc/config/cris/cris.opt:152:it it
 gcc/gensupport.c:1066:it\n it

 These are obvious to me, so I checked in fixes for them.

Likewise for this:

  gcc/ada/gnat_ugn.texi:12636:it\nit


From 9eeae941081b3d6a0dd2c7db2b4752a3f04b1541 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Sat, 23 Apr 2011 08:47:04 +0200
Subject: [PATCH] * gnat_ugn.texi (Examples of gnatxref Usage): Fix typo: s/it
 it/it is/

---
 gcc/ada/ChangeLog |4 
 gcc/ada/gnat_ugn.texi |2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/gcc/ada/ChangeLog b/gcc/ada/ChangeLog
index 70df175..f6e705e 100644
--- a/gcc/ada/ChangeLog
+++ b/gcc/ada/ChangeLog
@@ -1,3 +1,7 @@
+2011-04-23  Jim Meyering  meyer...@redhat.com
+
+   * gnat_ugn.texi (Examples of gnatxref Usage): Fix typo: s/it it/it is/
+
 2011-04-22  Eric Botcazou  ebotca...@adacore.com

* gcc-interface/decl.c (make_packable_type): Copy DECL_PARALLEL_TYPE
diff --git a/gcc/ada/gnat_ugn.texi b/gcc/ada/gnat_ugn.texi
index d843106..463662a 100644
--- a/gcc/ada/gnat_ugn.texi
+++ b/gcc/ada/gnat_ugn.texi
@@ -12634,7 +12634,7 @@ that is the entity @code{Main} is declared in main.ads, 
line 2, column 9,
 its body is in main.adb, line 1, column 14 and is not referenced any where.

 The entity @code{Print} is declared in bar.ads, line 2, column 15 and it
-it referenced in main.adb, line 6 column 12 and line 7 column 12.
+is referenced in main.adb, line 6 column 12 and line 7 column 12.

 @item gnatxref package1.adb package2.ads
 @code{gnatxref} will generates cross-reference information for
--
1.7.5.rc3.291.g63e4e

unnecessary test before free changes committed

[Jim Meyering]

FYI, I've just pushed the following two change sets.
I verified that make check on x86_64 produced the same set of 92
failures without as with my changes.  However, when I ran
make check MALLOC_PERTURB_=0 MALLOC_CHECK_=0, I saw only 91 failures.
(normally those MALLOC_*_ variables are set to nonzero values in my environment)

This was the culprit:

FAIL: gcc.dg/matrix/transpose-3.c execution,-fprofile-use 
-fipa-matrix-reorg -fdump-ipa-matrix-reorg -O3 -fwhole-program -fno-tree-fre

From 7e50b781d25170cf5bbe5f6247607c5dca879009 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Mon, 3 Jan 2011 16:52:37 +0100
Subject: [PATCH 1/2] discourage unnecessary use of if before free

* README.Portability: Explain why if (P) free (P) is best avoided.
---
 gcc/README.Portability |   27 ---
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/gcc/README.Portability b/gcc/README.Portability
index 32a33e2..4101a2f 100644
--- a/gcc/README.Portability
+++ b/gcc/README.Portability
@@ -51,14 +51,28 @@ foo (bar, )
 needs to be coded in some other way.


-free and realloc
-
+Avoid unnecessary test before free
+--

-Some implementations crash upon attempts to free or realloc the null
-pointer.  Thus if mem might be null, you need to write
+Since SunOS 4 stopped being a reasonable portability target,
+(which happened around 2007) there has been no need to guard
+against free (NULL).  Thus, any guard like the following
+constitutes a redundant test:
+
+  if (P)
+free (P);
+
+It is better to avoid the test.[*]
+Instead, simply free P, regardless of whether it is NULL.
+
+[*] However, if your profiling exposes a test like this in a
+performance-critical loop, say where P is nearly always NULL, and
+the cost of calling free on a NULL pointer would be prohibitively
+high, consider using __builtin_expect, e.g., like this:
+
+  if (__builtin_expect (ptr != NULL, 0))
+free (ptr);

-  if (mem)
-free (mem);


 Trigraphs
@@ -194,4 +208,3 @@ o Passing incorrect types to fprintf and friends.

 o Adding a function declaration for a module declared in another file to
   a .c file instead of to a .h file.
-
-- 
1.7.5.rc2.295.g19c42


From 08544935e6fcfd6a1d1cba6d302ccede02e13681 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Fri, 15 Apr 2011 20:47:40 +0200
Subject: [PATCH 2/2] remove useless if-before-free tests

Change if (E) free (E); to free (E); everywhere except in the
libgo/, intl/, zlib/ and classpath/ directories.
Also transform equivalent variants like
if (E != NULL) free (E); and allow an extra cast on the
argument to free.  Otherwise, the tested and freed E
expressions must be identical, modulo white space.
---
 gcc/ChangeLog   |   39 +
 gcc/ada/ChangeLog   |4 ++
 gcc/ada/initialize.c|3 +-
 gcc/c-family/ChangeLog  |7 +++-
 gcc/c-family/c-format.c |6 +--
 gcc/calls.c |   15 ++
 gcc/cfgcleanup.c|3 +-
 gcc/collect2.c  |3 +-
 gcc/config/i386/i386.c  |3 +-
 gcc/config/mcore/mcore.c|3 +-
 gcc/coverage.c  |3 +-
 gcc/cp/ChangeLog|4 ++
 gcc/cp/tree.c   |3 +-
 gcc/cse.c   |6 +--
 gcc/cselib.c|3 +-
 gcc/df-core.c   |   15 ++
 gcc/fortran/ChangeLog   |7 +++
 gcc/fortran/expr.c  |3 +-
 gcc/fortran/gfortranspec.c  |5 +-
 gcc/fortran/interface.c |3 +-
 gcc/fortran/trans-openmp.c  |3 +-
 gcc/function.c  |3 +-
 gcc/gcc.c   |   15 ++
 gcc/gcov.c  |6 +--
 gcc/gensupport.c|   12 ++
 gcc/graphite-clast-to-gimple.c  |3 +-
 gcc/graphite-sese-to-poly.c |3 +-
 gcc/haifa-sched.c   |3 +-
 gcc/ipa-prop.c  |3 +-
 gcc/ipa-pure-const.c|3 +-
 gcc/ipa-reference.c |3 +-
 gcc/ira-costs.c |   12 ++
 gcc/ira.c   |   18 +++-
 gcc/java/ChangeLog  |6 ++-
 gcc/java/jcf-parse.c|3 +-
 gcc/matrix-reorg.c  |9 +---
 gcc/prefix.c|3 +-
 gcc/profile.c   |3 +-
 gcc/reload1.c   |6 +--
 gcc/sched-deps.c|3 +-
 gcc/sel-sched-ir.c  |3 +-
 gcc/sese.c  |6 +--
 gcc/tree-data-ref.c |6 +--
 gcc/tree-eh.c   |3 +-
 gcc/tree-ssa-coalesce.c |3 +-
 gcc/tree-ssa-live.c |6 +--
 gcc/tree

Re: [PATCH v3] Re: avoid useless if-before-free tests

[Jim Meyering]

Janne Blomqvist wrote:
 On Thu, Mar 24, 2011 at 18:51, Jim Meyering j...@meyering.net wrote:
 Janne Blomqvist wrote:
 On Tue, Mar 8, 2011 at 19:53, Jim Meyering j...@meyering.net wrote:
 Relative to v2, I've added libgo/ to the list of exempt directories and 
 added
 this recently discussed gfc_free patch, at the request of Tobias Burnus.
 Also, I corrected an error in fortran's ChangeLog and removed all
 whitespace changes from all ChangeLog files.

 The libgfortran changes are Ok for 4.7.

 For the gfortran frontend (gcc/fortran/*) I'd prefer if you'd

 - Replace all calls to gfc_free (x) with free (x).
 - Remove the gfc_free() function and prototype.
 - Remove the free() macro which currently prevents calling free() directly.

 Following up, I've refreshed the series but hit a minor snag
 while converting new uses of gfc_free, removing new tests-before-free
 and merging/reordering changes.

 Applying this fix first makes my problem go away:

 [snip]

 So, what's the plan here? Do you plan to get a GCC account, do you
 already have one, or what? Now that 4.7 is open for development, it's
 perhaps the right time to poke the maintainers to get this patch in.

 If you don't have a GCC account, as one of the Fortran maintainers I
 can commit the Fortran and libgfortran parts, but someone else will
 have to do the rest (were they ever approved, BTW?) as I have only
 write after approval privileges for the rest of GCC.

Janne,

I've rebased and divided/reordered these changes as you suggested.
Here are the fortran parts.  I'll post the other parts separately.
Parts 1 and 3 are manual.  Part 2 is the big mechanical change with
two manual adjustments:

  [PATCH 1/3] gfortran: remove cpp definition of free, ...
  [PATCH 2/3] convert each use of gfc_free (p) to free (p)
  [PATCH 3/3] gfortran: remove now-unused definition of gfc_free


From ca8e1ad7d098a1e91113fc7052607b37c1c2a636 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Fri, 15 Apr 2011 18:22:34 +0200
Subject: [PATCH 1/3] gfortran: remove cpp definition of free, ...

in preparation for the s/gfc_free/free/ transformation.
* gfortran.h (free): Remove macro definition that would otherwise
prevent direct use of the function.
---
 gcc/fortran/ChangeLog  |7 +++
 gcc/fortran/gfortran.h |1 -
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
index 7154e62..094541c 100644
--- a/gcc/fortran/ChangeLog
+++ b/gcc/fortran/ChangeLog
@@ -1,3 +1,10 @@
+2011-04-15  Jim Meyering  meyer...@redhat.com
+
+   gfortran: remove cpp definition of free, ...
+   in preparation for the s/gfc_free/free/ transformation.
+   * gfortran.h (free): Remove macro definition that would otherwise
+   prevent direct use of the function.
+
 2011-04-18  Tobias Burnus  bur...@net-b.de

PR fortran/18918
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index c2c9d05..49fbd1f 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -49,7 +49,6 @@ along with GCC; see the file COPYING3.  If not see
 #define MAX_SUBRECORD_LENGTH 2147483639   /* 2**31-9 */


-#define free(x) Use_gfc_free_instead_of_free()
 #define gfc_is_whitespace(c) ((c==' ') || (c=='\t'))

 /* Stringization.  */
-- 
1.7.5.rc2.295.g19c42


From 672a558437e4cd52ca95b415c6036a6740a03f4c Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Fri, 15 Apr 2011 18:28:22 +0200
Subject: [PATCH 2/3] convert each use of gfc_free (p) to free (p)

Do that by running this command:
  perl -pi -e's/\bgfc_free ?\(/free (/' \
$(git grep -El '\bgfc_free ?\(')
which also corrects the few uses that lacked a space between
the function name and the open parenthesis.
Manually undo the change to the function definition itself
and its prototype.  They'll be removed next.
---
 gcc/fortran/ChangeLog |   11 +
 gcc/fortran/array.c   |4 +-
 gcc/fortran/constructor.c |2 +-
 gcc/fortran/cpp.c |8 +++---
 gcc/fortran/data.c|2 +-
 gcc/fortran/decl.c|   22 +-
 gcc/fortran/dependency.c  |2 +-
 gcc/fortran/error.c   |4 +-
 gcc/fortran/expr.c|   24 +-
 gcc/fortran/frontend-passes.c |   18 +++---
 gcc/fortran/interface.c   |   16 ++--
 gcc/fortran/intrinsic.c   |   10 
 gcc/fortran/io.c  |   16 ++--
 gcc/fortran/match.c   |   14 +-
 gcc/fortran/misc.c|2 +-
 gcc/fortran/module.c  |   50 
 gcc/fortran/openmp.c  |2 +-
 gcc/fortran/options.c |2 +-
 gcc/fortran/parse.c   |4 +-
 gcc/fortran/primary.c |4 +-
 gcc/fortran/resolve.c |8 +++---
 gcc/fortran/scanner.c |   46 ++--
 gcc/fortran/simplify.c|   16 ++--
 gcc/fortran/st.c

Re: [PATCH] doubled words

[Jim Meyering]

Mike Stump wrote:
 On Apr 16, 2011, at 4:04 AM, Gerald Pfeifer ger...@pfeifer.com wrote:
 On Fri, 15 Apr 2011, Mike Stump wrote:
 I think these are obvious.

 Which means that you can commit them without getting explicit approval

 Well, technically, it means nothing...  It only means something if the
 maintainer agrees with me, which in general we won't know until they
 weighing.  Any person that checks in under that rule runs the risk of
 a, no it isn't.  Now, if we had an obviousness maintainer, they could
 just approve it; we'd only need a wave of a magic wand to get one.
 :-)

Hi Mike,
If you hadn't said anything, I would have committed those typo fixes
by now, based on what I perceived as your review/approval and on my
reading of this part of http://gcc.gnu.org/svnwrite.html:

Free for all

The following changes can be made by everyone with SVN write access:

Fixes for obvious typos in ChangeLog files, docs, web pages,
comments and similar stuff. Just check in the fix and copy it
to gcc-patches. We don't want to get overly anal-retentive about
checkin policies.

If that policy is no longer in effect or does not apply here,
can you clarify or point to a more up to date policy?

Re: [PATCH v3] Re: avoid useless if-before-free tests

[Jim Meyering]

Janne Blomqvist wrote:

 On Mon, Apr 18, 2011 at 18:08, Jim Meyering j...@meyering.net wrote:
 I've rebased and divided/reordered these changes as you suggested.
 Here are the fortran parts.  I'll post the other parts separately.
 Parts 1 and 3 are manual.  Part 2 is the big mechanical change with
 two manual adjustments:

  [PATCH 1/3] gfortran: remove cpp definition of free, ...
  [PATCH 2/3] convert each use of gfc_free (p) to free (p)
  [PATCH 3/3] gfortran: remove now-unused definition of gfc_free

 Thanks. The patches themselves are ok, however the changelog parts
 should conform to the layout used otherwise for changelog entries (GNU
 ChangeLog format, IIRC).  The commit logs themselves that you have now
 used are, IMHO, more informative and contain less clutter, but alas..

 Note that it's recommended and usually easiest to provide changelog
 entries separately, not as diffs from the ChangeLog file.

Ok.  I interpret that as meaning I should add a line for each
file/function affected by the large mechanical change in part 2.
I will append that to the existing ChangeLog entry.
Thanks to Ralf's vc-chlog, that will be automatic:

http://git.sv.gnu.org/cgit/vc-dwim.git/tree/README

 Then when
 you commit you insert the entry. This avoids the problem of frequent
 conflicts as the top of the changelog file changes for every commit.

If you work with the gcc repository using git, it's easy to rebase even
pesky ChangeLog diffs once you hook up Bruno Haible's git-merge-changelog:

http://git.sv.gnu.org/cgit/gnulib.git/tree/lib/git-merge-changelog.c

If you'd like me to change the other two ChangeLog entries
or any commit log message, please let me know.

 (Why we, in the age of non-sucky version control, persist in keeping
 manual changelog files is beyond me..)

I couldn't agree more ;-)
Even in projects without a VC'd ChangeLog file,
I still keep one (and build up each ChangeLog entry as usual)
and use vc-dwim to automatically cross-check any change I make.
It regularly warns me about a changed file that's listed in
ChangeLog but for which my editor has unsaved changes.

 With these changes, Ok for trunk. Thanks a lot for the contribution!

Thanks for the review.

[PATCH v4] Re: avoid useless if-before-free tests

[Jim Meyering]

Since v3, I've rebased these and moved the fortran changes to precede
these (changing gfc_free to free introduced a few more instances
that this transformation deals with).  As mentioned before, the
conditional-removing transformation was done mechanically with
two manual corrections.  One to deal with a following else,
and another to restore a conditional that should not have been
removed.

A final(?) bootstrap and make check is running now.

From d64b103b57c3785bb66a8c456ef4c0ec992f4852 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Mon, 3 Jan 2011 16:52:37 +0100
Subject: [PATCH 1/2] discourage unnecessary use of if before free

* README.Portability: Explain why if (P) free (P) is best avoided.
---
 gcc/README.Portability |   27 ---
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/gcc/README.Portability b/gcc/README.Portability
index 32a33e2..4101a2f 100644
--- a/gcc/README.Portability
+++ b/gcc/README.Portability
@@ -51,14 +51,28 @@ foo (bar, )
 needs to be coded in some other way.


-free and realloc
-
+Avoid unnecessary test before free
+--

-Some implementations crash upon attempts to free or realloc the null
-pointer.  Thus if mem might be null, you need to write
+Since SunOS 4 stopped being a reasonable portability target,
+(which happened around 2007) there has been no need to guard
+against free (NULL).  Thus, any guard like the following
+constitutes a redundant test:
+
+  if (P)
+free (P);
+
+It is better to avoid the test.[*]
+Instead, simply free P, regardless of whether it is NULL.
+
+[*] However, if your profiling exposes a test like this in a
+performance-critical loop, say where P is nearly always NULL, and
+the cost of calling free on a NULL pointer would be prohibitively
+high, consider using __builtin_expect, e.g., like this:
+
+  if (__builtin_expect (ptr != NULL, 0))
+free (ptr);

-  if (mem)
-free (mem);


 Trigraphs
@@ -194,4 +208,3 @@ o Passing incorrect types to fprintf and friends.

 o Adding a function declaration for a module declared in another file to
   a .c file instead of to a .h file.
-
-- 
1.7.5.rc2.295.g19c42


From b4e18263cfa1e7ec717792da6e75c5fa179bd6d7 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Fri, 15 Apr 2011 20:47:40 +0200
Subject: [PATCH 2/2] remove useless if-before-free tests

Change if (E) free (E); to free (E); everywhere except in the
libgo/, intl/, zlib/ and classpath/ directories.
Also transform equivalent variants like
if (E != NULL) free (E); and allow an extra cast on the
argument to free.  Otherwise, the tested and freed E
expressions must be identical, modulo white space.
---
 gcc/ChangeLog   |   39 +
 gcc/ada/ChangeLog   |4 ++
 gcc/ada/initialize.c|3 +-
 gcc/c-family/ChangeLog  |7 +++-
 gcc/c-family/c-format.c |6 +--
 gcc/calls.c |   15 ++
 gcc/cfgcleanup.c|3 +-
 gcc/collect2.c  |3 +-
 gcc/config/i386/i386.c  |3 +-
 gcc/config/mcore/mcore.c|3 +-
 gcc/coverage.c  |3 +-
 gcc/cp/ChangeLog|4 ++
 gcc/cp/tree.c   |3 +-
 gcc/cse.c   |6 +--
 gcc/cselib.c|3 +-
 gcc/df-core.c   |   15 ++
 gcc/fortran/ChangeLog   |7 +++
 gcc/fortran/expr.c  |3 +-
 gcc/fortran/gfortranspec.c  |5 +-
 gcc/fortran/interface.c |3 +-
 gcc/fortran/trans-openmp.c  |3 +-
 gcc/function.c  |3 +-
 gcc/gcc.c   |   15 ++
 gcc/gcov.c  |6 +--
 gcc/gensupport.c|   12 ++
 gcc/graphite-clast-to-gimple.c  |3 +-
 gcc/graphite-sese-to-poly.c |3 +-
 gcc/haifa-sched.c   |3 +-
 gcc/ipa-prop.c  |3 +-
 gcc/ipa-pure-const.c|3 +-
 gcc/ipa-reference.c |3 +-
 gcc/ira-costs.c |   12 ++
 gcc/ira.c   |   18 +++-
 gcc/java/ChangeLog  |6 ++-
 gcc/java/jcf-parse.c|3 +-
 gcc/matrix-reorg.c  |9 +---
 gcc/prefix.c|3 +-
 gcc/profile.c   |3 +-
 gcc/reload1.c   |6 +--
 gcc/sched-deps.c|3 +-
 gcc/sel-sched-ir.c  |3 +-
 gcc/sese.c  |6 +--
 gcc/tree-data-ref.c |6 +--
 gcc/tree-eh.c   |3 +-
 gcc/tree-ssa-coalesce.c |3 +-
 gcc/tree-ssa-live.c |6 +--
 gcc/tree-ssa-loop-ivopts.c  |6 +--
 gcc

Re: [PATCH v4] Re: avoid useless if-before-free tests

[Jim Meyering]

Tom Tromey wrote:
 Jim == Jim Meyering j...@meyering.net writes:

 Jim Since v3, I've rebased these and moved the fortran changes to precede
 Jim these (changing gfc_free to free introduced a few more instances
 Jim that this transformation deals with).  As mentioned before, the
 Jim conditional-removing transformation was done mechanically with
 Jim two manual corrections.  One to deal with a following else,
 Jim and another to restore a conditional that should not have been
 Jim removed.

 I'm not sure if this needs a review (I forgot what happened on the last
 thread) but for avoidance of doubt, the java bits are ok.

Thanks.  I wasn't sure either.
I'll presume it's ok and commit tomorrow if no one says otherwise,
since some people actually did review (and spot the invalid
transformation that I've since corrected).

Re: [PATCH v3] Re: avoid useless if-before-free tests

[Jim Meyering]

Janne Blomqvist wrote:

 On Thu, Mar 24, 2011 at 18:51, Jim Meyering j...@meyering.net wrote:
 Janne Blomqvist wrote:
 On Tue, Mar 8, 2011 at 19:53, Jim Meyering j...@meyering.net wrote:
 Relative to v2, I've added libgo/ to the list of exempt directories and 
 added
 this recently discussed gfc_free patch, at the request of Tobias Burnus.
 Also, I corrected an error in fortran's ChangeLog and removed all
 whitespace changes from all ChangeLog files.

 The libgfortran changes are Ok for 4.7.

 For the gfortran frontend (gcc/fortran/*) I'd prefer if you'd

 - Replace all calls to gfc_free (x) with free (x).
 - Remove the gfc_free() function and prototype.
 - Remove the free() macro which currently prevents calling free() directly.

 Following up, I've refreshed the series but hit a minor snag
 while converting new uses of gfc_free, removing new tests-before-free
 and merging/reordering changes.

 Applying this fix first makes my problem go away:

 [snip]

 So, what's the plan here? Do you plan to get a GCC account, do you
 already have one, or what? Now that 4.7 is open for development, it's
 perhaps the right time to poke the maintainers to get this patch in.

 If you don't have a GCC account, as one of the Fortran maintainers I
 can commit the Fortran and libgfortran parts, but someone else will
 have to do the rest (were they ever approved, BTW?) as I have only
 write after approval privileges for the rest of GCC.

Can someone add me to the gcc group?  That would help.
I already have ssh access to sourceware.org.

I've rebased the series a few times, but it's been a week or so.
More convertible uses are being added regularly.
Plus I have to reorder/split things a little to avoid a new conflict.

[PATCH] doubled words

[Jim Meyering]

Signed-off-by: Jim Meyering meyer...@redhat.com
---
While most of these are in comments, the corrections
in gcc/tree-cfg.c and gcc/config/sh/constraints.md are in strings.
The former at least is marked for translation, and hence appears
in every .po file.

 gcc/config/alpha/vms-unwind.h  |4 ++--
 gcc/config/arm/unwind-arm.h|4 ++--
 gcc/config/microblaze/microblaze.c |2 +-
 gcc/config/sh/constraints.md   |4 ++--
 gcc/cp/pt.c|2 +-
 gcc/java/jcf-parse.c   |4 ++--
 gcc/tree-cfg.c |4 ++--
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/gcc/config/alpha/vms-unwind.h b/gcc/config/alpha/vms-unwind.h
index ea2c3a3..71cb7b8 100644
--- a/gcc/config/alpha/vms-unwind.h
+++ b/gcc/config/alpha/vms-unwind.h
@@ -1,5 +1,5 @@
 /* Fallback frame unwinding for Alpha/VMS.
-   Copyright (C) 1996, 1997, 1998, 2000, 2001, 2002, 2003, 2009, 2010
+   Copyright (C) 1996, 1997, 1998, 2000, 2001, 2002, 2003, 2009, 2010, 2011
Free Software Foundation, Inc.

This file is part of GCC.
@@ -229,7 +229,7 @@ alpha_vms_fallback_frame_state (struct _Unwind_Context 
*context,

   /* If PV designates an exception dispatcher, we have to adjust the return
  address column to get at the signal occurrence point, and account for
- for what the CHF context contains.  */
+ what the CHF context contains.  */

   if (DENOTES_EXC_DISPATCHER (pv))
 {
diff --git a/gcc/config/arm/unwind-arm.h b/gcc/config/arm/unwind-arm.h
index a9ba126..1a51d8d 100644
--- a/gcc/config/arm/unwind-arm.h
+++ b/gcc/config/arm/unwind-arm.h
@@ -1,5 +1,5 @@
 /* Header file for the ARM EABI unwinder
-   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009
+   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2011
Free Software Foundation, Inc.
Contributed by Paul Brook

@@ -65,7 +65,7 @@ extern C {
 }
   _Unwind_State;

-  /* Provided only for for compatibility with existing code.  */
+  /* Provided only for compatibility with existing code.  */
   typedef int _Unwind_Action;
 #define _UA_SEARCH_PHASE   1
 #define _UA_CLEANUP_PHASE  2
diff --git a/gcc/config/microblaze/microblaze.c 
b/gcc/config/microblaze/microblaze.c
index 6ea5fa2..5796bc7 100644
--- a/gcc/config/microblaze/microblaze.c
+++ b/gcc/config/microblaze/microblaze.c
@@ -919,7 +919,7 @@ microblaze_rtx_costs (rtx x, int code, int outer_code 
ATTRIBUTE_UNUSED, int *tot
  {
/* Add 1 to make shift slightly more expensive than add.  */
*total = COSTS_N_INSNS (INTVAL (XEXP (x, 1))) + 1;
-   /* Reduce shift costs for for special circumstances.  */
+   /* Reduce shift costs for special circumstances.  */
if (optimize_size  INTVAL (XEXP (x, 1))  5)
  *total -= 2;
if (!optimize_size  INTVAL (XEXP (x, 1))  17)
diff --git a/gcc/config/sh/constraints.md b/gcc/config/sh/constraints.md
index 6b0e5d2..40d0d0b 100644
--- a/gcc/config/sh/constraints.md
+++ b/gcc/config/sh/constraints.md
@@ -1,5 +1,5 @@
 ;; Constraint definitions for Renesas / SuperH SH.
-;; Copyright (C) 2007, 2008 Free Software Foundation, Inc.
+;; Copyright (C) 2007, 2008, 2011 Free Software Foundation, Inc.
 ;;
 ;; This file is part of GCC.
 ;;
@@ -99,7 +99,7 @@
(match_test ival = -128  ival = 127)))

 (define_constraint I10
-  A signed 10-bit constant, as used in in SHmedia andi, ori.
+  A signed 10-bit constant, as used in SHmedia andi, ori.
   (and (match_code const_int)
(match_test ival = -512  ival = 511)))

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 3356e75..fc69a0c 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -13980,7 +13980,7 @@ type_unification_real (tree tparms,
   gcc_assert (ntparms  0);

   /* Reset the number of non-defaulted template arguments contained
- in in TARGS.  */
+ in TARGS.  */
   NON_DEFAULT_TEMPLATE_ARGS_COUNT (targs) = NULL_TREE;

   switch (strict)
diff --git a/gcc/java/jcf-parse.c b/gcc/java/jcf-parse.c
index feeddad..a56c1b7 100644
--- a/gcc/java/jcf-parse.c
+++ b/gcc/java/jcf-parse.c
@@ -1,6 +1,6 @@
 /* Parser for Java(TM) .class files.
Copyright (C) 1996, 1998, 1999, 2000, 2001, 2002, 2003, 2004,
-   2005, 2006, 2007, 2008, 2009, 2010 Free Software Foundation, Inc.
+   2005, 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc.

 This file is part of GCC.

@@ -368,7 +368,7 @@ set_source_filename (JCF *jcf, int index)
from the input class file into the output file.  We don't decode the
data at all, merely rewriting constant indexes whenever we come
across them: this is necessary because the constant pool in the
-   output file isn't the same as the constant pool in in the input.
+   output file isn't the same as the constant pool in the input.

The main advantage of this technique is that the resulting
annotation data is pointer-free, so it doesn't have to be relocated
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index

Re: [PATCH v3] Re: avoid useless if-before-free tests

[Jim Meyering]

Janne Blomqvist wrote:
 On Fri, Apr 15, 2011 at 10:54, Jim Meyering j...@meyering.net wrote:
 Janne Blomqvist wrote:

 On Thu, Mar 24, 2011 at 18:51, Jim Meyering j...@meyering.net wrote:
 Janne Blomqvist wrote:
 On Tue, Mar 8, 2011 at 19:53, Jim Meyering j...@meyering.net wrote:
 Relative to v2, I've added libgo/ to the list of exempt
 directories and added
 this recently discussed gfc_free patch, at the request of Tobias Burnus.
 Also, I corrected an error in fortran's ChangeLog and removed all
 whitespace changes from all ChangeLog files.

 The libgfortran changes are Ok for 4.7.

 For the gfortran frontend (gcc/fortran/*) I'd prefer if you'd

 - Replace all calls to gfc_free (x) with free (x).
 - Remove the gfc_free() function and prototype.
 - Remove the free() macro which currently prevents calling free() 
 directly.

 Following up, I've refreshed the series but hit a minor snag
 while converting new uses of gfc_free, removing new tests-before-free
 and merging/reordering changes.

 Applying this fix first makes my problem go away:

 [snip]

 So, what's the plan here? Do you plan to get a GCC account, do you
 already have one, or what? Now that 4.7 is open for development, it's
 perhaps the right time to poke the maintainers to get this patch in.

 If you don't have a GCC account, as one of the Fortran maintainers I
 can commit the Fortran and libgfortran parts, but someone else will
 have to do the rest (were they ever approved, BTW?) as I have only
 write after approval privileges for the rest of GCC.

 Can someone add me to the gcc group?  That would help.
 I already have ssh access to sourceware.org.

 According to http://gcc.gnu.org/svnwrite.html , you should send an
 email to overseers(at)gcc.gnu.org . It says that you need a sponsor
 and The steering committee or a well-established GCC maintainer
 (including reviewers) can approve for write access any person with GNU
 copyright assignment papers in place and known to submit good
 patches.. I'm not sure if I'm considered to be well-established
 enough, so could someone help Jim out here, please?

 Or, if nobody pipes up within a few days, just mention my name as your
 sponsor and we'll see if the overseers consider me well-established or
 not.. ;)

Thanks.
FYI, I have an ANY assignment on file, so no need to wait for paperwork.

Re: doubled words

[Jim Meyering]

Mike Stump wrote:
 On Apr 10, 2011, at 4:54 AM, Jim Meyering wrote:
 I've noticed/fixed a few occurrences of doubled words like the the,

 I've fixed the obviously wrong ones in gcc/.* excluding ada and go.  I
 don't hate Ada and go, since I don't pretend to keep up on rules to
 their parts of the tree.  I did do java, but only gcc/java, as I think
 we still can put in obvious work there.  One or more of the ones you
 listed were spurious, I didn't change those.

 I'm skipping everything not gcc/.*, as I don't keep up on the various
 rules.  One disadvantage of having a multiplicity of rules.  :-(
 Longer term, would be nice to remove some of the multiplicity of
 rules.

 I also re-flowed a few of these to make them prettier and fixed the
 English on a few.

Thanks for the quick work.
I realized that I'd omitted a few short, common words
and have added in, on, at and for.
Running this revised command shows a few new ones,
at least one is in a diagnostic, which means it shows up
also in each of the .po files.

git ls-files .|xargs perl -0777 -n \
  -e 'while (/\b(then?|in|on|if|at|but|f?or|and|to)\s+\1\b/gms)' \
  -e '{$n=($` =~ tr/\n/\n/ + 1); ($v=$)=~s/\n/\\n/g; print $ARGV:$n:$v\n}'

Here are a few fixes:

diff --git a/gcc/config/alpha/vms-unwind.h b/gcc/config/alpha/vms-unwind.h
index ea2c3a3..71cb7b8 100644
--- a/gcc/config/alpha/vms-unwind.h
+++ b/gcc/config/alpha/vms-unwind.h
@@ -1,5 +1,5 @@
 /* Fallback frame unwinding for Alpha/VMS.
-   Copyright (C) 1996, 1997, 1998, 2000, 2001, 2002, 2003, 2009, 2010
+   Copyright (C) 1996, 1997, 1998, 2000, 2001, 2002, 2003, 2009, 2010, 2011
Free Software Foundation, Inc.

This file is part of GCC.
@@ -229,7 +229,7 @@ alpha_vms_fallback_frame_state (struct _Unwind_Context 
*context,

   /* If PV designates an exception dispatcher, we have to adjust the return
  address column to get at the signal occurrence point, and account for
- for what the CHF context contains.  */
+ what the CHF context contains.  */

   if (DENOTES_EXC_DISPATCHER (pv))
 {
diff --git a/gcc/config/arm/unwind-arm.h b/gcc/config/arm/unwind-arm.h
index a9ba126..1a51d8d 100644
--- a/gcc/config/arm/unwind-arm.h
+++ b/gcc/config/arm/unwind-arm.h
@@ -1,5 +1,5 @@
 /* Header file for the ARM EABI unwinder
-   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009
+   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2011
Free Software Foundation, Inc.
Contributed by Paul Brook

@@ -65,7 +65,7 @@ extern C {
 }
   _Unwind_State;

-  /* Provided only for for compatibility with existing code.  */
+  /* Provided only for compatibility with existing code.  */
   typedef int _Unwind_Action;
 #define _UA_SEARCH_PHASE   1
 #define _UA_CLEANUP_PHASE  2
diff --git a/gcc/config/microblaze/microblaze.c 
b/gcc/config/microblaze/microblaze.c
index 6ea5fa2..5796bc7 100644
--- a/gcc/config/microblaze/microblaze.c
+++ b/gcc/config/microblaze/microblaze.c
@@ -919,7 +919,7 @@ microblaze_rtx_costs (rtx x, int code, int outer_code 
ATTRIBUTE_UNUSED, int *tot
  {
/* Add 1 to make shift slightly more expensive than add.  */
*total = COSTS_N_INSNS (INTVAL (XEXP (x, 1))) + 1;
-   /* Reduce shift costs for for special circumstances.  */
+   /* Reduce shift costs for special circumstances.  */
if (optimize_size  INTVAL (XEXP (x, 1))  5)
  *total -= 2;
if (!optimize_size  INTVAL (XEXP (x, 1))  17)
diff --git a/gcc/config/sh/constraints.md b/gcc/config/sh/constraints.md
index 6b0e5d2..40d0d0b 100644
--- a/gcc/config/sh/constraints.md
+++ b/gcc/config/sh/constraints.md
@@ -1,5 +1,5 @@
 ;; Constraint definitions for Renesas / SuperH SH.
-;; Copyright (C) 2007, 2008 Free Software Foundation, Inc.
+;; Copyright (C) 2007, 2008, 2011 Free Software Foundation, Inc.
 ;;
 ;; This file is part of GCC.
 ;;
@@ -99,7 +99,7 @@
(match_test ival = -128  ival = 127)))

 (define_constraint I10
-  A signed 10-bit constant, as used in in SHmedia andi, ori.
+  A signed 10-bit constant, as used in SHmedia andi, ori.
   (and (match_code const_int)
(match_test ival = -512  ival = 511)))

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 86274e9..e055d7d 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -13979,7 +13979,7 @@ type_unification_real (tree tparms,
   gcc_assert (ntparms  0);

   /* Reset the number of non-defaulted template arguments contained
- in in TARGS.  */
+ in TARGS.  */
   NON_DEFAULT_TEMPLATE_ARGS_COUNT (targs) = NULL_TREE;

   switch (strict)
diff --git a/gcc/go/gofrontend/backend.h b/gcc/go/gofrontend/backend.h
index 01f3cfa..d112d1f 100644
--- a/gcc/go/gofrontend/backend.h
+++ b/gcc/go/gofrontend/backend.h
@@ -148,7 +148,7 @@ class Backend
   
   // Create a new label.  NAME will be empty if this is a label
   // created by the frontend for a loop construct.  The location is
-  // where the the label is defined.
+  // where the label is defined.
   virtual Blabel*
   label(Bfunction

Re: doubled words

[Jim Meyering]

Mike Stump wrote:
 On Apr 10, 2011, at 4:54 AM, Jim Meyering wrote:
 I've noticed/fixed a few occurrences of doubled words like the the,
 to to

 I fixed all the non-surious matches in the gcc/testsuite.  if if in
 the fortran testsuite was the major spurious match.  Thanks.

Thanks.
Yet more: an and it.
At least the first one in the .texi file is legit.

$ git ls-files .|xargs perl -0777 -n -e 'while (/\b(it)\s+\1\b/gms)' -e 
'{$n=($` =~ tr/\n/\n/ + 1); ($v=$)=~s/\n/\\n/g; print $ARGV:$n:$v\n}'|grep 
-v ChangeLog
gcc/ada/gnat_ugn.texi:12636:it\nit
gcc/config/cris/cris.opt:152:it it
gcc/gensupport.c:1066:it\n it
gcc/testsuite/ada/acats/tests/cxg/cxg2014.a:263:it it
libiberty/regex.c:4085:it\n it
libjava/classpath/java/io/ByteArrayOutputStream.java:61:it it
libjava/classpath/java/io/CharArrayWriter.java:58:it it
libjava/classpath/org/omg/CORBA_2_3/portable/OutputStream.java:75:it it
libstdc++-v3/doc/html/ext/lwg-defects.html:9102:it it
libstdc++-v3/testsuite/data/thirty_years_among_the_dead_preproc.txt:73079:it\nit
libstdc++-v3/testsuite/data/thirty_years_among_the_dead_preproc.txt:171737:it\nit


$ git ls-files .|xargs perl -0777 -n -e 'while (/\b(an)\s+\1\b/gms)' -e 
'{$n=($` =~ tr/\n/\n/ + 1); ($v=$)=~s/\n/\\n/g; print $ARGV:$n:$v\n}'|grep 
-v ChangeLog
gcc/fortran/frontend-passes.c:213:an an
libffi/src/dlmalloc.c:1822:an an
libjava/classpath/gnu/CORBA/CDR/AbstractCdrOutput.java:877:an an
libjava/classpath/gnu/javax/crypto/jce/params/BlockCipherParameters.java:78:an 
an
libjava/classpath/org/omg/CORBA/CompletionStatusHelper.java:93:an an

[PATCH gcc/fortran] get rid of gfc_free

[Jim Meyering]

Janne Blomqvist wrote:
 On Tue, Mar 8, 2011 at 19:53, Jim Meyering j...@meyering.net wrote:
 Relative to v2, I've added libgo/ to the list of exempt directories and added
 this recently discussed gfc_free patch, at the request of Tobias Burnus.
 Also, I corrected an error in fortran's ChangeLog and removed all
 whitespace changes from all ChangeLog files.

 The libgfortran changes are Ok for 4.7.

 For the gfortran frontend (gcc/fortran/*) I'd prefer if you'd

 - Replace all calls to gfc_free (x) with free (x).
 - Remove the gfc_free() function and prototype.
 - Remove the free() macro which currently prevents calling free() directly.

Hi Janne,

These requested changes are in addition to (and independent of)
the changes that I've already posted here.  The first cset below
does your #2 and #3, and the second does #1.  I separate them for
review because #1 is completely mechanical, while the others
are manual.  You may prefer to combine them before pushing, for
bisection.  Let me know if you'd prefer I submit in that form.

...
 +It is better to avoid the test.[*]
 +Instead, simply free P, regardless of whether it is NULL.
 +
 +[*] However, if your profiling exposes a test like this in a
 +performance-critical loop, say where P is nearly always NULL, and
 +the cost of calling free on a NULL pointer would be prohibitively
 +high, please let us know.

 Instead of please let us know, maybe recommend using
 __builtin_expect instead? E.g. something like

 if (__builtin_expect (ptr != NULL, 0))
 free (ptr);

Good idea.  Thanks.
Though how about avoiding the double negative?

   if (__builtin_expect (ptr == NULL, 1))
 free (ptr);

I've squashed the following onto my just-rebased commit:

diff --git a/gcc/README.Portability b/gcc/README.Portability
index e099a3f..4101a2f 100644
--- a/gcc/README.Portability
+++ b/gcc/README.Portability
@@ -68,7 +68,11 @@ Instead, simply free P, regardless of whether it is NULL.
 [*] However, if your profiling exposes a test like this in a
 performance-critical loop, say where P is nearly always NULL, and
 the cost of calling free on a NULL pointer would be prohibitively
-high, please let us know.
+high, consider using __builtin_expect, e.g., like this:
+
+  if (__builtin_expect (ptr == NULL, 1))
+free (ptr);
+


 Trigraphs
-


From c198b77add6d587cbe87f2d86049c46196946398 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 15 Mar 2011 10:33:03 +0100
Subject: [PATCH 1/2] gfortran: remove definition and prototype of gfc_free

* misc.c (gfc_free): Remove function.
* gfortran.h (gfc_free): Remove its prototype.
(free): Remove macro definition that would otherwise prevent
direct use of the function.
---
 gcc/fortran/ChangeLog  |7 +++
 gcc/fortran/gfortran.h |2 --
 gcc/fortran/misc.c |9 -
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
index 1871c71..bc2e1b2 100644
--- a/gcc/fortran/ChangeLog
+++ b/gcc/fortran/ChangeLog
@@ -1,3 +1,10 @@
+2011-03-15  Jim Meyering  meyer...@redhat.com
+
+   * misc.c (gfc_free): Remove function.
+   * gfortran.h (gfc_free): Remove its prototype.
+   (free): Remove macro definition that would otherwise prevent
+   direct use of the function.
+
 2011-03-08  Jim Meyering  meyer...@redhat.com

* gfortranspec.c (lang_specific_pre_link): Remove useless
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index b64fa20..24d1941 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -49,7 +49,6 @@ along with GCC; see the file COPYING3.  If not see
 #define MAX_SUBRECORD_LENGTH 2147483639   /* 2**31-9 */


-#define free(x) Use_gfc_free_instead_of_free()
 #define gfc_is_whitespace(c) ((c==' ') || (c=='\t'))

 /* Stringization.  */
@@ -2366,7 +2365,6 @@ void gfc_end_source_files (void);

 /* misc.c */
 void *gfc_getmem (size_t) ATTRIBUTE_MALLOC;
-void gfc_free (void *);
 int gfc_terminal_width (void);
 void gfc_clear_ts (gfc_typespec *);
 FILE *gfc_open_file (const char *);
diff --git a/gcc/fortran/misc.c b/gcc/fortran/misc.c
index 8a343a0..9d7d121 100644
--- a/gcc/fortran/misc.c
+++ b/gcc/fortran/misc.c
@@ -42,15 +42,6 @@ gfc_getmem (size_t n)
 }


-void
-gfc_free (void *p)
-{
-  /* The parentheses around free are needed in order to call not
- the redefined free of gfortran.h.  */
-  (free) (p);
-}
-
-
 /* Get terminal width.  */

 int
--
1.7.4.1.408.gb6b16


From 51ec56ecddf38589511502c58d75e1680e5a17e2 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 15 Mar 2011 10:37:17 +0100
Subject: [PATCH 2/2] convert each use of gfc_free (p) to free (p)

Do that by running this command:
  perl -pi -e's/\bgfc_free ?\(/free (/' \
$(git grep -El '\bgfc_free ?\(')
which also corrects the few uses that lacked a space between the
function name and the open parenthesis.
---
 gcc/fortran/ChangeLog |7 +
 gcc

Re: [PATCH gcc/fortran] get rid of gfc_free

[Jim Meyering]

Jakub Jelinek wrote:
 On Tue, Mar 15, 2011 at 11:02:38AM +0100, Jim Meyering wrote:
  Instead of please let us know, maybe recommend using
  __builtin_expect instead? E.g. something like
 
  if (__builtin_expect (ptr != NULL, 0))
  free (ptr);

 Good idea.  Thanks.
 Though how about avoiding the double negative?

if (__builtin_expect (ptr == NULL, 1))
  free (ptr);

 What double negative?  if (__builtin_expect (ptr != NULL, 0)) free (ptr);
 is certainly correct, the latter is wrong, it will leak memory.
 It will call free only if ptr is NULL, i.e. do a useless free (NULL),
 if it is non-NULL, it will not do anything.
 You could do
   if (!__builtin_expect (ptr == NULL, 1))
 free (ptr);
 but that doesn't seem to be nicer or clearer than
   if (__builtin_expect (ptr != NULL, 0))
 free (ptr);

Thanks for the quick correction.
I've fixed it locally, too.

Re: [PATCH gcc/fortran] get rid of gfc_free

[Jim Meyering]

Janne Blomqvist wrote:
...
 Hi Janne,

 These requested changes are in addition to (and independent of)
 the changes that I've already posted here.

 Yes, it was perhaps a bit unreasonable to ask you to fix this. OTOH
 with your changes gfc_free() was just a wrapper around free() and
 should thus be removed as unnecessary. Also, I believe this proper
 fix is more in the spirit of the request by Tobias and the message he
 linked to discussing the removal of gfc_free().

 The first cset below
 does your #2 and #3, and the second does #1.  I separate them for
 review because #1 is completely mechanical, while the others
 are manual.  You may prefer to combine them before pushing, for
 bisection.  Let me know if you'd prefer I submit in that form.

 All 3 changesets are ok for 4.7.

 I think it's fine to commit them separately if you prefer. If so,
 preferably in the order #3, #1, #2 in order to keep every revision
 buildable.

 Thanks for working on this!

Just so we're clear...
Currently while I do have a sourceware account,
I'm not in the gcc group, so don't have commit access,

sourceware$ id -a|grep gcc
[Exit 1]

so someone else would have to commit my changes.
Or add me to the gcc group and I will do it.

Another recently-approved change may be in limbo for this reason:

avoid memory overrun in a test leading to potential double-free
* testsuite/test-expandargv.c (writeout_test): Fix off-by-one error:
i.e., do copy the trailing NUL byte.

[PATCH] avoid memory overrun in a test leading to potential double-free

[Jim Meyering]

I ran make check and was dismayed to see that glibc detected a
double-free.  At first I thought it must be my fault, since I'd been
removing useless tests before free, but no...

Running valgrind ./test-expandargv confirmed it:

  ==29710== Conditional jump or move depends on uninitialised value(s)
  ==29710==at 0x400E14: run_replaces (test-expandargv.c:121)
  ==29710==by 0x400F63: writeout_test (test-expandargv.c:151)
  ==29710==by 0x401037: run_tests (test-expandargv.c:188)
  ==29710==by 0x40124C: main (test-expandargv.c:264)


From f60778ef0f07983b0ba72ed97fe52b687de28abb Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Tue, 8 Mar 2011 13:54:13 +0100
Subject: [PATCH] avoid memory overrun in a test leading to potential double-free

* testsuite/test-expandargv.c (writeout_test): Fix off-by-one error:
i.e., do copy the trailing NUL byte.
---
 libiberty/ChangeLog   |6 ++
 libiberty/testsuite/test-expandargv.c |2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog
index dc92638..802cf96 100644
--- a/libiberty/ChangeLog
+++ b/libiberty/ChangeLog
@@ -1,6 +1,12 @@
+2011-03-08  Jim Meyering  meyer...@redhat.com
+
+   avoid memory overrun in a test leading to potential double-free
+   * testsuite/test-expandargv.c (writeout_test): Fix off-by-one error:
+   i.e., do copy the trailing NUL byte.
+
 2011-02-28  Kai Tietz  kai.ti...@onevision.com

* filename_cmp.c (filename_ncmp): New function.
* functions.texi: Regenerated.

 2011-02-03  Ralf Wildenhues  ralf.wildenh...@gmx.de
diff --git a/libiberty/testsuite/test-expandargv.c 
b/libiberty/testsuite/test-expandargv.c
index c16a032..57b96b3 100644
--- a/libiberty/testsuite/test-expandargv.c
+++ b/libiberty/testsuite/test-expandargv.c
@@ -201,13 +201,13 @@ writeout_test (int test, const char * test_data)
   /* Generate RW copy of data for replaces */
   len = strlen (test_data);
   parse = malloc (sizeof (char) * (len + 1));
   if (parse == NULL)
 fatal_error (__LINE__, Failed to malloc parse., errno);
   
-  memcpy (parse, test_data, sizeof (char) * len);
+  memcpy (parse, test_data, sizeof (char) * (len + 1));
   /* Run all possible replaces */
   run_replaces (parse);

   fwrite (parse, len, sizeof (char), fd);
   free (parse);
   fclose (fd);
-- 
1.7.4.1.299.ga459d

Re: avoid useless if-before-free tests

[Jim Meyering]

Dr Andrew John Hughes wrote:
 On 22:47 Mon 07 Mar , Joseph S. Myers wrote:
 On Mon, 7 Mar 2011, Dr Andrew John Hughes wrote:

   http://gcc.gnu.org/codingconventions.html says Classpath changes should 
   go
   via Classpath upstream, not directly into GCC.  I don't know if that's
   still accurate.
  
 
  That's still true.  This seems to be the first message I've received in 
  this
  thread, so I'm not even aware of what these changes are.  Were the earlier
  messages not sent to this list?

 The original patch went only to gcc-patches.

 http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00252.html


 Thanks for the link.

 I'd like some explanation of why these changes are necessary before we start
 adding them to Classpath.  Are we just assuming that all free implementations
 will ignore NULL now?

IMHO, they're not officially necessary, but rather nice to have,
since they eliminate code that is now obviously obsolete.
Those tests have been unnecessary for at least 5 years.
The efficiency (of removing the redundant test) is never the
issue for me, personally.  My main argument for making the change
is improved maintainability/readability:

  - less logic (esp. when the expression is more complicated)
  - no surprise (for reviewers who stopped using such tests years ago)
  - more compact, so more lines fit on a page/screen
  - removing unused code is always worthwhile

Sort of along the same lines as removing anachronistic casts of
malloc/calloc/realloc return values in C code.  No longer needed,
but many people continue to use them for no good reason.

Re: [PATCH v2] Re: avoid useless if-before-free tests

[Jim Meyering]

Rainer Orth wrote:
 Jim Meyering j...@meyering.net writes:
 the change at hand.  And please don't change the alignment of entries
 with multiple email addresses.

 Changing 8-spaces to a TAB does not affect alignment when you're
 looking at the ChangeLog file itself with standard tab setting.

 Perhaps you looked at a hunk like the following and mistook it
 for one that introduces an alignment change?

  2011-11-04  Eric Botcazou  ebotca...@adacore.com
 -Jakub Jelinek  ja...@redhat.com
 +   Jakub Jelinek  ja...@redhat.com

 It does not.

 I'm pretty sure it does: before, you have 12 SPC, afterwards you have
 TAB + 3 SPC, which is equivalent to 11 SPC in my book.

Here's the precise excerpt from my patch:

 2011-11-04  Eric Botcazou  ebotca...@adacore.com
-Jakub Jelinek  ja...@redhat.com
+   Jakub Jelinek  ja...@redhat.com

That has TAB + 4, so induced no alignment change.

 I honestly don't see the point of this whitespace change unless done
 across all ChangeLogs, not just a few that you happen to touch.

As I said, it's gone, now, from my patch.
If no one objects, I'll normalize all ChangeLog files.