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

Mon Jul 20 17:13:26 UTC 2020

HI,
The updated patch available as http://cr.openjdk.java.net/~sviswanathan/Vladimir/8249672/webrev.03/
It use the ‘fgets’ instead of ‘getline’ to use local memory.
The tier1 tests passed on the release and fastdebug builds on Linux and fastdebug builds on MacOS systems.
Testing results same for patched and non-patched builds.

Thanks, Vladmir

From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Friday, July 17, 2020 10:25 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

Oh, sorry, you are right :(

I was under the assumption you wanted to call os::cpu_microcode_revision() directly from within VMError::report(). During initialization using c-heap like this should not be a problem and you can forget about 9/10ths of what I wrote, sorry.

In that case your original variant is fine, my only suggestion would be to clearly mark the free as ::free() with a comment to prevent someone from correcting it to os::free.

Thank you,

Thomas

On Sat, Jul 18, 2020 at 7:08 AM Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>> wrote:
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<mailto:thomas.stuefe at gmail.com>>
Sent: Friday, July 17, 2020 9:42 PM
To: Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>>
Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com<mailto:vladimir.kozlov at oracle.com>>; 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

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
>>>
>>>