Skip to content

8295795: hsdis does not build with binutils 2.39+ #15138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
+83 −36
Closed

8295795: hsdis does not build with binutils 2.39+ #15138

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2d23bab
Binutils fix
robehn Jun 21, 2023
a8ce2d3
Added parameter name
robehn Aug 3, 2023
b3c81f8
Reverted bad change
robehn Aug 7, 2023
f305e56
Check API during conf
robehn Sep 29, 2023
b514c33
Minor fixes
robehn Sep 29, 2023
57d5a24
Merge branch 'master' into binutils-working
robehn Oct 19, 2023
36226f8
Fix installed binutils
robehn Oct 19, 2023
ae9624c
Fixed include
robehn Oct 19, 2023
b717547
Merge branch 'master' into binutils-working
robehn Oct 19, 2023
ac75ae2
Added back guard
robehn Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 55 additions & 32 deletions make/autoconf/lib-hsdis.m4
View file
Original file line number Diff line number Diff line change
@@ -134,7 +134,8 @@ AC_DEFUN([LIB_BUILD_BINUTILS],
BINUTILS_SRC="$with_binutils_src"
UTIL_FIXUP_PATH(BINUTILS_SRC)

BINUTILS_DIR="$CONFIGURESUPPORT_OUTPUTDIR/binutils"
BINUTILS_BUILD_DIR="$CONFIGURESUPPORT_OUTPUTDIR/binutils"
BINUTILS_INSTALL_DIR="$CONFIGURESUPPORT_OUTPUTDIR/binutils-install"

if ! test -d $BINUTILS_SRC; then
AC_MSG_ERROR([--with-binutils-src is not pointing to a directory])
@@ -143,15 +144,15 @@ AC_DEFUN([LIB_BUILD_BINUTILS],
AC_MSG_ERROR([--with-binutils-src does not look like a binutils source directory])
fi

if ! test -d $BINUTILS_DIR; then
$MKDIR -p $BINUTILS_DIR
if ! test -d $BINUTILS_BUILD_DIR; then
$MKDIR -p $BINUTILS_BUILD_DIR
fi

if test -e $BINUTILS_DIR/bfd/libbfd.a && \
test -e $BINUTILS_DIR/opcodes/libopcodes.a && \
test -e $BINUTILS_DIR/libiberty/libiberty.a && \
test -e $BINUTILS_DIR/zlib/libz.a; then
AC_MSG_NOTICE([Found binutils binaries in binutils source directory -- not building])
# We don't know the version, not checking for libsframe.a
if test -e $BINUTILS_INSTALL_DIR/lib/libbfd.a && \
test -e $BINUTILS_INSTALL_DIR/lib/libopcodes.a && \
test -e $BINUTILS_INSTALL_DIR/lib/libiberty.a; then
AC_MSG_NOTICE([Found binutils binaries in binutils install directory -- not building])
else
# On Windows, we cannot build with the normal Microsoft CL, but must instead use
# a separate mingw toolchain.
@@ -190,20 +191,26 @@ AC_DEFUN([LIB_BUILD_BINUTILS],
binutils_cflags="$binutils_cflags $MACHINE_FLAG $JVM_PICFLAG $C_O_FLAG_NORM"

AC_MSG_NOTICE([Running binutils configure])
AC_MSG_NOTICE([configure command line: cd $BINUTILS_DIR && $BINUTILS_SRC/configure --disable-nls CFLAGS="$binutils_cflags" CC="$binutils_cc" AR="$AR" $binutils_target])
AC_MSG_NOTICE([configure command line: cd $BINUTILS_BUILD_DIR && $BINUTILS_SRC/configure --disable-werror --prefix=$BINUTILS_INSTALL_DIR --enable-install-libiberty --with-system-zlib --without-zstd --disable-nls CFLAGS="$binutils_cflags" CC="$binutils_cc" AR="$AR" $binutils_target])
saved_dir=`pwd`
cd "$BINUTILS_DIR"
$BINUTILS_SRC/configure --disable-nls CFLAGS="$binutils_cflags" CC="$binutils_cc" AR="$AR" $binutils_target
if test $? -ne 0 || ! test -e $BINUTILS_DIR/Makefile; then
cd "$BINUTILS_BUILD_DIR"
$BINUTILS_SRC/configure --disable-werror --prefix=$BINUTILS_INSTALL_DIR --enable-install-libiberty --with-system-zlib --without-zstd --disable-nls CFLAGS="$binutils_cflags" CC="$binutils_cc" AR="$AR" $binutils_target
if test $? -ne 0 || ! test -e $BINUTILS_BUILD_DIR/Makefile; then
AC_MSG_NOTICE([Automatic building of binutils failed on configure. Try building it manually])
AC_MSG_ERROR([Cannot continue])
fi
AC_MSG_NOTICE([Running binutils make])
$MAKE all-opcodes
$MAKE all-opcodes all-libiberty
if test $? -ne 0; then
AC_MSG_NOTICE([Automatic building of binutils failed on make. Try building it manually])
AC_MSG_ERROR([Cannot continue])
fi
AC_MSG_NOTICE([Running binutils make install])
$MAKE install-opcodes install-libiberty
if test $? -ne 0; then
AC_MSG_NOTICE([Automatic building, install step, of binutils failed on make. Try building it manually])
AC_MSG_ERROR([Cannot continue])
fi
cd $saved_dir
AC_MSG_NOTICE([Building of binutils done])
fi
@@ -223,41 +230,57 @@ AC_DEFUN([LIB_SETUP_HSDIS_BINUTILS],

# We need the binutils static libs and includes.
if test "x$with_binutils_src" != x; then
# Try building the source first. If it succeeds, it sets $BINUTILS_DIR.
# Try building the source first. If it succeeds, it sets $BINUTILS_INSTALL_DIR.
LIB_BUILD_BINUTILS
fi

if test "x$with_binutils" != x; then
BINUTILS_DIR="$with_binutils"
BINUTILS_INSTALL_DIR="$with_binutils"
fi

binutils_system_error=""
HSDIS_LDFLAGS=""
HSDIS_LIBS=""
if test "x$BINUTILS_DIR" = xsystem; then
disasm_header="<dis-asm.h>"

if test "x$BINUTILS_INSTALL_DIR" = xsystem; then
AC_CHECK_LIB(bfd, bfd_openr, [ HSDIS_LIBS="-lbfd" ], [ binutils_system_error="libbfd not found" ])
AC_CHECK_LIB(opcodes, disassembler, [ HSDIS_LIBS="$HSDIS_LIBS -lopcodes" ], [ binutils_system_error="libopcodes not found" ])
AC_CHECK_LIB(z, deflate, [ HSDIS_LIBS="$HSDIS_LIBS -lz" ], [ binutils_system_error="libz not found" ])
# libiberty is not required on Ubuntu
AC_CHECK_LIB(iberty, xmalloc, [ HSDIS_LIBS="$HSDIS_LIBS -liberty" ])
AC_CHECK_LIB(z, deflate, [ HSDIS_LIBS="$HSDIS_LIBS -lz" ], [ binutils_system_error="libz not found" ])
AC_CHECK_LIB(sframe, frame, [ HSDIS_LIBS="$HSDIS_LIBS -lsframe" ], )
HSDIS_CFLAGS="-DLIBARCH_$OPENJDK_TARGET_CPU_LEGACY_LIB"
elif test "x$BINUTILS_DIR" != x; then
if test -e $BINUTILS_DIR/bfd/libbfd.a && \
test -e $BINUTILS_DIR/opcodes/libopcodes.a && \
test -e $BINUTILS_DIR/libiberty/libiberty.a && \
test -e $BINUTILS_DIR/zlib/libz.a; then
HSDIS_CFLAGS="-DLIBARCH_$OPENJDK_TARGET_CPU_LEGACY_LIB"
if test -n "$BINUTILS_SRC"; then
HSDIS_CFLAGS="$HSDIS_CFLAGS -I$BINUTILS_SRC/include -I$BINUTILS_DIR/bfd"
else
HSDIS_CFLAGS="$HSDIS_CFLAGS -I$BINUTILS_DIR/include -I$BINUTILS_DIR/bfd"
elif test "x$BINUTILS_INSTALL_DIR" != x; then
disasm_header="\"$BINUTILS_INSTALL_DIR/include/dis-asm.h\""
if test -e $BINUTILS_INSTALL_DIR/lib/libbfd.a && \
test -e $BINUTILS_INSTALL_DIR/lib/libopcodes.a && \
test -e $BINUTILS_INSTALL_DIR/lib/libiberty.a; then
HSDIS_CFLAGS="-DLIBARCH_$OPENJDK_TARGET_CPU_LEGACY_LIB -I$BINUTILS_INSTALL_DIR/include"
HSDIS_LIBS="$BINUTILS_INSTALL_DIR/lib/libbfd.a $BINUTILS_INSTALL_DIR/lib/libopcodes.a $BINUTILS_INSTALL_DIR/lib/libiberty.a"
# If we have libsframe add it.
if test -e $BINUTILS_INSTALL_DIR/lib/libsframe.a; then
HSDIS_LIBS="$HSDIS_LIBS $BINUTILS_INSTALL_DIR/lib/libsframe.a"
fi
HSDIS_LDFLAGS=""
HSDIS_LIBS="$BINUTILS_DIR/bfd/libbfd.a $BINUTILS_DIR/opcodes/libopcodes.a $BINUTILS_DIR/libiberty/libiberty.a $BINUTILS_DIR/zlib/libz.a"
AC_CHECK_LIB(z, deflate, [ HSDIS_LIBS="$HSDIS_LIBS -lz" ], AC_MSG_ERROR([libz not found]))
else
AC_MSG_ERROR(["$BINUTILS_INSTALL_DIR/libs/ must contain libbfd.a, libopcodes.a, libiberty.a"])
fi
fi

AC_MSG_CHECKING([Checking binutils API])
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include $disasm_header],[[void foo() {init_disassemble_info(0, 0, 0, 0);}]])],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me this is not working as expected but I don't have an env to verify this.

I've been working on something similar to deal with capstone API changes and the pattern here seemed to not compile but not for the reasons expected.

For example, I created:

AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include $capstone_header],[[void foo() {cs_arch test = CS_ARCH_AARCH64;}]])],

