RFR: 8220784: hsdis cannot be built with MinGW64

[David Holmes]

On 20/03/2019 4:03 pm, Yasumasa Suenaga wrote:
> Hi David,
> 
> 2019年3月20日(水) 14:25 David Holmes <david.holmes at oracle.com>:
>>
>> Hi Yasumasa,
>>
>> On 20/03/2019 3:01 pm, Yasumasa Suenaga wrote:
>>> Thanks David!
>>>
>>> I uploaded new webrev.
>>> This change affects Windows only, and I confirmed it works fine for
>>> Linux x64 and MinGW64.
>>>     http://cr.openjdk.java.net/~ysuenaga/JDK-8220784/webrev.01/
>>
>> I this is for MINGW only then shouldn't the change be made within this
>> section:
> 
> Did you say we should determine with $(MINGW) whether libz.a is added
> to LDFLAGS?
> webrev is here:
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8220784/webrev.02/
> 
> The code is added to outside of the section you pointed.
> 
> $(LIBRARIES) is available after OS-specific code.
> So we need to change like webrev.02 to minimize changes.

Or you could add

LIBRARIES = .../libzip.a

inside the existing MINGW section and then change the external

LIBRARIES = ...

to

LIBRARIES += ...

I'm unclear whether MINGW is only an issue on Linux when building for 
Windows or whether also of use on Windows itself?

David

> 
> Yasumasa
> 
> 
>>     ## OS = Linux ##
>>     ifeq         ($(OS),Linux)
>>       ifneq           ($(MINGW),)
>>         LIB_EXT          = .dll
>>         CPPFLAGS += -I$(TARGET_DIR)/include
>>         LDFLAGS += -L$(TARGET_DIR)/lib
>>         OS=windows
>>         ifneq           ($(findstring x86_64-,$(MINGW)),)
>>           ARCH=amd64
>>         else
>>           ARCH=i386
>>         endif
>>         CC               = $(MINGW)-gcc
>>         CONFIGURE_ARGS= --host=$(MINGW) --target=$(MINGW)
>>       else   #linux
>> ?
>>
>> David
>> -----
>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> 2019年3月20日(水) 11:25 David Holmes <david.holmes at oracle.com>:
>>>>
>>>> On 20/03/2019 11:54 am, Yasumasa Suenaga wrote:
>>>>> Hi David,
>>>>>
>>>>> 2019年3月20日(水) 10:38 David Holmes <david.holmes at oracle.com>:
>>>>>>
>>>>>> Hi Yasumasa,
>>>>>>
>>>>>> I'm not familar with building hsdis, but if the only currnet problem is
>>>>>> on Windows, why is the fix not restricted to only building on Windows?
>>>>>
>>>>> For Solaris and Linux, -lz is not added to LDFLAGS. I think it means
>>>>> initial committer want to use zlib in binutils.
>>>>> And zlib in binutils will be built implicitly in build phase in binutils.
>>>>>
>>>>>> Otherwise we need people who build hsdis on other platforms to comment
>>>>>> on the appropriateness of the fix.
>>>>>
>>>>> If using different zlib is used by platforms is allowed, I will upload
>>>>> webrev which affects Windows only.
>>>>> What do you think?
>>>>
>>>> That seems preferable/simpler. Then you only need to know that your
>>>> change works for non-MinGW64 environments.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 20/03/2019 11:07 am, Yasumasa Suenaga wrote:
>>>>>>> Hi Erik, David,
>>>>>>>
>>>>>>> I checked this change on Linux x64 and MinGW for Windows.
>>>>>>> According to hsdis README, we need to use MinGW cross compiler to
>>>>>>> build hsdis [1]. So I think Cygwin is not required.
>>>>>>>
>>>>>>> I do not have macOS and AIX. So I cannot check this change on them.
>>>>>>>
>>>>>>> BTW is hsdis included Java SE spec?
>>>>>>> hsdis seems not to be included jtreg tests, and it is not contained in
>>>>>>> OpenJDK binaries.
>>>>>>> I think it is allowed even if we lack some test for hsdis if hsdis is
>>>>>>> not required for Java SE.
>>>>>>> If not so, I want sponsor(s) for this change.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> [1] http://hg.openjdk.java.net/jdk/jdk/file/ddfb658c8ce3/src/utils/hsdis/README#l91
>>>>>>>
>>>>>>> 2019年3月20日(水) 6:54 David Holmes <david.holmes at oracle.com>:
>>>>>>>>
>>>>>>>> CC'ing hotspot-dev. I agree this needs to be checked on every platform
>>>>>>>> affected. I can't comment on the fix itself.
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>> On 20/03/2019 2:36 am, Erik Joelsson wrote:
>>>>>>>>> I think this needs to be reviewed by at least someone in hotspot who
>>>>>>>>> regularly builds hsdis. I can't really comment on the validity of the
>>>>>>>>> patch as I'm unfamiliar with both hsdis as well as this makefile. Have
>>>>>>>>> you at least verified the build on all the platforms which you affect
>>>>>>>>> with this change (which would be at least Macos, AIX and Windows in a
>>>>>>>>> normal Cygwin VS env)?
>>>>>>>>>
>>>>>>>>> /Erik
>>>>>>>>>
>>>>>>>>> On 2019-03-18 17:56, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Please review this change:
>>>>>>>>>>
>>>>>>>>>>        JBS: https://bugs.openjdk.java.net/browse/JDK-8220784
>>>>>>>>>>        webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8220784/webrev.00/
>>>>>>>>>>
>>>>>>>>>> I attempt to build hsdis for Windows on WSL Ubuntu 18.04 with
>>>>>>>>>> gcc-mingw-w64-x86-64, but I saw error messages that some functions
>>>>>>>>>> which are provided by zlib are unresolved.
>>>>>>>>>> We need to link to zlib.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa