RFR (S) 8237591: Mac: include OS X version in hs_err_pid crash log file

[gerard ziemski]

Thank you for the review.

May I please have a 2nd reviewer? (or would this be considered trivial?)


cheers

On 7/30/20 1:10 AM, David Holmes wrote:
> On 30/07/2020 4:51 am, gerard ziemski wrote:
>> On 7/27/20 6:21 PM, David Holmes wrote:
>>> On 28/07/2020 2:12 am, gerard ziemski wrote:
>>>> Thank you David for taking a look.
>>>>
>>>>
>>>> On 7/19/20 11:37 PM, David Holmes wrote:
>>>>> Hi Gerard,
>>>>>
>>>>> On 18/07/2020 5:19 am, gerard ziemski wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this small fix that adds the OS version and the OS 
>>>>>> build number to the hs_err_pidXXX.log output in the “Summary” 
>>>>>> section for Mac platform (it’s easier to use for developers than 
>>>>>> the Darwin kernel version that we display right now).
>>>>>>
>>>>>> This is how things used to look:
>>>>>>
>>>>>>
>>>>>> --------------- S U M M A R Y ------------
>>>>>>
>>>>>> Command Line: Crasher
>>>>>>
>>>>>> Host: Gerards-MBP-16, MacBookPro16,1 x86_64 2600 MHz, 12 cores, 
>>>>>> 32G, Darwin 19.5.0
>>>>>> Time: Thu Jul 16 14:01:46 2020 CDT elapsed time: 1.089465 seconds 
>>>>>> (0d 0h 0m 1s)
>>>>>>
>>>>>>
>>>>>> And this is how the “Summary” section looks like with the 
>>>>>> proposed change:
>>>>>>
>>>>>>
>>>>>> --------------- S U M M A R Y ------------
>>>>>>
>>>>>> Command Line: Crasher
>>>>>>
>>>>>> Host: Gerards-MBP-16, MacBookPro16,1 x86_64 2600 MHz, 12 cores, 
>>>>>> 32G, Darwin 19.5.0, macOS 10.15.5 (19F101)
>>>>>> Time: Thu Jul 16 14:02:29 2020 CDT elapsed time: 0.360881 seconds 
>>>>>> (0d 0h 0m 0s)
>>>>>>
>>>>>>
>>>>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8237591
>>>>>> open webrev at http://cr.openjdk.java.net/~gziemski/8237591_rev1
>>>>>> testing Mach5 hs_tier1,2,3,4,5 in progress
>>>>>
>>>>> Just to be clear, the changes prior to:
>>>>>
>>>>> 1555 #ifdef __APPLE__
>>>>>
>>>>> are just fixing up existing indentation errors - correct?
>>>>
>>>> Yes, hope that's OK, as this was the only spot in the function that 
>>>> stood out with inconsistent indentation.
>>>
>>> Yes that is fine.
>>>
>>>>>
>>>>> The actual change seems okay, just one query:
>>>>>
>>>>> 1562     int mib_build[] = { CTL_KERN, KERN_OSVERSION };
>>>>>
>>>>> I couldn't find KERN_OSVERSION documented for sysctl - is it a 
>>>>> "recent" addition?
>>>>
>>>> Yes it is. Apple added it back in 2018 (see bug comments or this 
>>>> link 
>>>> https://github.com/apple/darwin-xnu/commit/5bbb823c13f3ab1ab58878f96b35433a29882676?diff=split#diff-6651b0c84a045f400bc45faa9f61c9e1 
>>>> )
>>>
>>> That link shows the addition of sysctl_osproductversion which I 
>>> assume underpins "kern.osproductversion". But my question was on 
>>> KERN_OSVERSION. That definition seems to already exist prior to the 
>>> change you link. My concern is whether it was also fairly recently 
>>> introduced and so referring to it would require a minimum macOS 
>>> version on the build machine?
>>
>> Sorry, I thought you meant "kern.osproductversion", not 
>> KERN_OSVERSION, but that's a valid question.
>>
>> I found Apple using KERN_OSVERSION in its own code since macOS 10.7, 
>> i.e. 
>> https://opensource.apple.com/source/Libc/Libc-763.11/gen/assumes.c.auto.html 
>> , though I could not find any documentation of it either.
>
> I found it in sysctl.h from 10.5 dev kit as well, so that looks fine 
> to use.
>
> Thanks for checking.
> David
> -----