Bug 17077 - --with-system-readline uses bundled readline include files
Summary: --with-system-readline uses bundled readline include files
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: build (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 8.3
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-20 10:39 UTC by Jan Kratochvil
Modified: 2019-10-23 21:39 UTC (History)
4 users (show)

See Also:
Host:
Target: x86_64-linux-gnu
Build:
Last reconfirmed:


Attachments
Include the bundled readline header files, only when the bundled readline is built. (2.76 KB, patch)
2015-07-13 07:23 UTC, dilyan.palauzov@aegee.org
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Kratochvil 2014-06-20 10:39:44 UTC
When building tui/tui-io.c one can see in gcc -E still the bundled readline include files get used.

# 32 "./../opcodes/../readline/rltypedefs.h"
#define _FUNCTION_DEF

As the libreadline.a is linked from system it may lead to incompatible ABI.

I do not have an idea how to fix it.  Fedora GDB packaging ensures the build is correct by "rm -rf readline" in the source tree.
Comment 1 Samuel Bronson 2014-06-20 18:55:36 UTC
I've also noticed that --with-system-readline doesn't prevent *building* readline.

--disable-readline does, but unfortunately the headers still get used :-(.

But, uh, what's the point of using --with-system-readline if you're linking it statically, anyway?
Comment 2 Jan Kratochvil 2014-06-20 21:11:35 UTC
(In reply to Samuel Bronson from comment #1)
> But, uh, what's the point of using --with-system-readline if you're linking
> it statically, anyway?

Fedora links libreadline dynamically.  Comment 0 says libreadline.a just for simplicity, I did not mean specifically the .a variant, .so is also OK.
Comment 3 dilyan.palauzov@aegee.org 2015-07-07 10:58:31 UTC
What about removing readline/ from the gdb tarball/version tracking and insisting of having readline on the system, failing at ./configure time if this is not the case.

The ones who can compile gdb, shall also be able to compile and install libreadline.
Comment 4 Jan Kratochvil 2015-07-07 11:41:05 UTC
That would not work as GDB needs patched readline.

Fortunately most of the patches are for non-Linux systems.  But even for Linux Fedora needs to carry one:
  http://pkgs.fedoraproject.org/cgit/readline.git/tree/readline-6.2-gdb.patch

Maybe the required patches are all gone now with readline-6.3 but GDB sources still bundle readline-6.2:
  [PATCH] [RFC] Sync readline to version 6.3 patchlevel 8
  https://sourceware.org/ml/gdb-patches/2015-05/msg00352.html
  https://sourceware.org/ml/gdb-patches/2015-06/msg00381.html
Comment 5 Jan Kratochvil 2015-07-07 11:43:49 UTC
(In reply to dilyan.palauzov@aegee.org from comment #3)
> The ones who can compile gdb, shall also be able to compile and install
> libreadline.

Additionally IIRC there was a resistance against any mandatory external dependencies for the sourceware sourcetree.  The reasoning IIRC was that it is inconvenient for cross-arch bootstraps but GDB does not belong to the bootstraping compiler chain.  So IMO it is wrong but it is so.
Comment 6 dilyan.palauzov@aegee.org 2015-07-13 07:23:45 UTC
Created attachment 8429 [details]
Include the bundled readline header files, only when the bundled readline is built.

The provided patch:

  -- makes use of the fact, that the preprocessor can differentiate between #include "file" and #include <file>, when proceeding paths;

  -- substitutes all occurrences of #include "readline/{readline,history,tilde}.h" with #include <readline/{readline,history,tilde}.h>;

  -- enables the project root directory for includes (from opcode), unconditionally only for #include "file", and for all includes ("",<>), when the bundled readline is built (using -iquote in place of -I);

  -- does not approach the problem for sim/erc32/sis.c.
Comment 7 Jan Kratochvil 2015-07-13 07:49:34 UTC
I haven't tested it but -iquote seems to be a fix for this Bug.  Patches should be sent to gdb-patches ml.
Comment 8 dilyan.palauzov@aegee.org 2015-07-28 08:49:08 UTC
While -iquote is supported in gcc, clang and Intel's C++, HP aC++ does not support it.  

Another approach would be to use -I- instead, which is supported by HP aC++ and Oracle's Solaris Studio, but IBM's XL C++ compiler does not support it.  For the latter is stated explicitly, that gxlc, a utility that accepts GNU C or C++ compiler options and translates them into comparable XL C/C++ options, does not understand -I- and therefore generates a warning.

To sum up, either the -I- option is applied, until somebody complains that a compiler is not working, or the bintutils-gdb/readline directory is moved to binutils-gdb/rl/readline (or alike) and -Irl is comnditionally added to CPPFLAGS.
Comment 9 Tom Tromey 2018-09-26 13:01:09 UTC
I sent a patch for this.
Comment 10 Sourceware Commits 2018-10-07 04:47:40 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a8a5dbcab8df0b3a9e04745d4fe8d64740acb323

commit a8a5dbcab8df0b3a9e04745d4fe8d64740acb323
Author: Tom Tromey <tom@tromey.com>
Date:   Wed Sep 26 06:54:17 2018 -0600

    Do not accidentally include in-tree readline headers
    
    PR build/17077 points out that when --with-system-readline is given,
    gdb will still pick up the in-tree readline headers.  Normally this is
    not a big problem, because readline is very stable and so the ABI does
    not change much; but it is clearly a bug to do this, and could bite at
    some point.
    
    The basic problem is that OPCODES_CFLAGS uses -I$(OPCODES_SRC)/..  so
    that #include "opcodes/..." works.  However, this also makes it so the
    
    This patch fixes the problem in a mildly hacky way: remove the
    offending -I option, and change gdb to use #include "../opcodes/..."
    instead.  This continues to make it clear where the header comes from,
    without allowing incorrect behavior.
    
    Tested by rebuilding and then looking at the *.Po files.
    
    gdb/ChangeLog
    2018-10-06  Tom Tromey  <tom@tromey.com>
    
    	PR build/17077:
    	* Makefile.in (OPCODES_CFLAGS): Remove "-I$(OPCODES_SRC)/..".
    	* arc-tdep.c, frv-tdep.c, lm32-tdep.c, mep-tdep.c,
    	microblaze-tdep.c, or1k-tdep.h: Use ../opcodes, not opcodes, in
    	#include.
Comment 11 Tom Tromey 2018-10-07 05:32:48 UTC
Fixed.
Comment 12 Sourceware Commits 2019-10-23 21:39:56 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6999161a2a3b3cbd918570e094199184331d4f81

commit 6999161a2a3b3cbd918570e094199184331d4f81
Author: Tom Tromey <tom@tromey.com>
Date:   Sat Oct 5 16:39:44 2019 -0600

    Move readline to the readline/readline subdirectory
    
    readline turns out to be a bit of a stumbling block for the project to
    move gdbsupport (and then gdbserver) to the top-level.
    
    The issue is that readline headers are intended to be included with
    names like "readline/readline.h".  To support this, gdb effectively
    adds a -I option pointing to the top-level source directory -- but,
    importantly, this option is not used when the system readline is used.
    
    For gdbsupport, a -I option like this would always be needed, but that
    in turn would break the system readline case.  This was PR build/17077,
    fixed in commit a8a5dbcab8df0b3a9e04745d4fe8d64740acb323.
    
    Previously, we had discussed this on the gdb-patches list in terms of
    removing readline from the tree
    
        https://sourceware.org/ml/gdb-patches/2019-09/msg00317.html
    
    However, Eli expressed some concerns, and Joel did as well (off-list).
    
    Given those concerns, and the fact that a patch-free local readline is
    relatively new in gdb (it was locally patched for years), I changed my
    mind and decided to handle this situation by moving the readline
    sources down a level.
    
    That is, upstream readline is now in readline/readline, and the
    top-level readline directory just contains the minimal configury
    needed to build that.
    
    This fixes the problem because, when gdb unconditionally adds a
    -I$(top_srcdir), this will not find readline headers.  A separate -I
    will be needed instead, which is exactly what's needed for
    --with-system-readline.
    
    gdb/ChangeLog
    2019-10-23  Tom Tromey  <tom@tromey.com>
    
    	* Makefile.in (READLINE_DIR): Update.
    
    gdb/doc/ChangeLog
    2019-10-23  Tom Tromey  <tom@tromey.com>
    
    	* Makefile.in (READLINE_DIR): Update.
    
    readline/ChangeLog
    2019-10-23  Tom Tromey  <tom@tromey.com>
    
    	Move old contents to readline/ subdirectory.
    	* aclocal.m4, configure, configure.ac, .gitignore, Makefile.am,
    	Makefile.in, README: New files.
    
    Change-Id: Ice156a2ee09ea68722b48f64d97146d7428ea9e4