RFR: Handle confstr version lookup failures gracefully

[Magnus Ihse Bursie]

On 2017-04-12 04:49, Mikael Vidstedt wrote:
>> On Apr 11, 2017, at 7:43 PM, David Holmes <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>>> 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>>> 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/
>>>>>> 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>
>>
>> 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. I thought you 
were a master at C programming. ;-)

(Thanks for the laugh of today...)

/Magnus
>
> Cheers,
> Mikael
>
>> David
>>
>>> Cheers,
>>> Mikael
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Cheers,
>>>>> Mikael