Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8261966: macOS M1: report in hs_err log if we are running x86 code in emulation mode (Rosetta) #3077

Closed
wants to merge 5 commits into from

Conversation

@gerard-ziemski
Copy link

@gerard-ziemski gerard-ziemski commented Mar 18, 2021

Please review this simple enhancement where we indicate in hs_err log file whether the CPU is being emulated. Right now the only use case is for Apple's M1 aarch64 running x64 code, i.e. "Rosetta" emulation.

This enhancement will insert (EMULATED) label in the Host and CPU section, ex:

Host: Oracles-MacBook-Pro-16.local, MacBookPro16,1 x86_64 2600 MHz, 12 cores, 32G, Darwin 19.6.0, macOS 10.15.7 (19H114)

becomes

Host: Oracles-MacBook-Pro-16.local, "MacBookPro16,1" x86_64 (EMULATED) 2600 MHz, 12 cores, 32G, Darwin 19.6.0, macOS 10.15.7 (19H114)

and

CPU: total 12 (initial active 12) (6 cores per cpu, 2 threads per core) family 6 model 158 stepping 10 microcode 0xde, cx8, cmov, fxsr, ht, mmx, 3dnowpref, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, lzcnt, tsc, tscinvbit, avx, avx2, aes, erms, clmul, bmi1, bmi2, adx, fma, vzeroupper, clflush, clflushopt

becomes

CPU: (EMULATED) total 12 (initial active 12) (6 cores per cpu, 2 threads per core) family 6 model 158 stepping 10 microcode 0xde, cx8, cmov, fxsr, ht, mmx, 3dnowpref, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, lzcnt, tsc, tscinvbit, avx, avx2, aes, erms, clmul, bmi1, bmi2, adx, fma, vzeroupper, clflush, clflushopt

I also took the opportunity here to fix the model name by adding " around it as Apple uses commas in the name, which makes it harder to parse the Host section, since we also use commas to delineate, ex: MacBookPro16,1 becomes "MacBookPro16,1"


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8261966: macOS M1: report in hs_err log if we are running x86 code in emulation mode (Rosetta)

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3077/head:pull/3077
$ git checkout pull/3077

To update a local copy of the PR:
$ git checkout pull/3077
$ git pull https://git.openjdk.java.net/jdk pull/3077/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 18, 2021

👋 Welcome back gziemski! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

@gerard-ziemski The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@gerard-ziemski gerard-ziemski marked this pull request as ready for review Mar 18, 2021
@openjdk openjdk bot added the rfr label Mar 18, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 18, 2021

Loading

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

In the bug report, @dholmes-ora mentions two different places where
"(EMULATED)" should show up. This change covers the first:

Host: MacBookPro17,1 x86_64 2400 MHz, 8 cores, 16G, Darwin 20.3.0

becomes

Host: MacBookPro17,1 x86_64 (EMULATED) 2400 MHz, 8 cores, 16G, Darwin 20.3.0

but I don't think that it covers the second:

CPU: total 8 (initial active 8) (1 cores per cpu, 1 threads per core) family 6 model 44 stepping 0 microcode 0x0, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, aes, clmul, tsc, tscinvbit, tscinv, clflush

becomes:

CPU: (EMULATED) total 8 (initial active 8) (1 cores per cpu, 1 threads per core) family 6 model 44 stepping 0 microcode 0x0, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, aes, clmul, tsc, tscinvbit, tscinv, clflush

Am I missing something here?

Loading

@gerard-ziemski
Copy link
Author

@gerard-ziemski gerard-ziemski commented Mar 19, 2021

Thank you for the review Dan!

In the bug report, @dholmes-ora mentions two different places where
"(EMULATED)" should show up. This change covers the first:

Host: MacBookPro17,1 x86_64 2400 MHz, 8 cores, 16G, Darwin 20.3.0
becomes
Host: MacBookPro17,1 x86_64 (EMULATED) 2400 MHz, 8 cores, 16G, Darwin 20.3.0

but I don't think that it covers the second:

CPU: total 8 (initial active 8) (1 cores per cpu, 1 threads per core) family 6 model 44 stepping 0 microcode 0x0, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, aes, clmul, tsc, tscinvbit, tscinv, clflush
becomes:
CPU: (EMULATED) total 8 (initial active 8) (1 cores per cpu, 1 threads per core) family 6 model 44 stepping 0 microcode 0x0, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, aes, clmul, tsc, tscinvbit, tscinv, clflush

Am I missing something here?

I wasn't sure the 2nd case was needed.

The 1st case shows the architecture, so it's clearly needed there.

In the 2nd case we just show CPU features, with no explicit mention of the architecture, so I didn't think it was 100% relevant there.

Is the opinion here that we need to mark it as EMULATED in both of these cases?

Loading

@dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Mar 19, 2021

I would be okay if the second case was not tagged with "(EMULATED)", but
since @dholmes-ora made that comment in the bug report, you should wait
to hear from him.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 19, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 20/03/2021 12:56 am, Gerard Ziemski wrote:

On Thu, 18 Mar 2021 19:03:26 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

Please review this simple enhancement where we indicate in hs_err log file whether the CPU is being emulated. Right now the only use case is for Apple's M1 aarch64 running x64 code, i.e. "Rosetta" emulation.

This enhancement will insert `(EMULATED)` label after we print host CPU type, ex:

`Host: Oracles-MacBook-Pro-16.local, MacBookPro16,1 x86_64 2600 MHz (EMULATED), 12 cores, 32G, Darwin 19.6.0, macOS 10.15.7 (19H114)`