And when I looked the compilation error it showed:

conftest.c:27:12: error: function definition is not allowed here
void foo() {cs_arch test = CS_ARCH_AARCH64;}
           ^
1 error generated.
configure:142919: $? = 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME "OpenJDK"
| #define PACKAGE_TARNAME "openjdk"
| #define PACKAGE_VERSION "openjdk"
| #define PACKAGE_STRING "OpenJDK openjdk"
| #define PACKAGE_BUGREPORT "build-dev@openjdk.org"
| #define PACKAGE_URL "https://openjdk.org"
| #define HAVE_STDIO_H 1
| #define HAVE_STDLIB_H 1
| #define HAVE_STRING_H 1
| #define HAVE_INTTYPES_H 1
| #define HAVE_STDINT_H 1
| #define HAVE_STRINGS_H 1
| #define HAVE_SYS_STAT_H 1
| #define HAVE_SYS_TYPES_H 1
| #define HAVE_UNISTD_H 1
| #define STDC_HEADERS 1
| #define HAVE_STDIO_H 1
| #define SIZEOF_INT_P 8
| #define HAVE_CUPS_CUPS_H 1
| #define HAVE_CUPS_PPD_H 1
| /* end confdefs.h.  */
| #include "/Users/galder/opt/capstone/include/capstone/capstone.h"
| int
| main (void)
| {
| void foo() {cs_arch test = CS_ARCH_AARCH64;}
|   ;
|   return 0;
| }

