Hello Thanks for looking into this Here is updated webrev. http://cr.openjdk.java.net/~vkempik/8248495/webrev.03/
Regards, Vladimir. > 1 июля 2020 г., в 21:59, Erik Joelsson <erik.joels...@oracle.com> написал(а): > > I think this looks ok, but would like Magnus' input as he is already involved > in this review. > > /Erik > > On 2020-07-01 02:45, Vladimir Kempik wrote: >> Hello >> >> Please take a look at updated webrev - >> http://cr.openjdk.java.net/~vkempik/8248495/webrev.02/ >> <http://cr.openjdk.java.net/~vkempik/8248495/webrev.02/> >> >> I decided to use AC_CHECK_HEADERS instead AC_CHECK_FILE as it doesn’t work >> in cross-compilation scenario. >> >> libffi binary is located in absolutely standard location, so -lffi was >> enough. >> >> Thanks, Vladimir >> >>> 30 июня 2020 г., в 23:09, Magnus Ihse Bursie >>> <magnus.ihse.bur...@oracle.com> написал(а): >>> >>> >>> On 2020-06-30 21:08, Vladimir Kempik wrote: >>>> Hello >>>> >>>> I agree modding hpp files is a bad idea >>>> >>>> Thanks for idea with setting LIBFFI_CFLAGS >>>> >>>> here is updated webrev: >>>> http://cr.openjdk.java.net/~vkempik/8248495/webrev.01/ >>> I still think you are doing this too complicated, and the wrong way around. >>>> AC_CHECK_HEADERS ignored CFLAGS for some reason, so modding header_name >>>> for it was still needed. >>> No, AC_CHECK_HEADERS does not work that way. It knows nothing about our >>> internal variables. How could it? >>> >>> First of all, I still think you should let PKG_CHECK_MODULES do its magic >>> first. If that fails, you can try compiling with AC_CHECK_HEADERS([ffi.h]), >>> just as the code currently does. >>> >>> Only if this fails, your workaround should kick in, before giving up >>> completely. At this point, you should check if >>> ${SYSROOT}/usr/include/ffi/ffi/ffi.h exists. If it does, you should set >>> LIBFFI_CFLAGS := -I${SYSROOT}/usr/include/ffi/ffi >>> >>> and you will not need the AC_CHECK_HEADERS, since you know the ffi.h file >>> is there, and there is a AC_LINK_IFELSE at the end to verify that >>> everything works. You can even skip the platform checks, since this will >>> apply to all configurations where the header file is in this odd place >>> relative to the sysroot. (But please save a comment about where you have >>> spotted it.) >>> >>> However, you are not done yet. Your patch do not address the whereabouts >>> of the library, only the include file. I assume it might too be stored in >>> an odd location? >>> >>> I see that we do not follow the best-practice of separating LDFLAGS and >>> LIBS here, so if you need to point to a non-standard location for the >>> library, you have to do like this example: >>> >>> LIBFFI_LIBS="-L${with_libffi}/lib -lffi" >>> >>> Ideally, this should be explit out to an LIBFFI_LDFLAGS, but that's a >>> change for another day, since it required changes in many places. >>> >>> /Magnus >>> >>> >>> >>>> This special case only applies to macos/clang when sysroot is set and no >>>> libffi configure options is used. (which is default case) >>>> >>>> Thanks, Vladimir >>>> >>>> >>>>> 30 июня 2020 г., в 19:46, Magnus Ihse Bursie >>>>> <magnus.ihse.bur...@oracle.com> написал(а): >>>>> >>>>> Vladimir, >>>>> >>>>> This looks like it can break in other situation than your specific case. >>>>> >>>>> It sounds like you should set LIBFFI_CFLAGS= to -I<path to your ffi >>>>> installation>, such that "<path to your ffi installation>/ffi.h" exists. >>>>> In particular, the change of include path in globalDefinitions_zero.hpp >>>>> looks bad. >>>>> >>>>> /Magnus >>>>> >>>>> On 2020-06-30 15:33, Vladimir Kempik wrote: >>>>>> Hello >>>>>> >>>>>> Please review this fix for zero vm building on macos. >>>>>> >>>>>> The issue comes from the libffi, it’s headers are located inside >>>>>> usr/include/ffi/ folder in Macos.sdk, so it can’t be found by configure >>>>>> script. >>>>>> >>>>>> If one wants to use system’s libffi and pass path to libffi via >>>>>> configure argument as --with-libffi-include=/usr/include/ffi, then it >>>>>> won’t be found by configure because clang will look exactly in >>>>>> /usr/include/ffi, but not in macos.sdk >>>>>> The system, at least on 10.15 doesn’t have /usr/includes at all. >>>>>> >>>>>> This patch makes jdk to look for ffi/ffi.h header in case of Macos/clang >>>>>> and no --with-libffi-include argument. >>>>>> >>>>>> However there is one issue with this patch, if --with-libffi-include >>>>>> passed then c++ code will still try to include <ffi/ffi.h> >>>>>> >>>>>> I’m not sure which way is the best for such rare case. it could be >>>>>> possible to define include filename in configure and pass it via -D and >>>>>> CFLAGS to c++ code. >>>>>> >>>>>> >>>>>> The webrev - http://cr.openjdk.java.net/~vkempik/8248495/webrev.00/ >>>>>> >>>>>> The bug - https://bugs.openjdk.java.net/browse/JDK-8248495 >>>>>> >>>>>> Thanks, Vladimir
