public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH][gdb/build] Hardcode --with-included-regex
@ 2021-04-13 15:34 Tom de Vries
  2021-04-19 19:47 ` Tom Tromey
  0 siblings, 1 reply; 4+ messages in thread
From: Tom de Vries @ 2021-04-13 15:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

Hi,

Currently gdb has a configure option:
...
$ ./src/gdb/configure  --help
  ...
  --without-included-regex
                          don't use included regex; this is the default on
                          systems with version 2 of the GNU C library (use
                          with caution on other system)
...

The configure option controls config.h macro USE_INCLUDED_REGEX, which is
used in gdb/gdb_regex.h to choose between:
- using regex from libiberty (which is included in the binutils-gdb.git repo,
  hence the 'included' in USE_INCLUDED_REGEX), or
- using regex.h.

In the former case, the symbol regcomp is remapped to a symbol xregcomp, which
is then provided by libiberty.

In the latter case, the symbol regcomp is resolved at runtime, usually binding
to libc.  However, there is no mechanism in place to enforce this.

PR27681 is an example of where that causes problems.  On openSUSE Tumbleweed,
the ncurses package got the --with-pcre2 configure switch enabled, and solved
the resulting dependencies using:
...
 $ cat /usr/lib64/libncursesw.so
 /* GNU ld script */
-INPUT(/lib64/libncursesw.so.6 AS_NEEDED(-ltinfo -ldl))
+INPUT(/lib64/libncursesw.so.6 AS_NEEDED(-ltinfo -ldl -lpcre2-posix -lpcre2-8))
...

This lead to regcomp being bound to libpcre2-posix instead of libc.

This causes problems in several ways:
- by compiling using regex.h, we've already chosen a specific regex_t
  implementation, and the one from pcre2-posix is not the same.
- in gdb_regex.c we use GNU regex function re_search, which pcre2-posix
  doesn't provide, so while regcomp binds to pcre2-posix, re_search binds to
  libc.

A note on the latter: it's actually a bug to compile a regex using regcomp and
then pass it to re_search.  The GNU regex interface requires one to use
re_compile_pattern or re_compile_fastmap.  But as long we're using one of the
GNU regex incarnations in gnulib, glibc or libiberty, we get away with this.

The PR could be fixed by adding -lc in a specific position in the link line,
to force regcomp to be bound to glibc.  But this solution was considered
in the discussion in the PR as being brittle, and possibly causing problems
elsewhere.

Another solution offered was to restrict regex usage to posix, and no longer
use the GNU regex API.  This however could mean having to reproduce some of
that functionality locally, which would mean maintaining the same
functionality in more than one place.

The solution chosen here, is to hardcode --with-included-regex, that is, using
libiberty.

The option of using glibc for regex was introduced because glibc became the
authorative source for GNU regex, so it offered the possibility to link
against a more up-to-date regex version.

In that aspect, this patch is a step back.  But we have the option of using a
more up-to-date regex version as a follow-up step: by using the regex from
gnulib.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/build] Hardcode --with-included-regex

gdb/ChangeLog:

2021-04-13  Tom de Vries  <tdevries@suse.de>

	PR build/27681
	* configure.ac: Remove --without-included-regex/--with-included-regex.
	* config.in: Regenerate.
	* configure: Regenerate.
	* gdb_regex.h: Assume USE_INCLUDED_REGEX is defined.

---
 gdb/config.in    |  3 ---
 gdb/configure    | 59 --------------------------------------------------------
 gdb/configure.ac | 33 -------------------------------
 gdb/gdb_regex.h  |  6 ------
 4 files changed, 101 deletions(-)

diff --git a/gdb/config.in b/gdb/config.in
index 14a77c661d5..a4592ad5a90 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -722,9 +722,6 @@
 /* Define if <thread_db.h> has the TD_VERSION error code. */
 #undef THREAD_DB_HAS_TD_VERSION
 
-/* Define to 1 if the regex included in libiberty should be used. */
-#undef USE_INCLUDED_REGEX
-
 /* Enable extensions on AIX 3, Interix.  */
 #ifndef _ALL_SOURCE
 # undef _ALL_SOURCE
diff --git a/gdb/configure b/gdb/configure
index 4c80350596c..9b345d23f4e 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -911,7 +911,6 @@ enable_source_highlight
 with_intel_pt
 with_libipt_prefix
 with_libipt_type
-with_included_regex
 with_sysroot
 with_system_gdbinit
 with_system_gdbinit_dir
@@ -1661,10 +1660,6 @@ Optional Packages:
   --with-libipt-prefix[=DIR]  search for libipt in DIR/include and DIR/lib
   --without-libipt-prefix     don't search for libipt in includedir and libdir
   --with-libipt-type=TYPE     type of library to search for (auto/static/shared)
-  --without-included-regex
-                          don't use included regex; this is the default on
-                          systems with version 2 of the GNU C library (use
-                          with caution on other system)
   --with-sysroot[=DIR]    search for usr/lib et al within DIR
   --with-system-gdbinit=PATH
                           automatically load a system-wide gdbinit file
@@ -15891,60 +15886,6 @@ if test "$ac_cv_func_setpgrp_void" = yes; then
 fi
 fi
 
-# Assume we'll default to using the included libiberty regex.
-gdb_use_included_regex=yes
-
-# However, if the system regex is GNU regex, then default to *not*
-# using the included regex.
-{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for GNU regex" >&5
-$as_echo_n "checking for GNU regex... " >&6; }
-if ${gdb_cv_have_gnu_regex+:} false; then :
-  $as_echo_n "(cached) " >&6
-else
-  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
-/* end confdefs.h.  */
-#include <gnu-versions.h>
-int
-main ()
-{
-#define REGEX_INTERFACE_VERSION 1
-	 #if _GNU_REGEX_INTERFACE_VERSION != REGEX_INTERFACE_VERSION
-	 # error "Version mismatch"
-	 #endif
-
-  ;
-  return 0;
-}
-_ACEOF
-if ac_fn_c_try_compile "$LINENO"; then :
-  gdb_cv_have_gnu_regex=yes
-else
-  gdb_cv_have_gnu_regex=no
-
-fi
-rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
-
-fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gdb_cv_have_gnu_regex" >&5
-$as_echo "$gdb_cv_have_gnu_regex" >&6; }
-if test "$gdb_cv_have_gnu_regex" = yes; then
-  gdb_use_included_regex=no
-fi
-
-
-# Check whether --with-included-regex was given.
-if test "${with_included_regex+set}" = set; then :
-  withval=$with_included_regex; gdb_with_regex=$withval
-else
-  gdb_with_regex=$gdb_use_included_regex
-fi
-
-if test "$gdb_with_regex" = yes; then
-
-$as_echo "#define USE_INCLUDED_REGEX 1" >>confdefs.h
-
-fi
-
 # Check if <sys/proc.h> defines `struct thread' with a td_pcb member.
 ac_fn_c_check_member "$LINENO" "struct thread" "td_pcb" "ac_cv_member_struct_thread_td_pcb" "#include <sys/param.h>
 #include <sys/proc.h>
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 7035014484e..48f6ae7426f 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1363,39 +1363,6 @@ if test "$ac_cv_func_setpgrp_void" = yes; then
 fi
 fi
 
