From: Jani Nikula <jani@nikula.org>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH] xutil.c: remove duplicate copies,
	create new library libutil.a to contain xutil.
In-Reply-To: <1319383133-11006-1-git-send-email-david@tethera.net>
References: <1319383133-11006-1-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.5-232-g917e874 (http://notmuchmail.org) Emacs/23.1.1
	(i686-pc-linux-gnu)
Date: Mon, 31 Oct 2011 09:20:13 +0000
Message-ID: <877h3ly0tu.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Cc: David Bremner <bremner@debian.org>
Precedence: list


Hi David, this patch (commit 1dedfc90f6eee7cad10f1a1ceb39a7a1c4dbd1b1)
broke my build with:

CXX -O2 lib/libnotmuch.so.2.0.0
lib/message-file.o: In function `copy_header_unfolding':
message-file.c:(.text+0xea): undefined reference to `xrealloc'
lib/message-file.o: In function
`notmuch_message_file_restrict_headersv':
message-file.c:(.text+0x2cc): undefined reference to `xstrdup'
lib/message-file.o: In function `notmuch_message_file_get_header':
message-file.c:(.text+0x4af): undefined reference to `xstrndup'
message-file.c:(.text+0x5c8): undefined reference to `xmalloc'
lib/messages.o: In function `notmuch_messages_collect_tags':
messages.c:(.text+0x1c4): undefined reference to `xstrdup'
lib/sha1.o: In function `_hex_of_sha1_digest':
sha1.c:(.text+0x18): undefined reference to `xcalloc'
lib/thread.o: In function `_notmuch_thread_create':
thread.cc:(.text+0x34e): undefined reference to `xstrdup'
thread.cc:(.text+0x58c): undefined reference to `xstrdup'
/usr/bin/ld: lib/libnotmuch.so.2.0.0: hidden symbol `xmalloc' isn't
defined
/usr/bin/ld: final link failed: Bad value
collect2: ld returned 1 exit status
make: *** [lib/libnotmuch.so.2.0.0] Error 1

I don't have the time to debug this further right now, but I bisected
the failure to this commit and did a clean clone to make sure leftover
files weren't the cause.

BR,
Jani.


On Sun, 23 Oct 2011 12:18:53 -0300, David Bremner <david@tethera.net> wrote:
> From: David Bremner <bremner@debian.org>
>=20
> We keep the lib/xutil.c version. As a consequence, also factor out
> _internal_error and associated macros.  It might be overkill to make a
> new file error_util.c for this, but _internal_error does not really
> belong in database.cc.
> ---
>=20
> This turned out to be more disruptive than I thought. On the other
> hand, having two copies of xutil.c seems like a recipe for disaster.
> I wanted to factor out the logic in xregcomp so I could use it in
> situations where miscompilation is not an internal error, but more
> likely a user error.
>=20
>  Makefile              |    2 +-
>  Makefile.local        |    7 +--
>  lib/Makefile.local    |    5 +-
>  lib/database.cc       |   15 -----
>  lib/notmuch-private.h |   20 +-------
>  lib/xutil.c           |  134 -------------------------------------------=
----
>  lib/xutil.h           |   55 -------------------
>  util/Makefile         |    5 ++
>  util/Makefile.local   |   11 ++++
>  util/error_util.c     |   41 +++++++++++++++
>  util/error_util.h     |   45 ++++++++++++++++
>  util/xutil.c          |  136 +++++++++++++++++++++++++++++++++++++++++++=
+++++
>  util/xutil.h          |   55 +++++++++++++++++++
>  xutil.c               |  138 -------------------------------------------=
------
>  14 files changed, 300 insertions(+), 369 deletions(-)
>  delete mode 100644 lib/xutil.c
>  delete mode 100644 lib/xutil.h
>  create mode 100644 util/Makefile
>  create mode 100644 util/Makefile.local
>  create mode 100644 util/error_util.c
>  create mode 100644 util/error_util.h
>  create mode 100644 util/xutil.c
>  create mode 100644 util/xutil.h
>  delete mode 100644 xutil.c
>=20
> diff --git a/Makefile b/Makefile
> index 11e3a3d..2fb2a61 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3,7 +3,7 @@
>  all:
>=20=20
>  # List all subdirectories here. Each contains its own Makefile.local
> -subdirs =3D compat completion emacs lib test
> +subdirs =3D compat completion emacs lib util test
>=20=20
>  # We make all targets depend on the Makefiles themselves.
>  global_deps =3D Makefile Makefile.config Makefile.local \
> diff --git a/Makefile.local b/Makefile.local
> index 38f6c17..e31defa 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -39,7 +39,7 @@ PV_FILE=3Dbindings/python/notmuch/version.py
>  # Smash together user's values with our extra values
>  FINAL_CFLAGS =3D -DNOTMUCH_VERSION=3D$(VERSION) $(CFLAGS) $(WARN_CFLAGS)=
 $(CONFIGURE_CFLAGS) $(extra_cflags)
>  FINAL_CXXFLAGS =3D $(CXXFLAGS) $(WARN_CXXFLAGS) $(CONFIGURE_CXXFLAGS) $(=
extra_cflags) $(extra_cxxflags)
> -FINAL_NOTMUCH_LDFLAGS =3D $(LDFLAGS) -Llib -lnotmuch $(AS_NEEDED_LDFLAGS=
) $(GMIME_LDFLAGS) $(TALLOC_LDFLAGS)
> +FINAL_NOTMUCH_LDFLAGS =3D $(LDFLAGS) -Lutil -lutil -Llib -lnotmuch $(AS_=
NEEDED_LDFLAGS) $(GMIME_LDFLAGS) $(TALLOC_LDFLAGS)
>  FINAL_NOTMUCH_LINKER =3D CC
>  ifneq ($(LINKER_RESOLVES_LIBRARY_DEPENDENCIES),1)
>  FINAL_NOTMUCH_LDFLAGS +=3D $(CONFIGURE_LDFLAGS)
> @@ -288,12 +288,11 @@ notmuch_client_srcs =3D		\
>  	notmuch-time.c		\
>  	query-string.c		\
>  	show-message.c		\
> -	json.c			\
> -	xutil.c
> +	json.c=09=09=09
>=20=20
>  notmuch_client_modules =3D $(notmuch_client_srcs:.c=3D.o)
>=20=20
> -notmuch: $(notmuch_client_modules) lib/libnotmuch.a
> +notmuch: $(notmuch_client_modules) lib/libnotmuch.a util/libutil.a
>  	$(call quiet,CXX $(CFLAGS)) $^ $(FINAL_LIBNOTMUCH_LDFLAGS) -o $@
>=20=20
>  notmuch-shared: $(notmuch_client_modules) lib/$(LINKER_NAME)
> diff --git a/lib/Makefile.local b/lib/Makefile.local
> index ea20b2b..f148661 100644
> --- a/lib/Makefile.local
> +++ b/lib/Makefile.local
> @@ -49,8 +49,7 @@ libnotmuch_c_srcs =3D		\
>  	$(dir)/message-file.c	\
>  	$(dir)/messages.c	\
>  	$(dir)/sha1.c		\
> -	$(dir)/tags.c		\
> -	$(dir)/xutil.c
> +	$(dir)/tags.c=09=09
>=20=20
>  libnotmuch_cxx_srcs =3D		\
>  	$(dir)/database.cc	\
> @@ -66,7 +65,7 @@ $(dir)/libnotmuch.a: $(libnotmuch_modules)
>  	$(call quiet,AR) rcs $@ $^
>=20=20
>  $(dir)/$(LIBNAME): $(libnotmuch_modules) notmuch.sym
> -	$(call quiet,CXX $(CXXFLAGS)) $(libnotmuch_modules) $(FINAL_LIBNOTMUCH_=
LDFLAGS) $(LIBRARY_LINK_FLAG) -o $@
> +	$(call quiet,CXX $(CXXFLAGS)) $(libnotmuch_modules) $(FINAL_LIBNOTMUCH_=
LDFLAGS) $(LIBRARY_LINK_FLAG) -o $@ -L$(srcdir)/util -lutil
>=20=20
>  notmuch.sym: lib/notmuch.h $(libnotmuch_modules)
>  	sh lib/gen-version-script.sh $< $(libnotmuch_modules) > $@
> diff --git a/lib/database.cc b/lib/database.cc
> index e77fd53..88be939 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -209,21 +209,6 @@ static prefix_t PROBABILISTIC_PREFIX[]=3D {
>      { "folder",			"XFOLDER"}
>  };
>=20=20
> -int
> -_internal_error (const char *format, ...)
> -{
> -    va_list va_args;
> -
> -    va_start (va_args, format);
> -
> -    fprintf (stderr, "Internal error: ");
> -    vfprintf (stderr, format, va_args);
> -
> -    exit (1);
> -
> -    return 1;
> -}
> -
>  const char *
>  _find_prefix (const char *name)
>  {
> diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
> index d319530..0d3cc27 100644
> --- a/lib/notmuch-private.h
> +++ b/lib/notmuch-private.h
> @@ -47,6 +47,7 @@ NOTMUCH_BEGIN_DECLS
>  #include <talloc.h>
>=20=20
>  #include "xutil.h"
> +#include "error_util.h"
>=20=20
>  #pragma GCC visibility push(hidden)
>=20=20
> @@ -60,25 +61,6 @@ NOTMUCH_BEGIN_DECLS
>  #define STRNCMP_LITERAL(var, literal) \
>      strncmp ((var), (literal), sizeof (literal) - 1)
>=20=20
> -/* There's no point in continuing when we've detected that we've done
> - * something wrong internally (as opposed to the user passing in a
> - * bogus value).
> - *
> - * Note that PRINTF_ATTRIBUTE comes from talloc.h
> - */
> -int
> -_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
> -
> -/* There's no point in continuing when we've detected that we've done
> - * something wrong internally (as opposed to the user passing in a
> - * bogus value).
> - *
> - * Note that __location__ comes from talloc.h.
> - */
> -#define INTERNAL_ERROR(format, ...)			\
> -    _internal_error (format " (%s).\n",			\
> -		     ##__VA_ARGS__, __location__)
> -
>  #define unused(x) x __attribute__ ((unused))
>=20=20
>  #ifdef __cplusplus
> diff --git a/lib/xutil.c b/lib/xutil.c
> deleted file mode 100644
> index 268225b..0000000
> diff --git a/lib/xutil.h b/lib/xutil.h
> deleted file mode 100644
> index fd77f73..0000000
> diff --git a/util/Makefile b/util/Makefile
> new file mode 100644
> index 0000000..fa25832
> --- /dev/null
> +++ b/util/Makefile
> @@ -0,0 +1,5 @@
> +all:
> +	$(MAKE) -C .. all
> +
> +.DEFAULT:
> +	$(MAKE) -C .. $@
> diff --git a/util/Makefile.local b/util/Makefile.local
> new file mode 100644
> index 0000000..2ff42b3
> --- /dev/null
> +++ b/util/Makefile.local
> @@ -0,0 +1,11 @@
> +# -*- makefile -*-
> +
> +dir :=3D util
> +extra_cflags +=3D -I$(srcdir)/$(dir)
> +
> +libutil_c_srcs :=3D $(dir)/xutil.c $(dir)/error_util.c
> +
> +libutil_modules :=3D $(libutil_c_srcs:.c=3D.o)
> +
> +$(dir)/libutil.a: $(libutil_modules)
> +	$(call quiet,AR) rcs $@ $^
> diff --git a/util/error_util.c b/util/error_util.c
> new file mode 100644
> index 0000000..630d228
> --- /dev/null
> +++ b/util/error_util.c
> @@ -0,0 +1,41 @@
> +/* error_util.c - internal error utilities for notmuch.
> + *
> + * Copyright =C2=A9 2009 Carl Worth
> + *
> + * This program 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 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: Carl Worth <cworth@cworth.org>
> + */
> +
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +
> +#include "error_util.h"
> +
> +int
> +_internal_error (const char *format, ...)
> +{
> +    va_list va_args;
> +
> +    va_start (va_args, format);
> +
> +    fprintf (stderr, "Internal error: ");
> +    vfprintf (stderr, format, va_args);
> +
> +    exit (1);
> +
> +    return 1;
> +}
> +
> diff --git a/util/error_util.h b/util/error_util.h
> new file mode 100644
> index 0000000..0f1e5ef
> --- /dev/null
> +++ b/util/error_util.h
> @@ -0,0 +1,45 @@
> +/* error_util.h - Internal interfaces for notmuch.
> + *
> + * Copyright =C2=A9 2009 Carl Worth
> + *
> + * This program 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 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: Carl Worth <cworth@cworth.org>
> + */
> +
> +#ifndef ERROR_UTIL_H
> +#define ERROR_UTIL_H
> +
> +#include <talloc.h>
> +
> +/* There's no point in continuing when we've detected that we've done
> + * something wrong internally (as opposed to the user passing in a
> + * bogus value).
> + *
> + * Note that PRINTF_ATTRIBUTE comes from talloc.h
> + */
> +int
> +_internal_error (const char *format, ...) PRINTF_ATTRIBUTE (1, 2);
> +
> +/* There's no point in continuing when we've detected that we've done
> + * something wrong internally (as opposed to the user passing in a
> + * bogus value).
> + *
> + * Note that __location__ comes from talloc.h.
> + */
> +#define INTERNAL_ERROR(format, ...)			\
> +    _internal_error (format " (%s).\n",			\
> +		     ##__VA_ARGS__, __location__)
> +
> +#endif
> diff --git a/util/xutil.c b/util/xutil.c
> new file mode 100644
> index 0000000..15ff765
> --- /dev/null
> +++ b/util/xutil.c
> @@ -0,0 +1,136 @@
> +/* xutil.c - Various wrapper functions to abort on error.
> + *
> + * Copyright =C2=A9 2009 Carl Worth
> + *
> + * This program 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 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: Carl Worth <cworth@cworth.org>
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "xutil.h"
> +#include "error_util.h"
> +
> +void *
> +xcalloc (size_t nmemb, size_t size)
> +{
> +    void *ret;
> +
> +    ret =3D calloc (nmemb, size);
> +    if (ret =3D=3D NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	exit (1);
> +    }
> +
> +    return ret;
> +}
> +
> +void *
> +xmalloc (size_t size)
> +{
> +    void *ret;
> +
> +    ret =3D malloc (size);
> +    if (ret =3D=3D NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	exit (1);
> +    }
> +
> +    return ret;
> +}
> +
> +void *
> +xrealloc (void *ptr, size_t size)
> +{
> +    void *ret;
> +
> +    ret =3D realloc (ptr, size);
> +    if (ret =3D=3D NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	exit (1);
> +    }
> +
> +    return ret;
> +}
> +
> +char *
> +xstrdup (const char *s)
> +{
> +    char *ret;
> +
> +    ret =3D strdup (s);
> +    if (ret =3D=3D NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	exit (1);
> +    }
> +
> +    return ret;
> +}
> +
> +char *
> +xstrndup (const char *s, size_t n)
> +{
> +    char *ret;
> +
> +    if (strlen (s) <=3D n)
> +	n =3D strlen (s);
> +
> +    ret =3D malloc (n + 1);
> +    if (ret =3D=3D NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	exit (1);
> +    }
> +    memcpy (ret, s, n);
> +    ret[n] =3D '\0';
> +
> +    return ret;
> +}
> +
> +void
> +xregcomp (regex_t *preg, const char *regex, int cflags)
> +{
> +    int rerr;
> +
> +    rerr =3D regcomp (preg, regex, cflags);
> +    if (rerr) {
> +	size_t error_size =3D regerror (rerr, preg, NULL, 0);
> +	char *error =3D xmalloc (error_size);
> +
> +	regerror (rerr, preg, error, error_size);
> +	INTERNAL_ERROR ("compiling regex %s: %s\n",
> +			regex, error);
> +    }
> +}
> +
> +int
> +xregexec (const regex_t *preg, const char *string,
> +	  size_t nmatch, regmatch_t pmatch[], int eflags)
> +{
> +    unsigned int i;
> +    int rerr;
> +
> +    rerr =3D regexec (preg, string, nmatch, pmatch, eflags);
> +    if (rerr)
> +	return rerr;
> +
> +    for (i =3D 0; i < nmatch; i++) {
> +	if (pmatch[i].rm_so =3D=3D -1)
> +	    INTERNAL_ERROR ("matching regex against %s: Sub-match %d not found\=
n",
> +			    string, i);
> +    }
> +
> +    return 0;
> +}
> diff --git a/util/xutil.h b/util/xutil.h
> new file mode 100644
> index 0000000..fd77f73
> --- /dev/null
> +++ b/util/xutil.h
> @@ -0,0 +1,55 @@
> +/* xutil.h - Various wrapper functions to abort on error.
> + *
> + * Copyright =C2=A9 2009 Carl Worth
> + *
> + * This program 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 of the License, or
> + * (at your option) any later version.
> + *
> + * This program 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: Carl Worth <cworth@cworth.org>
> + */
> +
> +#ifndef NOTMUCH_XUTIL_H
> +#define NOTMUCH_XUTIL_H
> +
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <regex.h>
> +
> +#pragma GCC visibility push(hidden)
> +
> +/* xutil.c */
> +void *
> +xcalloc (size_t nmemb, size_t size);
> +
> +void *
> +xmalloc (size_t size);
> +
> +void *
> +xrealloc (void *ptrr, size_t size);
> +
> +char *
> +xstrdup (const char *s);
> +
> +char *
> +xstrndup (const char *s, size_t n);
> +
> +void
> +xregcomp (regex_t *preg, const char *regex, int cflags);
> +
> +int
> +xregexec (const regex_t *preg, const char *string,
> +	  size_t nmatch, regmatch_t pmatch[], int eflags);
> +
> +#pragma GCC visibility pop
> +
> +#endif
> diff --git a/xutil.c b/xutil.c
> deleted file mode 100644
> index 5f98f3f..0000000
> --=20
> 1.7.6.3
>=20
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch