This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: RFA: Try to include libunwind-ia64.h in libunwind-frame.h
On 02/21/2012 05:24 AM, Jan Kratochvil wrote:
> On Mon, 20 Feb 2012 23:00:53 +0100, Pedro Alves wrote:
>> Fix GDB's broken inclusion of "libunwind.h".
>> As I've explained before, including "libunwind.h" in GDB is _always_ wrong
>> for GDB.
>
> Not in the native mode.
It is broken. See Tristan's original post, or below.
> For example core files support on some platforms is
> also supported only natively now.
I don't understand the relevance of this comment. libunwind-$arch.h works
just the same for native core files. Except is doesn't result in a silently
broken gdb if the wrong "libunwind.h" happens to be on the include path.
>> I don't imagine how any multi-arch work we do to libunwind would make it
>> possible to include "libunwind.h".
>
> For native build of GDB - which is IMO 99% of its builds - with a oneliner
> patch of installing the unwinder it should work fully multi-arch.
>
> After your patch it will be slightly more complicated patch to make the
> libunwind support multi-arch (in native mode).
First, I don't understand what do you mean by multi-arch in native mode.
Multi-arch necessarily means non-native mode for at least one for
the archs in the multi set.
Second, that's not a convincing explanation at all. Please explain how the
code will be able to choose the correct libunwind.h header for each of the
archs in the multi-arch world, given that libunwind.h is a _native_ header:
$ cat libunwind.h:
...
#if defined __arm__
# include "libunwind-arm.h"
#elif defined __hppa__
# include "libunwind-hppa.h"
#elif defined __ia64__
# include "libunwind-ia64.h"
#elif defined __mips__
# include "libunwind-mips.h"
#elif defined __powerpc__ && !defined __powerpc64__
# include "libunwind-ppc32.h"
#elif defined __powerpc64__
# include "libunwind-ppc64.h"
#elif defined __i386__
# include "libunwind-x86.h"
#elif defined __x86_64__
# include "libunwind-x86_64.h"
#else
# error "Unsupported arch"
#endif
and so necessarily at least one of the archs will be built against the
wrong header.
Third, if we wanted to restrict to using libunwind only in the native case,
and we wanted to add a second arch using libunwind-frame, my patch does not
make it worse; it makes it clearer what is necessary. The easier path would
be to teach configure.tgt/configure.ac to define a
global -DGDB_LIBUNWIND_HEADER=libunwind-$arch.h that libunwind-frame.h|c
would include instead. _And_, we'd need some way for the 'non --target'
arch to not use libunwind (as it would use the wrong libunwind).
Forth, nothing got worse/harder to build a native compiler. Both
libunwind.h and libunwind-$arch.h are always installed. So including one
or the other is the same:
$ ls /usr/include/libunwind*
/usr/include/libunwind-common.h /usr/include/libunwind-dynamic.h /usr/include/libunwind.h /usr/include/libunwind-ptrace.h /usr/include/libunwind-x86_64.h
But if we pick the right one (the $arch variant) even on native builds,
then e.g., a x64_64 x ia64 cross build will no longer work incorrectly. A cross
build _does_ build incorrectly today because gdb will silently build
against / load the x84_64 libunwind! That is what Tristan's original
is all about.
>> But in any case, this is much more than the real need we have now. And I
>> don't see why we can't fix the include problem, and do multi-arching as
>> follow up work as necessary.
> +
>> I don't see how different the result will be from today's state,
>
> Currently the code has some attempt to be multi-arch, despite not yet there.
> Let's say it is in 50% of the multi-arch libunwind supports.
>
> After this limitation to ia64 it will be at 30% of the multi-arch libunwind
> support.
IMO, that's bogus. Making the code multi-arch necessarily means _not including
libunwind.h anywhere, so it makes the code 70% multiarch (more than 50%, hey
I can make up stats too! :-) ).
>
> We have already spent more time talking about it than to either making it
> ia64-exclusive making the support 0% or to making the support fully multi-arch
> being 100% where nobody needs to talk about it anymore.
We have already spent more time talking about it than committing the patch
below, which fixes the bug, and doesn't make any multi-arch work any harder.
> The code in GDB which no longer makes sense but still is neither removed nor
> fixed makes it difficult for any contributions, coding, reviewing, anything,
> besides a few people who know what it should mean one day in the future.
That's an exaggeration fallacy. I have added comments to prevent people from
getting confused.
Besides, you can't make libunwind-frame.c ia64-exclusive (whatever that means,
though I assume basically file and function renaming) without fixing the
libunwind.h header problem.
2012-02-21 Tristan Gingold <gingold@adacore.com>
Pedro Alves <palves@redhat.com>
* ia64-tdep.c: Do not include libunwind-ia64.h.
* libunwind-frame.h: Remove #ifdef HAVE_LIBUNWIND_H guard.
Include libunwind-ia64.h instead of libunwind.h.
* configure.ac (--with-libunwind, $enable_libunwind): Don't check
for libunwind.h existence.
* configure, config.in: Regenerate.
---
gdb/config.in | 3 ---
gdb/configure | 22 +++++++++-------------
gdb/configure.ac | 6 +++---
gdb/ia64-tdep.c | 1 -
gdb/libunwind-frame.c | 8 ++++++++
gdb/libunwind-frame.h | 12 +++++++-----
6 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/gdb/config.in b/gdb/config.in
index bae1763..194cc7d 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -242,9 +242,6 @@
/* Define if libunwind library is being used. */
#undef HAVE_LIBUNWIND
-/* Define to 1 if you have the <libunwind.h> header file. */
-#undef HAVE_LIBUNWIND_H
-
/* Define to 1 if you have the <libunwind-ia64.h> header file. */
#undef HAVE_LIBUNWIND_IA64_H
diff --git a/gdb/configure b/gdb/configure
index 2566410..9cb3742 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -8252,21 +8252,19 @@ if test "${with_libunwind+set}" = set; then :
esac
else
- for ac_header in libunwind.h libunwind-ia64.h
+ for ac_header in libunwind-ia64.h
do :
- as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
-ac_fn_c_check_header_mongrel "$LINENO" "$ac_header" "$as_ac_Header" "$ac_includes_default"
-eval as_val=\$$as_ac_Header
- if test "x$as_val" = x""yes; then :
+ ac_fn_c_check_header_mongrel "$LINENO" "libunwind-ia64.h" "ac_cv_header_libunwind_ia64_h" "$ac_includes_default"
+if test "x$ac_cv_header_libunwind_ia64_h" = x""yes; then :
cat >>confdefs.h <<_ACEOF
-#define `$as_echo "HAVE_$ac_header" | $as_tr_cpp` 1
+#define HAVE_LIBUNWIND_IA64_H 1
_ACEOF
fi
done
- if test x"$ac_cv_header_libunwind_h" = xyes -a x"$ac_cv_header_libunwind_ia64_h" = xyes; then
+ if test x"$ac_cv_header_libunwind_ia64_h" = xyes; then
enable_libunwind=yes;
fi
@@ -8274,14 +8272,12 @@ fi
if test x"$enable_libunwind" = xyes; then
- for ac_header in libunwind.h libunwind-ia64.h
+ for ac_header in libunwind-ia64.h
do :
- as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`
-ac_fn_c_check_header_mongrel "$LINENO" "$ac_header" "$as_ac_Header" "$ac_includes_default"
-eval as_val=\$$as_ac_Header
- if test "x$as_val" = x""yes; then :
+ ac_fn_c_check_header_mongrel "$LINENO" "libunwind-ia64.h" "ac_cv_header_libunwind_ia64_h" "$ac_includes_default"
+if test "x$ac_cv_header_libunwind_ia64_h" = x""yes; then :
cat >>confdefs.h <<_ACEOF
-#define `$as_echo "HAVE_$ac_header" | $as_tr_cpp` 1
+#define HAVE_LIBUNWIND_IA64_H 1
_ACEOF
fi
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 1b11adb..c4c84a0 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -348,14 +348,14 @@ AS_HELP_STRING([--with-libunwind], [use libunwind frame unwinding support]),
no) enable_libunwind=no ;;
*) AC_MSG_ERROR(bad value ${withval} for GDB with-libunwind option) ;;
esac],[
- AC_CHECK_HEADERS(libunwind.h libunwind-ia64.h)
- if test x"$ac_cv_header_libunwind_h" = xyes -a x"$ac_cv_header_libunwind_ia64_h" = xyes; then
+ AC_CHECK_HEADERS(libunwind-ia64.h)
+ if test x"$ac_cv_header_libunwind_ia64_h" = xyes; then
enable_libunwind=yes;
fi
])
if test x"$enable_libunwind" = xyes; then
- AC_CHECK_HEADERS(libunwind.h libunwind-ia64.h)
+ AC_CHECK_HEADERS(libunwind-ia64.h)
AC_DEFINE(HAVE_LIBUNWIND, 1, [Define if libunwind library is being used.])
CONFIG_OBS="$CONFIG_OBS libunwind-frame.o"
CONFIG_DEPS="$CONFIG_DEPS libunwind-frame.o"
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index a297ecc..a36dc22 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -43,7 +43,6 @@
#ifdef HAVE_LIBUNWIND_IA64_H
#include "elf/ia64.h" /* for PT_IA_64_UNWIND value */
#include "libunwind-frame.h"
-#include "libunwind-ia64.h"
/* Note: KERNEL_START is supposed to be an address which is not going
to ever contain any valid unwind info. For ia64 linux, the choice
diff --git a/gdb/libunwind-frame.c b/gdb/libunwind-frame.c
index 893fe1e..f8f5289 100644
--- a/gdb/libunwind-frame.c
+++ b/gdb/libunwind-frame.c
@@ -40,6 +40,14 @@
#include "complaints.h"
+/* IA-64 is the only target that currently uses libunwind-frame. Note
+ how UNW_TARGET, UNW_OBJ, etc. are compile time constants below.
+ Those come from libunwind's headers, and are target dependent.
+ Also, some of libunwind's typedefs are target dependent, as e.g.,
+ unw_word_t. If some other target wants to use this, we will need
+ to do some abstracting in order to make it possible to select which
+ libunwind we're talking to at runtime (and have one per arch). */
+
/* The following two macros are normally defined in <endian.h>.
But systems such as ia64-hpux do not provide such header, so
we just define them here if not already defined. */
diff --git a/gdb/libunwind-frame.h b/gdb/libunwind-frame.h
index 0251819..ef98177 100644
--- a/gdb/libunwind-frame.h
+++ b/gdb/libunwind-frame.h
@@ -19,8 +19,6 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
-#ifdef HAVE_LIBUNWIND_H
-
struct frame_info;
struct frame_id;
struct regcache;
@@ -29,7 +27,13 @@ struct gdbarch;
#ifndef LIBUNWIND_FRAME_H
#define LIBUNWIND_FRAME_H 1
-#include "libunwind.h"
+/* IA-64 is the only target that currently uses libunwind. If some
+ other target wants to use it, we will need to do some abstracting
+ in order to make it possible to have more than one libunwind-frame
+ instance. Including "libunwind.h" is wrong as that ends up
+ including the libunwind-$(arch).h for the host gdb is running
+ on. */
+#include "libunwind-ia64.h"
struct libunwind_descr
{
@@ -72,5 +76,3 @@ int libunwind_get_reg_special (struct gdbarch *gdbarch,
int regnum, void *buf);
#endif /* libunwind-frame.h */
-
-#endif /* HAVE_LIBUNWIND_H */