bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: enable -Werror for lib/ in coreutils


From: Jim Meyering
Subject: Re: enable -Werror for lib/ in coreutils
Date: Thu, 29 Oct 2009 09:23:39 +0100

Paolo Bonzini wrote:
>> +diff --git a/lib/regcomp.c b/lib/regcomp.c

Hi Paolo.

Thanks for the review!

> This is okay.
>
>> diff --git a/gl/lib/regex_internal.c.diff b/gl/lib/regex_internal.c.diff
>
> This is okay.  There is one caller of re_node_set_remove_at, in
> regexec.c, which might pass SIZE_MAX as the second parameter to
> re_node_set_remove_at.  This works, but I'd make sure this does not
> happen by doing this:
>
> diff --git a/lib/regexec.c b/lib/regexec.c
> index 21a8166..34630fa 100644
> --- a/lib/regexec.c
> +++ b/lib/regexec.c
> @@ -1913,8 +1913,9 @@
>         Idx cur_node = inv_eclosure->elems[ecl_idx];
>         if (!re_node_set_contains (&except_nodes, cur_node))
>           {
> -           Idx idx = re_node_set_contains (dest_nodes, cur_node) - 1;
> -           re_node_set_remove_at (dest_nodes, idx);
> +           Idx idx = re_node_set_contains (dest_nodes, cur_node);
> +           if (idx)
> +             re_node_set_remove_at (dest_nodes, idx - 1);

That looks like a good bug fix.

>           }
>       }
>      re_node_set_free (&except_nodes);
>
>
> However...
>
>> diff --git a/gl/lib/regexec.c.diff b/gl/lib/regexec.c.diff
>
> ... this is wrong,

True, I had introduced an error there (I'd eliminated a still-useful
comparison -- corrected in the patch below), but...

> and in fact the entire castle crumbles because of this patch.

I don't see how that error would make the castle crumble.
Nor do I see any more-fundamental error in Paul's use of an unsigned
type or in my elimination of tests like "unsigned_var < 0" that
can never be true.

The way I see it is that if an unsigned type is appropriate to the
semantics of the variables being modeled, then an unsigned type
is required.

Maybe you were looking at this code?
One might think we'd have to accommodate the #else block.

  #ifdef _REGEX_LARGE_OFFSETS
    ...comments elided...
    typedef ssize_t regoff_t;
    typedef size_t __re_idx_t;
    typedef size_t __re_size_t;
    typedef size_t __re_long_size_t;
  #else
    /* Use types that are binary-compatible with the traditional GNU regex
       implementation, which mishandles strings longer than INT_MAX.  */
    typedef int regoff_t;
    typedef int __re_idx_t;
    typedef unsigned int __re_size_t;
    typedef unsigned long int __re_long_size_t;
  #endif

But for us, _REGEX_LARGE_OFFSETS is always defined via regex.m4,
so the code in the #else block is never used.

> Idx i.e. __re_idx_t is size_t, but it replaced int with
> Paul Eggert's patch to support >2GB subjects and regexes (which Uli
> rejected upstream).
>
> If you wish, feel free to commit to gnulib the other patches (plus the
> regexec.c hunk above).  However, I think it's better to make a
> different patch, which defines __re_idx_t as ptrdiff_t.  Such a patch
> would also remove the
>
>   if ((Idx) -1 < 0 || end != REG_MISSING)
>
> condition from regcomp.c which is absent upstream.  Also, with this
> alternative patch the change to regexec.c that I suggested above would
> be unnecessary.

Why use a signed type throughout rege*.[ch] when an unsigned one
more accurately models the data and interfaces?


>From f2859424cbdf23f6673fd4f3a9d7a5b197e0595e Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 27 Oct 2009 12:06:43 +0100
Subject: [PATCH] build (--enable-gcc-warnings): enable gcc's -Werror also in 
lib/

* configure.ac (GNULIB_WARN_CFLAGS): Define.
* lib/Makefile.am (AM_CFLAGS): Use $(GNULIB_WARN_CFLAGS)
rather than $(WARN_CFLAGS) and add $(WERROR_CFLAGS).
* gl/lib/regcomp.c.diff: New file.
* gl/lib/regex_internal.c.diff: New file.
* gl/lib/regexec.c.diff: New file.
---
 configure.ac                 |   10 +++++++++
 gl/lib/regcomp.c.diff        |   23 +++++++++++++++++++++
 gl/lib/regex_internal.c.diff |   25 +++++++++++++++++++++++
 gl/lib/regexec.c.diff        |   45 ++++++++++++++++++++++++++++++++++++++++++
 lib/Makefile.am              |    2 +-
 5 files changed, 104 insertions(+), 1 deletions(-)
 create mode 100644 gl/lib/regcomp.c.diff
 create mode 100644 gl/lib/regex_internal.c.diff
 create mode 100644 gl/lib/regexec.c.diff

diff --git a/configure.ac b/configure.ac
index 3efc819..fb963ee 100644
--- a/configure.ac
+++ b/configure.ac
@@ -114,6 +114,16 @@ if test "$gl_gcc_warnings" = yes; then
   AC_DEFINE([_FORTIFY_SOURCE], [2],
     [enable compile-time and run-time bounds-checking, and some warnings])
   AC_DEFINE([GNULIB_PORTCHECK], [1], [enable some gnulib portability checks])
+
+  # We use a slightly smaller set of warning options for lib/.
+  # Remove the following and save the result in GNULIB_WARN_CFLAGS.
+  nw=
+  nw="$nw -Wuninitialized"
+  nw="$nw -Wunused-macros"
+  nw="$nw -Wmissing-prototypes"
+  nw="$nw -Wold-style-definition"
+  gl_MANYWARN_COMPLEMENT([GNULIB_WARN_CFLAGS], [$WARN_CFLAGS], [$nw])
+  AC_SUBST([GNULIB_WARN_CFLAGS])
 fi

 AC_FUNC_FORK
