<AWT Dev> <Swing Dev> RFR(S): 8156020: 8145547 breaks AIX and and uses RTLD_NOLOAD incorrectly

Sat May 7 08:05:14 UTC 2016

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.race at 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.race at 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
>>
>>
>