[16] RFR(S) 8249672: Include microcode revision in features_string on x86

Sat Jul 18 05:07:59 UTC 2020

Hi,
seems, this info created during initialization phase. Is it correct? Collect or parse common info at the crash point usually not a good idea. During initialization usage of the c-heap not a problem.
The “::free” work OK here. At least tier1 test produce same results for patched and non-patched builds. But these tests not generates real case for hs_err files.
It looks like 2k byte array enough for the one record for CPU from cpuinfo file. Will update code to use local buffer.

Thanks, Vladimir

From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Friday, July 17, 2020 9:42 PM
To: Ivanov, Vladimir A <vladimir.a.ivanov at intel.com>
Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>; hotspot-compiler-dev at openjdk.java.net
Subject: Re: [16] RFR(S) 8249672: Include microcode revision in features_string on x86

Hi,

yes, you must use the raw free here (for the same reason we cannot pass in an os::malloc() allocated buffer to getline, since if it were to resize it would use raw ::realloc() internally and crash the same way).

But as I wrote in my first mail to the original thread, I would not use c-heap memory at all, since this function is used during crash reporting in the signal handler and the c-heap may be corrupted.

It the max line length of /proc/cpu can be reliably predicted (so that getline wont realloc()) I would pass a stack allocated buffer into getline. If not, I would not use getline() at all but rewrite this, probably using fgets().

Cheers, Thomas

On Sat, Jul 18, 2020 at 1:24 AM Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>> wrote:
Thanks, I expected the C's functions here. Let's wait a little bit for Runtime team and update work with buffer.

 Thanks, Vladimir

-----Original Message-----
From: Vladimir Kozlov <vladimir.kozlov at oracle.com<mailto:vladimir.kozlov at oracle.com>>
Sent: Friday, July 17, 2020 4:17 PM
To: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>; Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>>
Cc: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>>; hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>
Subject: Re: [16] RFR(S) 8249672: Include microcode revision in features_string on x86

I think the issue is 'line' buffer is allocated by libc getline() and os:free() which is HotSpot function [1] does not know about it. You need C's ::free() or use HS's os::malloc() to allocate 'line' buffer.

Someone from Runtime may suggest what is the best for this case.

Thanks,
Vladimir K

[1] http://hg.openjdk.java.net/jdk/jdk/file/14f465f62984/src/hotspot/share/runtime/os.cpp#l792