So, the code was not compiling not because of the wrong cs_arch value but because you cannot define a method within main.

Instead I made my AC_COMPILE_IFELSE looks like this:

AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include $capstone_header],[[cs_arch test = CS_ARCH_AARCH64;]])],

And that works as expected. For example, when using a capstone library that supports CS_ARCH_AARCH64, it does:

configure:142754: result: 'capstone'
configure:142767: checking for capstone
configure:142770: result: /Users/galder/opt/capstone
configure:142906: checking capstone aarch64 arch name
configure:142919: /usr/bin/clang -c  -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks conftest.c >&5
configure:142919: $? = 0
configure:142922: result: AARCH64

And when the capstone library does not have CS_ARCH_AARCH64, the compilation error I get is the expected one:

conftest.c:27:16: error: use of undeclared identifier 'CS_ARCH_AARCH64'; did you mean 'CS_ARCH_ARM64'?
cs_arch test = CS_ARCH_AARCH64;
               ^~~~~~~~~~~~~~~
               CS_ARCH_ARM64
/Users/galder/opt/capstone-5/include/capstone/capstone.h:76:2: note: 'CS_ARCH_ARM64' declared here
        CS_ARCH_ARM64,          ///< ARM-64, also called AArch64
        ^
1 error generated.
configure:142919: $? = 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME "OpenJDK"
| #define PACKAGE_TARNAME "openjdk"
| #define PACKAGE_VERSION "openjdk"
| #define PACKAGE_STRING "OpenJDK openjdk"
| #define PACKAGE_BUGREPORT "build-dev@openjdk.org"
| #define PACKAGE_URL "https://openjdk.org"
| #define HAVE_STDIO_H 1
| #define HAVE_STDLIB_H 1
| #define HAVE_STRING_H 1
| #define HAVE_INTTYPES_H 1
| #define HAVE_STDINT_H 1
| #define HAVE_STRINGS_H 1
| #define HAVE_SYS_STAT_H 1
| #define HAVE_SYS_TYPES_H 1
| #define HAVE_UNISTD_H 1
| #define STDC_HEADERS 1
| #define HAVE_STDIO_H 1
| #define SIZEOF_INT_P 8
| #define HAVE_CUPS_CUPS_H 1
| #define HAVE_CUPS_PPD_H 1
| /* end confdefs.h.  */
| #include "/Users/galder/opt/capstone-5/include/capstone/capstone.h"
| int
| main (void)
| {
| cs_arch test = CS_ARCH_AARCH64;
|   ;
|   return 0;
| }

