Hi Phil, Sergey, @Sergey: thaks for the review
Please see my other comments inline: On Sat, May 7, 2016 at 1:45 AM, Phil Race <philip.r...@oracle.com> wrote: > On 05/06/2016 04:33 PM, Volker Simonis wrote: > > Hi Phil, > > Thanks for looking at this problem. > > On Saturday, May 7, 2016, Phil Race <philip.r...@oracle.com> wrote: >> >> Hi, >> >> I've confirmed that what was pushed was v5 and it should really have been >> v6. >> I can't say unequivocally that it would have built on AIX but >> 1) It does not use RTLD_NOLOAD anywhere >> 2) all calls to dlopen include RTLD_LAZY. >> >> Here is the delta - the patch that makes v5 into v6 and although >> it is not quite the same as yours it bears a striking resemblance >> http://cr.openjdk.java.net/~prr/8156020/ >> > I don't think this will work correctly on AIX (although it may build) > because AIX only has a crippled /proc file system. And it won't work on any > other platform without /proc file system (it is actually even less "POSIX" > than RTLD_NOLOAD :) > > > Hmm. In internal discussions /proc was proposed precisely because > it was more widely (may be universally available). > > > How can you be sure that isLoadedLib() is really returning false because the > library is not loaded and not because the corresponding file in the proc > file system wasn't found? > > > This is definitely still early days but it was claimed to be more reliable > although we filed a follow-on bug because it did not seem like the kind > of thing we want to do. Even finding something that works reliably on Linux > is not final. It may be that this is something that needs to be tweaked for > each platform port, > > >> JPRT did build your patch (including on our embedded builds), and >> I am doing the same - in progress - for the v5->v6 delta but I have a >> dilemma. >> >> Option 1) Apply the delta of v5 -> v6 to client to get us where we should >> be >> and you "workaround it" in AIX until it makes its way to dev >> >> Option 2) Apply one of these patches to dev and sync it back to client >> and clean up the mess later >> 2a) apply v5->v6 >> 2b) apply Volker's patch and fix up the mess of the difference later. >> >> We are taking something of a risk in applying either to dev so I will >> need to do some kind of sanity checking >> > > It is OK for me if we fix this in client first as this seems to be the > easiest solution process-wise. But are you sure v6 gives you the right > answers on all you platforms and not just false positives as did the v5 > version? > > > Heck no :-). I know there was promising testing but it's early. > I am not sure this has been tried at all on Solaris either. > I'm pretty sure it hasn't and I'm pretty sure it wil not work :) On Solaris isLoadedLib() will always return false because "/proc/self/maps" is called "/proc/self/map" (without trailing 's'). And it is not only the different name. While the Linux version is in text format and contains the names of all the loaded libraries [1] the Solaris version is in binary format and only contains links to files in /proc/self/objects which don't seem to correspond to the real names of the libraries [2]. [1] http://linux.die.net/man/5/proc [2] http://docs.oracle.com/cd/E19253-01/816-5174/6mbb98ui2/index.html > > If you will check in v6 now I can fix it on AIX on Monday if that should > still give build problems. > > > Well it looks like v6 is about to finish up in JPRT so either is an option. > Your comments about /proc make me more inclined to go with your code. > So now I know more and have compared these how about we do as you > originally proposed. You push it to dev and Monday I will sync it into > client > and we'll take it from there. > Taking into account my findings about /proc on Solaris I agree and I've jsut pushed my version to jdk9-dev. > But maybe after the build fixes we should go for a more general solution and > define platform-specific "isLibLoaded()" functions? > > > Yes, this is a work-in-progress. > We should definitely go for platform-specific 'isLibLoaded()' version which can be used by all the class library components. I remember there is at least one other place somewhere in crypto which uses RTLD_NOLOAD. And we should definitely try to get a regression test for this issue although I understand that testing the case with a pre-loaded libgtk may be not so easy... Regards, Volker > -phil. > > > > Regards, > Volker > >> Opinions ? >> >> -phil. >> >> >> On 05/05/2016 09:32 PM, Philip Race wrote: >>> >>> Hi Volker, >>> >>> 1) adding awt-dev. Semyon did the review on swing but really it should >>> always >>> (and mainly!) have been awt. >>> >>> 2) Yes, this ought to be pushed to 9-client, specifically not 9-dev. >>> Assuming it goes to 9-dev we may need to deal with conflicts. >>> Also if it causes any kind of problem with 9-dev I would not want to pile >>> fix on fix, so it would probably just get anti-deltaed. Just a warning. >>> >>> 3) It strictly needs a JPRT run before pushing so someone will need to do >>> that. >>> >>> 4) This change definitely needs two reviewers. >>> >>> And we were discussing RTLD_NOLOAD is not Posix as that came up why it >>> was not >>> a solution in a cross-platform solution for determining whether libs were >>> already loaded but it was reported to not be able to detect some cases. >>> So I thought we had determined it was not a general solution. >>> Leaving aside why it is in there after that (something I will need to >>> check), >>> the lack of the other flag may explain why it was apparently "not >>> working". >>> >>> So one interesting thing is it appears to me that I thought we pushed >>> the .v6 webrev - the one I thought we (or I) approved since it was the >>> latest >>> obviously >>> http://mail.openjdk.java.net/pipermail/swing-dev/2016-April/005684.html >>> >>> but this looks like the v5 webrev was pushed : >>> http://mail.openjdk.java.net/pipermail/swing-dev/2016-April/005678.html >>> >>> All of this "detection" code was the main issue at that juncture. >>> So I would like some time to disentangle that before anything is changed. >>> >>> -phil >>> >>> On 5/4/16, 11:32 AM, Volker Simonis wrote: >>>> >>>> Hi, >>>> >>>> can somebody please review this small change which fixes the AIX build >>>> after change 8145547, but also fixes an incorrect usage pattern of >>>> RTLD_NOLOAD in 8145547: >>>> >>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8156020/ >>>> https://bugs.openjdk.java.net/browse/JDK-8156020 >>>> >>>> Here are the details from the bug report: >>>> >>>> Change 8145547 uses the RTLD_NOLOAD flag when calling dlopen to probe >>>> the availability of the GTK libraries. >>>> >>>> But unfortunately RTLD_NOLOAD is not Posix and for example not >>>> available on AIX and BSD. >>>> >>>> I also found out, that the implementation of 8145547 contains a bug. >>>> It uses RTLD_NOLOAD in an incorrect way. The man page for dlopen >>>> clearly states that one of the two flags RTLD_LAZY or RTLD_NOW has to >>>> be included in the flags. But the current implementation uses >>>> RTLD_NOLOAD as single flag. Therefor the call to dlopen() currently >>>> always returns NULL, no difference if the corresponding library has >>>> been loaded already or not. >>>> >>>> The bug report also contains a small C program which can be used to >>>> reproduce the problem. >>>> >>>> The fix is to only use RTLD_NOLOAD if it is defined. The change >>>> removes the 'flags' argument from the various check() functions and >>>> replaces it with a boolean 'load' argument. It indicates if the check >>>> functions should just look for a previously loaded version of the GTK >>>> libraries (i.e. if 'load' == false) or if it should additionally try >>>> to load the libraries if that hasn't been done before (i.e. if 'load' >>>> == true). >>>> >>>> I hope I haven't changed the previous program semantics with my >>>> change. At least I couldn't see any difference :) >>>> >>>> I've built and smoke tested on Linux/Solaris and AIX with various >>>> combinations for jdk.gtk.version, >>>> -Dswing.defaultlaf=com.sun.java.swing.plaf.gtk.GTKLookAndFeel and >>>> FileDialog implementations. >>>> >>>> I'd like to push this directly to jdk9-dev to fix the AIX build as >>>> fast as possible. Would that be OK? >>>> >>>> Thank you and best regards, >>>> Volker >> >> >