Thanks Ioi, David! I will push webrev.02 later.
Yasumasa 2019年3月21日(木) 14:08 David Holmes <david.hol...@oracle.com>: > On 21/03/2019 2:11 am, Ioi Lam wrote: > > That's the problem with ld and static libraries. If libbfd.a depends on > > libz.a, you must put libbfd.a before libz.a. > > > > Since this is a rarely touched Makefile, I think what you're doing in > > webrev.02 is fine. We can refactor it later if it becomes more unwieldy. > > Okay I can live with webrev.02 as well. > > Thanks, > David > ----- > > > Thanks > > - Ioi > > > > On 3/20/19 12:07 AM, Yasumasa Suenaga wrote: > >> Hi David, > >> > >> I tried to fix as you suggested, but it was failed. > >> > >> x86_64-w64-mingw32-gcc -o build/windows-amd64/hsdis-amd64.dll > >> -Ibuild/windows-amd64/include > >> -I/home/ysuenaga/rpmbuild/BUILD/binutils-2.31.1/include > >> -I/home/ysuenaga/rpmbuild/BUILD/binutils-2.31.1/bfd > >> -Ibuild/windows-amd64/bfd -DLIBARCH_amd64 -DLIBARCH=\"amd64\" > >> -DLIB_EXT=\".dll\" -O hsdis.c -shared build/windows-amd64/zlib/libz.a > >> build/windows-amd64/bfd/libbfd.a > >> build/windows-amd64/opcodes/libopcodes.a > >> build/windows-amd64/libiberty/libiberty.a > >> build/windows-amd64/bfd/libbfd.a(compress.o):compress.c:(.text+0x5b): > >> undefined reference to `inflateInit_' > >> > >> I tried to compile with moving libz.a to the last argument, it was > >> succeeded. > >> > >> x86_64-w64-mingw32-gcc -o build/windows-amd64/hsdis-amd64.dll > >> -Ibuild/windows-amd64/include > >> -I/home/ysuenaga/rpmbuild/BUILD/binutils-2.31.1/include > >> -I/home/ysuenaga/rpmbuild/BUILD/binutils-2.31.1/bfd > >> -Ibuild/windows-amd64/bfd -DLIBARCH_amd64 -DLIBARCH=\"amd64\" > >> -DLIB_EXT=\".dll\" -O hsdis.c -shared > >> build/windows-amd64/bfd/libbfd.a > >> build/windows-amd64/opcodes/libopcodes.a > >> build/windows-amd64/libiberty/libiberty.a > >> build/windows-amd64/zlib/libz.a > >> > >> I'm not unclear why it is failed. > >> But order of libraries seems to be important. > >> > >> > >> Yasumasa > >> > >> > >> 2019年3月20日(水) 15:18 David Holmes <david.hol...@oracle.com>: > >>> On 20/03/2019 4:03 pm, Yasumasa Suenaga wrote: > >>>> Hi David, > >>>> > >>>> 2019年3月20日(水) 14:25 David Holmes <david.hol...@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.hol...@oracle.com>: > >>>>>>> On 20/03/2019 11:54 am, Yasumasa Suenaga wrote: > >>>>>>>> Hi David, > >>>>>>>> > >>>>>>>> 2019年3月20日(水) 10:38 David Holmes <david.hol...@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.hol...@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 > > >