/cc @theRealAph

Copy link
Contributor

@galderz galderz Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it clear, seems to me this line should be this instead:

AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include $disasm_header],[[init_disassemble_info(0, 0, 0, 0);]])],

You could probably remove the ; since it's already included.

Copy link
Contributor Author

@robehn robehn Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct, this was not my intention.
For some reason I mixed up AC_LANG_SOURCE and AC_LANG_PROGRAM.

But nested function are fine, so there is actually no issue with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But nested function are fine, so there is actually no issue with it.

Is it? The compilation error above says "error: function definition is not allowed here". It seems to me is saying that the compilation fails because of the nested function. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why.

[rehn@rehn-xps ~]$ cat m.c
int main() {
  void foo() {
    return;
  };
  foo();
  return 0;
}
[rehn@rehn-xps ~]$ gcc -Wall -Wextra -std=c89 m.c 
[rehn@rehn-xps ~]$

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robehn I think that is a gcc extension; not ANSI C.

Copy link
Contributor Author

@robehn robehn Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, you are correct. Apparently the gcc extension is not turned off even if you use -ansi.
clang on the other hand complains.

@galderz your suggested patch is correct, can you open PR ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robehn Way ahead of you: https://bugs.openjdk.org/browse/JDK-8320663 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @magicus !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was fast! Thx @magicus and thanks @robehn for verifying things!

[
AC_MSG_RESULT([New API])
HSDIS_CFLAGS="$HSDIS_CFLAGS -DBINUTILS_NEW_API"
],
[
AC_MSG_RESULT([Old API])
]
)

AC_MSG_CHECKING([for binutils to use with hsdis])
case "x$BINUTILS_DIR" in
case "x$BINUTILS_INSTALL_DIR" in
xsystem)
if test "x$OPENJDK_TARGET_OS" != xlinux; then
AC_MSG_RESULT([invalid])
@@ -280,10 +303,10 @@ AC_DEFUN([LIB_SETUP_HSDIS_BINUTILS],
;;
*)
if test "x$HSDIS_LIBS" != x; then
AC_MSG_RESULT([$BINUTILS_DIR])
AC_MSG_RESULT([$BINUTILS_INSTALL_DIR])
else
AC_MSG_RESULT([invalid])
AC_MSG_ERROR([$BINUTILS_DIR does not contain a proper binutils installation])
AC_MSG_ERROR([$BINUTILS_INSTALL_DIR does not contain a proper binutils installation])
fi
;;
esac
32 changes: 28 additions & 4 deletions src/utils/hsdis/binutils/hsdis-binutils.c
View file
Original file line number Diff line number Diff line change
@@ -49,14 +49,16 @@
HotSpot PrintAssembly option.
*/

#ifndef SYSTEM_BINUTILS
#include <config.h> /* required by bfd.h */
#endif

#include <errno.h>
#include <inttypes.h>
#include <string.h>

#ifndef SYSTEM_BINUTILS
/* defines for bfd.h */
#define PACKAGE "hsdis"
#define PACKAGE_VERSION 1
#endif

#include <bfd.h>
#include <dis-asm.h>

@@ -555,12 +557,34 @@ static void parse_fake_insn(disassembler_ftype dfn,
dinfo->fprintf_func = fprintf_func;
}

static fprintf_ftype target_fprintf_func = NULL;

#ifdef BINUTILS_NEW_API
static int wrapper_fprintf_styled_ftype(void *v, enum disassembler_style style_unused, const char* fmt, ...) {
char buffer[1024] = {};
va_list args;
int r;
va_start(args, fmt);
r = vsnprintf(buffer, sizeof(buffer), fmt, args);
va_end(args);
if (target_fprintf_func != NULL) {
return target_fprintf_func(v, "%s", buffer);
}
return r;
}
#endif

static void init_disassemble_info_from_bfd(struct disassemble_info* dinfo,
void *stream,
fprintf_ftype fprintf_func,
bfd* abfd,
char* disassembler_options) {
target_fprintf_func = fprintf_func;
#ifdef BINUTILS_NEW_API
init_disassemble_info(dinfo, stream, fprintf_func, wrapper_fprintf_styled_ftype);
#else
init_disassemble_info(dinfo, stream, fprintf_func);
#endif

dinfo->flavour = bfd_get_flavour(abfd);
dinfo->arch = bfd_get_arch(abfd);