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

Sat Jul 18 04:41:33 UTC 2020

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> 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>
> Sent: Friday, July 17, 2020 4:17 PM
> To: Thomas Stüfe <thomas.stuefe at gmail.com>; Ivanov, Vladimir A <
> vladimir.a.ivanov at intel.com>
> Cc: 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
>
> 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> 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>
> >>> *Sent:* Friday, July 17, 2020 2:26 PM
> >>> *To:* Ivanov, Vladimir A <vladimir.a.ivanov at intel.com>; Hotspot dev
> >>> runtime <hotspot-runtime-dev at openjdk.java.net>
> >>> *Cc:* 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>
> >>> 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> 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
> >>>
> >>>
>