On 7/17/20 4:03 PM, Vladimir Kozlov wrote:
> I updated subject to our formal review request format (JDK version, RFE's id and subject).
>
> I moved RFE to runtime group as Thomas said:
>
> https://bugs.openjdk.java.net/browse/JDK-8249672
>
> Submitted tier1 testing to build on all our supported platforms. And debug builds on linux failed:
>
> #  SIGSEGV (0xb) at pc=0x0000146fc6af4b0b, pid=9715, tid=9718 # V
> [libjvm.so+0xc12b0b]  GuardedMemory::print_on(outputStream*)
> const+0xeb
>
> V  [libjvm.so+0xc12b0b]  GuardedMemory::print_on(outputStream*)
> const+0xeb V  [libjvm.so+0x13c898a]  verify_memory(void*)+0x26a V
> [libjvm.so+0x13cd30b]  os::free(void*)+0x5b V  [libjvm.so+0x13e5598]
> os::cpu_microcode_revision()+0xc8 V  [libjvm.so+0x17d314c]
> VM_Version::get_processor_features()+0x76c
> V  [libjvm.so+0x17d6ead]  VM_Version::initialize()+0x10d V
> [libjvm.so+0x17ce6c6]  VM_Version_init()+0x26 V  [libjvm.so+0xcb2895]
> init_globals()+0x55 V  [libjvm.so+0x16dde63]
> Threads::create_vm(JavaVMInitArgs*, bool*)+0x2d3
>
>
> Regards,
> Vladimir K
>
> On 7/17/20 3:02 PM, Thomas Stüfe wrote:
>> Hi Vladimir,
>>
>> On Fri, Jul 17, 2020 at 11:57 PM Ivanov, Vladimir A <
>> vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>> wrote:
>>
>>>>   +#if defined(IA32) || defined(AMD64)
>>>>
>>>> Is that not synonymous with x86?
>>>
>>> This patter was copied from the method “print_model_name_and_flags”
>>> (file os/linux/os_linux.cpp).
>>>
>>> This method also read the “/proc/cpuinfo” file and I reuse it as
>>> ‘template’ for the new method.
>>>
>>> It is better to use one pattern to work with exactly same file but
>>> in general you are right.
>>>
>>> The X86 is defined in the file ./share/utilities/macros.hpp as:
>>>
>>> #if defined(IA32) || defined(AMD64)
>>>
>>> #define X86
>>>
>>> #define X86_ONLY(code) code
>>>
>>> #define NOT_X86(code)
>>>
>>>
>>>
>>> The question here: could I delete this “ifdefs” while this method
>>> should work on x86 only?
>>>
>>>
>>>
>>
>> os_linux_x86.cpp is compiled for x86 platforms only, whereas
>> os_linux.cpp is shared among all architectures.
>>
>> So, in the former you do not need to exclude non-x86 architectures.
>>
>> Cheers, Thomas
>>
>>
>>> Thanks, Vladimir
>>>
>>>
>>>
>>> *From:* Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
>>> *Sent:* Friday, July 17, 2020 2:26 PM
>>> *To:* Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>>; Hotspot dev
>>> runtime <hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>>
>>> *Cc:* hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>
>>> *Subject:* Re: add microcode version to the hs_err files
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> On Fri, Jul 17, 2020 at 11:19 PM Thomas Stüfe
>>> <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
>>> wrote:
>>>
>>> Hi Vladimir,
>>>
>>>
>>>
>>> I think this would be more suited to hotspot-runtime.
>>>
>>>
>>>
>>>
>>> http://cr.openjdk.java.net/~sviswanathan/Vladimir/8249672/webrev.00/
>>> src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp.udiff.html
>>>
>>>
>>>
>>> +#if defined(IA32) || defined(AMD64)
>>>
>>> Is that not synonymous with x86?
>>>
>>>
>>>
>>> +    while ((read = getline(&line, &len, fp)) != -1) {
>>> +      if (len > 10 && strstr(line, "microcode") != NULL) {
>>> +        char* rev = strchr(line, ':');
>>> +        if (rev != NULL) sscanf(rev + 1, "%x", &result);
>>> +        break;
>>> +      }
>>> +    }
>>> +    free(line);
>>>
>>>
>>>
>>> Not sure this works as intended. At the first call to getline() it
>>> will allocate a line buffer for you and return it. That buffer will
>>> be as large as the first line you happen to read. You then pass that
>>> same buffer into getline to fetch the next lines, but what if those
>>> are longer than the first?
>>>
>>>
>>>
>>>
>>>
>>> Forget that point, getline calls realloc() on the line buffer to
>>> resize it, so this should be okay.
>>>
>>>
>>>
>>> Thanks, Thomas
>>>
>>>
>>>
>>> But anyway it would be better to pass a simple caller provided
>>> buffer in - stack allocated. Since this function is called at crash
>>> time and the C heap could be corrupted.
>>>
>>>
>>>
>>> Cheers, Thomas
>>>
>>>
>>>
>>>
>>>
>>> On Fri, Jul 17, 2020 at 10:22 PM Ivanov, Vladimir A <
>>> vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>> wrote:
>>>
>>> Hello,
>>>
>>> could you please review the patch
>>> http://cr.openjdk.java.net/~sviswanathan/Vladimir/8249672/webrev.00/
>>>
>>> This patch add the microcode version for different OSes that may be
>>> useful in the issue resolution process.
>>>
>>>
>>>
>>> The reported microcode version for different OSes loos as:
>>>
>>>
>>>
>>> Linux (RHEL7.7):
>>>
>>> # cat hs_err_pid251046.log |grep microc
>>>
>>> CPU: total 112 (initial active 112) (28 cores per cpu, 2 threads per
>>> core) family 6 model 85 stepping 4 microcode 0x200005e, cmov, cx8,
>>> fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt,
>>> vzeroupper, avx, avx2, aes, clmul, erms, rtm, 3dnowpref, lzcnt, ht,
>>> tsc, tscinvbit, bmi1, bmi2, adx, fma, clflush, clflushopt, clwb
>>>
>>>
>>>
>>> Windows (Win10, v1809):
>>>
>>> CPU: total 4 (initial active 4) (2 cores per cpu, 2 threads per
>>> core) family 6 model 142 stepping 9 microcode 0xb4, cmov, cx8, fxsr,
>>> mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, vzeroupper,
>>> avx, avx2, aes, clmul, erms, rtm, 3dnowpref, lzcnt, ht, tsc,
>>> tscinvbit, bmi1, bmi2, adx, fma, clflush, clflushopt
>>>
>>>
>>>
>>> MacOS (Darwin):
>>>
>>> $ cat hs_err_pid95187.log |grep microc
>>>
>>> CPU: total 8 (initial active 8) (4 cores per cpu, 2 threads per
>>> core) family 6 model 126 stepping 5 microcode 0x78, cmov, cx8, fxsr,
>>> mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, vzeroupper,
>>> avx, avx2, aes, clmul, erms, 3dnowpref, lzcnt, ht, tsc, tscinvbit,
>>> bmi1, bmi2, adx, sha, fma, clflush, clflushopt
>>>
>>>
>>>
>>> Thanks, Vladimir
>>>
>>>
>>>    Thanks, Vladimir
>>>
>>>