diff --git a/gl/lib/regcomp.c.diff b/gl/lib/regcomp.c.diff
new file mode 100644
index 0000000..88097a4
--- /dev/null
+++ b/gl/lib/regcomp.c.diff
@@ -0,0 +1,23 @@
+diff --git a/lib/regcomp.c b/lib/regcomp.c
+index 6472ff6..fadf36d 100644
+--- a/lib/regcomp.c
++++ b/lib/regcomp.c
+@@ -18,6 +18,8 @@
+    with this program; if not, write to the Free Software Foundation,
+    Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */
+
++#include "intprops.h"
++#include "verify.h"
+ static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern,
+                                         size_t length, reg_syntax_t syntax);
+ static void re_compile_fastmap_iter (regex_t *bufp,
+@@ -2571,7 +2573,8 @@ parse_dup_op (bin_tree_t *elem, re_string_t *regexp, 
re_dfa_t *dfa,
+   /* This loop is actually executed only when end != REG_MISSING,
+      to rewrite <re>{0,n} as (<re>(<re>...<re>?)?)?...  We have
+      already created the start+1-th copy.  */
+-  if ((Idx) -1 < 0 || end != REG_MISSING)
++  verify (! TYPE_SIGNED (Idx));
++  if (end != REG_MISSING)
+     for (i = start + 2; i <= end; ++i)
+       {
+       elem = duplicate_tree (elem, dfa);
diff --git a/gl/lib/regex_internal.c.diff b/gl/lib/regex_internal.c.diff
new file mode 100644
index 0000000..2cede3c
--- /dev/null
+++ b/gl/lib/regex_internal.c.diff
@@ -0,0 +1,25 @@
+diff --git a/lib/regex_internal.c b/lib/regex_internal.c
+index 904b88e..61c8d9d 100644
+--- a/lib/regex_internal.c
++++ b/lib/regex_internal.c
+@@ -18,6 +18,8 @@
+    with this program; if not, write to the Free Software Foundation,
+    Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */
+
++#include "verify.h"
++#include "intprops.h"
+ static void re_string_construct_common (const char *str, Idx len,
+                                       re_string_t *pstr,
+                                       RE_TRANSLATE_TYPE trans, bool icase,
+@@ -1390,7 +1392,10 @@ static void
+ internal_function
+ re_node_set_remove_at (re_node_set *set, Idx idx)
+ {
+-  if (idx < 0 || idx >= set->nelem)
++  verify (! TYPE_SIGNED (Idx));
++  /* if (idx < 0)
++     return; */
++  if (idx >= set->nelem)
+     return;
+   --set->nelem;
+   for (; idx < set->nelem; idx++)
diff --git a/gl/lib/regexec.c.diff b/gl/lib/regexec.c.diff
new file mode 100644
index 0000000..5c985f9
--- /dev/null
+++ b/gl/lib/regexec.c.diff
@@ -0,0 +1,45 @@
+diff --git a/lib/regexec.c b/lib/regexec.c
+index 21a8166..3b0d85c 100644
+--- a/lib/regexec.c
++++ b/lib/regexec.c
+@@ -18,6 +18,8 @@
+    with this program; if not, write to the Free Software Foundation,
+    Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */
+
++#include "verify.h"
++#include "intprops.h"
+ static reg_errcode_t match_ctx_init (re_match_context_t *cache, int eflags,
+                                    Idx n) internal_function;
+ static void match_ctx_clean (re_match_context_t *mctx) internal_function;
+@@ -378,8 +380,11 @@ re_search_2_stub (struct re_pattern_buffer *bufp,
+   Idx len = length1 + length2;
+   char *s = NULL;
+
+-  if (BE (length1 < 0 || length2 < 0 || stop < 0 || len < length1, 0))
+-    return -2;
++  verify (! TYPE_SIGNED (Idx));
++  if (BE (len < length1, 0))
++     return -2; */
++  /* if (BE (length1 < 0 || length2 < 0 || stop < 0, 0))
++     return -2; */
+
+   /* Concatenate the strings.  */
+   if (length2 > 0)
+@@ -431,11 +436,14 @@ re_search_stub (struct re_pattern_buffer *bufp,
+   Idx last_start = start + range;
+
+   /* Check for out-of-range.  */
+-  if (BE (start < 0 || start > length, 0))
+-    return -1;
++  verify (! TYPE_SIGNED (Idx));
++  /* if (BE (start < 0, 0))
++     return -1; */
++  if (BE (start > length, 0))
++     return -1;
+   if (BE (length < last_start || (0 <= range && last_start < start), 0))
+     last_start = length;
+-  else if (BE (last_start < 0 || (range < 0 && start <= last_start), 0))
++  else if (BE (/* last_start < 0 || */ (range < 0 && start <= last_start), 0))
+     last_start = 0;
+
+   __libc_lock_lock (dfa->lock);
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 074cc9c..896458f 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -17,7 +17,7 @@

 include gnulib.mk

-AM_CFLAGS += $(WARN_CFLAGS) # $(WERROR_CFLAGS)
+AM_CFLAGS += $(GNULIB_WARN_CFLAGS) $(WERROR_CFLAGS)

 libcoreutils_a_SOURCES += \
   buffer-lcm.c buffer-lcm.h \
--
1.6.5.2.375.g164f1




reply via email to

[Prev in Thread] Current Thread [Next in Thread]