RFR: Handle confstr version lookup failures gracefully

[Mikael Vidstedt]

> On Apr 12, 2017, at 3:04 AM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
> 
> On 2017-04-12 04:49, Mikael Vidstedt wrote:
>>> On Apr 11, 2017, at 7:43 PM, David Holmes <david.holmes at oracle.com> <mailto:david.holmes at oracle.com> wrote:
>>> 
>>> 
>>> 
>>> On 12/04/2017 12:06 PM, Mikael Vidstedt wrote:
>>>>> On Apr 11, 2017, at 6:58 PM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com>
>>>>> <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com>>> wrote:
>>>>> 
>>>>> On 12/04/2017 11:42 AM, Mikael Vidstedt wrote:
>>>>>>> On Apr 11, 2017, at 6:27 PM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com>
>>>>>>> <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com>>> wrote:
>>>>>>> 
>>>>>>> On 12/04/2017 10:00 AM, Mikael Vidstedt wrote:
>>>>>>>> During bootstrapping, os::Linux::libpthread_init() tries to
>>>>>>>> discover the versions of (g)libc and libpthread respectively. This
>>>>>>>> is done using confstr(3). However, musl doesn’t implement support
>>>>>>>> for version discovery, so confstr will return 0/EINVAL.
>>>>>>>> 
>>>>>>>> This change makes the code handle confstr failures gracefully, and
>>>>>>>> defaults using the string “unknown” if the version information
>>>>>>>> cannot be looked up.
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/portola/glibcversion/webrev.01/hotspot/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/portola/glibcversion/webrev.01/hotspot/webrev/>
>>>>>>> There was no need to separate out the declaration here:
>>>>>>> 
>>>>>>> 504   size_t n;
>>>>>>> 505
>>>>>>> 506   n = confstr(_CS_GNU_LIBC_VERSION, NULL, 0);
>>>>>>> 
>>>>>>> Overall I don't think this change is quite what we would want in
>>>>>>> that it is wrong to print "unknown" when in fact there is no libc
>>>>>>> (or libpthread?) involved. And we would surely want to see that we
>>>>>>> are using musl.
>>>>>> Since the variable is reused for the second confstr call I prefer to
>>>>>> declare it separately.
>>>>> Declaring uninitialized variables is an anachronism IMHO. :)
>>>>> 
>>>>>> In an upcoming change I have added support for adding “-musl” to the
>>>>>> (internal) version string so that we can at least see that it’s a
>>>>>> musl VM instead of a glibc one.
>>>>>> 
>>>>>> There is a libc - musl. libpthread happens to be part of that same
>>>>>> libc instead of being a separate library, so depending on how you
>>>>>> look at it there is or isn’t a libpthread (it is effectively an
>>>>>> implementation detail of musl). The problem is that there’s no way of
>>>>>> getting the version information at runtime with musl...
>>>>> So if we can detect that we are musl then the libc version should be
>>>>> "musl" and the libpthread version should be "<integrated>" (or
>>>>> something like that.
>>>> That’s fair. My upcoming change will pass in the target C library as an
>>>> explicit compiler define. Detecting it at runtime is AFAICT not
>>>> possible. Once I have the change in place I can go back and update the
>>>> values of these two “versions”.
>>>> 
>>>>> I'm surprised there is no mechanism at all for runtime detection of musl!
>>>>> 
>>>>> But musl defines in unistd.h
>>>>> 
>>>>> #define _CS_GNU_LIBC_VERSION2
>>>>> #define _CS_GNU_LIBPTHREAD_VERSION3
>>>>> 
>>>>> so won't those be reported by confstr ??
>>>> Unfortunately not. confstr will return 0/EINVAL for almost all
>>>> arguments. Those two defines are likely only there to make code compile
>>>> at all with musl, but if confstr is actually called at runtime with any
>>>> of those values it will return an error.
>>> I can not make any sense out of:
>>> 
>>> https://git.musl-libc.org/cgit/musl/tree/src/conf/confstr.c <https://git.musl-libc.org/cgit/musl/tree/src/conf/confstr.c> <https://git.musl-libc.org/cgit/musl/tree/src/conf/confstr.c> <https://git.musl-libc.org/cgit/musl/tree/src/conf/confstr.c>
>>> 
>>> Quality code - love the clear explanatory comments. :(
>> I’ve stared at that one before wondering what the second if statement is trying to express, but in either case it’s not going to return anything useful :)
> Clearly, it's checking if name &~ 4U is not equal to -1, and if name minus _CS_POSIX_V6_ILP32_OFF32_CFLAGS is greater than 33U.

Ah, of course. Stupid me!

> I thought you were a master at C programming. ;-)

Not since the accident ;)

Cheers,
Mikael


> 
> (Thanks for the laugh of today...)
> 
> /Magnus
>> 
>> Cheers,
>> Mikael
>> 
>>> David
>>> 
>>>> Cheers,
>>>> Mikael
>>>> 
>>>>> Thanks,
>>>>> David
>>>>> 
>>>>>> Cheers,
>>>>>> Mikael
>