-# Assume we'll default to using the included libiberty regex.
-gdb_use_included_regex=yes
-
-# However, if the system regex is GNU regex, then default to *not*
-# using the included regex.
-AC_CACHE_CHECK(
-  [for GNU regex],
-  [gdb_cv_have_gnu_regex],
-  [AC_COMPILE_IFELSE(
-     [AC_LANG_PROGRAM(
-	[#include <gnu-versions.h>],
-	[#define REGEX_INTERFACE_VERSION 1
-	 #if _GNU_REGEX_INTERFACE_VERSION != REGEX_INTERFACE_VERSION
-	 # error "Version mismatch"
-	 #endif]
-      )],
-     [gdb_cv_have_gnu_regex=yes],
-     [gdb_cv_have_gnu_regex=no]
-   )]
-)
-if test "$gdb_cv_have_gnu_regex" = yes; then
-  gdb_use_included_regex=no
-fi
-
-AC_ARG_WITH(included-regex,
-  AS_HELP_STRING([--without-included-regex], [don't use included regex; this is the default on systems with version 2 of the GNU C library (use with caution on other system)]),
-  gdb_with_regex=$withval,
-  gdb_with_regex=$gdb_use_included_regex)
-if test "$gdb_with_regex" = yes; then
-  AC_DEFINE(USE_INCLUDED_REGEX, 1,
-    [Define to 1 if the regex included in libiberty should be used.])
-fi
-
 # Check if <sys/proc.h> defines `struct thread' with a td_pcb member.
 AC_CHECK_MEMBERS([struct thread.td_pcb], [], [],
 [#include <sys/param.h>
diff --git a/gdb/gdb_regex.h b/gdb/gdb_regex.h
index afe9909de25..0cac96e85c1 100644
--- a/gdb/gdb_regex.h
+++ b/gdb/gdb_regex.h
@@ -19,13 +19,7 @@
 #ifndef GDB_REGEX_H
 #define GDB_REGEX_H 1
 
-#ifdef USE_INCLUDED_REGEX
 # include "xregex.h"
-#else
-/* Request 4.2 BSD regex functions.  */
-# define _REGEX_RE_COMP
-# include <regex.h>
-#endif
 
 /* A compiled regex.  This is mainly a wrapper around regex_t.  The
    the constructor throws on regcomp error and the destructor is

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH][gdb/build] Hardcode --with-included-regex
  2021-04-13 15:34 [PATCH][gdb/build] Hardcode --with-included-regex Tom de Vries
@ 2021-04-19 19:47 ` Tom Tromey
  2021-04-19 20:33   ` tdevries
  0 siblings, 1 reply; 4+ messages in thread
From: Tom Tromey @ 2021-04-19 19:47 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches, Tom Tromey

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> A note on the latter: it's actually a bug to compile a regex using regcomp and
Tom> then pass it to re_search.  The GNU regex interface requires one to use
Tom> re_compile_pattern or re_compile_fastmap.  But as long we're using one of the
Tom> GNU regex incarnations in gnulib, glibc or libiberty, we get away with this.

Tom> The PR could be fixed by adding -lc in a specific position in the link line,
Tom> to force regcomp to be bound to glibc.  But this solution was considered
Tom> in the discussion in the PR as being brittle, and possibly causing problems
Tom> elsewhere.

Tom> Another solution offered was to restrict regex usage to posix, and no longer
Tom> use the GNU regex API.  This however could mean having to reproduce some of
Tom> that functionality locally, which would mean maintaining the same
Tom> functionality in more than one place.

Just to be clear, using the GNU extensions isn't currently done, I
think.  I'd like to use them so GDB can avoid copying some strings to
ensure they are \0-terminated; but this isn't done at the moment.

So, it would also be equivalent, right now, to switch GDB to conform to
POSIX here.

Personally I'm fine with just requiring the libiberty regexp.
That said we should probably see if there are differing opinions, or
some reason to keep the flag.

thanks,
Tom

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH][gdb/build] Hardcode --with-included-regex
  2021-04-19 19:47 ` Tom Tromey
@ 2021-04-19 20:33   ` tdevries
  2021-04-21 16:59     ` Tom Tromey
  0 siblings, 1 reply; 4+ messages in thread
From: tdevries @ 2021-04-19 20:33 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 2021-04-19 21:47, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> A note on the latter: it's actually a bug to compile a regex
> using regcomp and
> Tom> then pass it to re_search.  The GNU regex interface requires one 
> to use
> Tom> re_compile_pattern or re_compile_fastmap.  But as long we're
> using one of the
> Tom> GNU regex incarnations in gnulib, glibc or libiberty, we get away
> with this.
> 
> Tom> The PR could be fixed by adding -lc in a specific position in the
> link line,
> Tom> to force regcomp to be bound to glibc.  But this solution was 
> considered
> Tom> in the discussion in the PR as being brittle, and possibly causing 
> problems
> Tom> elsewhere.
> 
> Tom> Another solution offered was to restrict regex usage to posix,
> and no longer
> Tom> use the GNU regex API.  This however could mean having to 
> reproduce some of
> Tom> that functionality locally, which would mean maintaining the same
> Tom> functionality in more than one place.
> 
> Just to be clear, using the GNU extensions isn't currently done, I
> think.

In gdb_regex.{c,h}, we have compiled_regex::search, which uses GNU regex 
function re_search.  The functionality is actually used, removing it 
breaks the gdb build.

So I don't quite understand your remark.

Thanks,
- Tom

>  I'd like to use them so GDB can avoid copying some strings to
> ensure they are \0-terminated; but this isn't done at the moment.
> 
> So, it would also be equivalent, right now, to switch GDB to conform to
> POSIX here.
> 
> Personally I'm fine with just requiring the libiberty regexp.
> That said we should probably see if there are differing opinions, or
> some reason to keep the flag.
> 
> thanks,
> Tom

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH][gdb/build] Hardcode --with-included-regex
  2021-04-19 20:33   ` tdevries
@ 2021-04-21 16:59     ` Tom Tromey
  0 siblings, 0 replies; 4+ messages in thread
From: Tom Tromey @ 2021-04-21 16:59 UTC (permalink / raw)
  To: tdevries; +Cc: Tom Tromey, gdb-patches

>> In gdb_regex.{c,h}, we have compiled_regex::search, which uses GNU
>> regex function re_search.  The functionality is actually used,
>> removing it breaks the gdb build.

>> So I don't quite understand your remark.

No, it's me who didn't understand -- I missed this important detail.

I think your idea is correct: we should use the libiberty regex, and if
we really need to use the glibc one, we should import it via gnulib.
(Or maybe put this same logic into libiberty.)

I think your patch is ok.  Thank you.

Tom

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-04-21 16:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 15:34 [PATCH][gdb/build] Hardcode --with-included-regex Tom de Vries
2021-04-19 19:47 ` Tom Tromey
2021-04-19 20:33   ` tdevries
2021-04-21 16:59     ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).