In the bug report, @dholmes-ora mentions two different places where
"(EMULATED)" should show up. This change covers the first:

Host: MacBookPro17,1 x86_64 2400 MHz, 8 cores, 16G, Darwin 20.3.0

becomes

Host: MacBookPro17,1 x86_64 (EMULATED) 2400 MHz, 8 cores, 16G, Darwin 20.3.0

but I don't think that it covers the second:

CPU: total 8 (initial active 8) (1 cores per cpu, 1 threads per core) family 6 model 44 stepping 0 microcode 0x0, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, aes, clmul, tsc, tscinvbit, tscinv, clflush

becomes:

CPU: (EMULATED) total 8 (initial active 8) (1 cores per cpu, 1 threads per core) family 6 model 44 stepping 0 microcode 0x0, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, aes, clmul, tsc, tscinvbit, tscinv, clflush

Am I missing something here?

Thank you for the review Dan!

In the bug report, @dholmes-ora mentions two different places where
"(EMULATED)" should show up. This change covers the first:

Host: MacBookPro17,1 x86_64 2400 MHz, 8 cores, 16G, Darwin 20.3.0
becomes
Host: MacBookPro17,1 x86_64 (EMULATED) 2400 MHz, 8 cores, 16G, Darwin 20.3.0

but I don't think that it covers the second:

CPU: total 8 (initial active 8) (1 cores per cpu, 1 threads per core) family 6 model 44 stepping 0 microcode 0x0, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, aes, clmul, tsc, tscinvbit, tscinv, clflush
becomes:
CPU: (EMULATED) total 8 (initial active 8) (1 cores per cpu, 1 threads per core) family 6 model 44 stepping 0 microcode 0x0, cmov, cx8, fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, aes, clmul, tsc, tscinvbit, tscinv, clflush

Am I missing something here?

I wasn't sure the 2nd case was needed.

The 1st case shows the architecture, so it's clearly needed there.

In the 2nd case we just show CPU features, with no explicit mention of the architecture, so I didn't think it was 100% relevant there.

It is perhaps an oversight that the actual architecture moniker is not
mentioned in that line, but " family 6 model 44 " refers to the CPU that
is actually being emulated in this case so we need to know that this
line is for the emulated x64 CPU not the physical ARM cpu.

Is the opinion here that we need to mark it as `EMULATED` in both of these cases?

Yes please.

Thanks,
David

Loading

@openjdk openjdk bot removed the rfr label Mar 22, 2021
@openjdk openjdk bot added the rfr label Mar 22, 2021
char emulated[16] = "\0";
#ifdef __APPLE__
if (VM_Version::is_cpu_emulated()) {
strcpy(emulated, " (EMULATED)");
Copy link
Member

@dcubed-ojdk dcubed-ojdk Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think code checkers will complain about using strcpy() here.
I believe that strncpy() is preferred.

Loading

Copy link
Contributor

@vidmik vidmik Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps:

const char* emulated = "";
#ifdef APPLE
if (VM_Version::is_cpu_emulated()) {
emulated = " (EMULATED)";
}
#endif
...

?

Loading

Copy link
Author

@gerard-ziemski gerard-ziemski Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will work very nicely, thank you Mikael!

Loading

@gerard-ziemski
Copy link
Author

@gerard-ziemski gerard-ziemski commented Mar 23, 2021

Do I need CSR for this fix?

Loading

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up!

I don't think you need a CSR for this change.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@gerard-ziemski This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8261966: macOS M1: report in hs_err log if we are running x86 code in emulation mode (Rosetta)

Reviewed-by: dcubed, mikael, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 85 new commits pushed to the master branch:

  • 0b2aa1b: 8263978: Clarify why 0 argument is ignored in SecureRandom::setSeed
  • 15bcf6d: 8264055: backout JDK-8248904 in order to resubmit with additional attribution.
  • 2425462: 8263903: Use Cleaner instead of finalize to auto stop Timer thread
  • 35102cb: 8263992: Remove dead code NativeLookup::base_library_lookup
  • 91d86e6: 8263572: Output from jstack mixed mode is misaligned
  • 47ef038: 8263905: Remove finalize methods for SocketInput/OutputStream
  • 1c9817b: 8261479: CDS runtime code should check exceptions
  • 087c8bf: 8264041: Incorrect comments for ParallelCompactData::summarize_dense_prefix
  • c087f3e: 8263995: Incorrect double-checked locking in Types.arraySuperType()
  • d7268fa: 8251942: PrintStream specification is not clear which flush method is automatically invoked
  • ... and 75 more: https://git.openjdk.java.net/jdk/compare/21db0f6768d086f49e0ab1265c274b8917c23ebd...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Mar 23, 2021
vidmik
vidmik approved these changes Mar 23, 2021
@gerard-ziemski
Copy link
Author

@gerard-ziemski gerard-ziemski commented Mar 23, 2021

Thank you Dan and Mikael for the feedback and reviews!

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

LGTM too!

Thanks,
David

Loading

@gerard-ziemski
Copy link
Author

@gerard-ziemski gerard-ziemski commented Mar 24, 2021

Thank you David.

Loading

@gerard-ziemski
Copy link
Author

@gerard-ziemski gerard-ziemski commented Mar 24, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 24, 2021
@openjdk openjdk bot added integrated and removed ready labels Mar 24, 2021
@openjdk openjdk bot removed the rfr label Mar 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 24, 2021

@gerard-ziemski Since your change was applied there have been 100 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 4d8e986.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@gerard-ziemski gerard-ziemski deleted the JDK-8261966 branch Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants