RFR[16] 8248495: [macos] zerovm is broken due to libffi headers location

[Magnus Ihse Bursie]

On 2020-07-02 11:13, Vladimir Kempik wrote:
> Hello
> Thanks for looking into this
> Here is updated webrev.
>
> http://cr.openjdk.java.net/~vkempik/8248495/webrev.03/
>
> Regards, Vladimir.
Thank you. Now it looks good.

/Magnus
>
>> 1 июля 2020 г., в 21:59, Erik Joelsson <erik.joelsson at 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.bursie at 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.bursie at 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