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

Migrate processor/feature detection from OpenJ9 to OMR Part 1 #4503

Merged
merged 1 commit into from Dec 7, 2019

Conversation

@AidanHa
Copy link
Contributor

AidanHa commented Oct 23, 2019

OpenJ9 port library has processor and feature detections that can be sunk down to OMR. This can be divided into 4 steps:
1: Creation of processor and feature detection in OMR (from OpenJ9).
2: Implementation of the new feature throughout out OMR.
3: Implementation of the new feature throughout OpenJ9.
4: Deletion of the processor detection in OpenJ9 port library.

This PR serves to implement step 1 of the list above. This PR includes the following:

  • Move and rename necessary port library macros from OpenJ9 to OMR's omrport.h (Commit 1)
  • Move and implement processor/feature detection for windows systems[WIP] (Commit 2)
  • Move and implement processor/feature detection for unix systems (x86, Power, AIX, Z) (Commit 3)
  • Move and test the respective port tests that helps verify that processor detection works (Commit 4)

Issue: #4339

@AidanHa AidanHa force-pushed the AidanHa:Issue4339 branch from d98afc1 to dc3700a Oct 29, 2019
@AidanHa

This comment has been minimized.

Copy link
Contributor Author

AidanHa commented Oct 29, 2019

These functions have been tested on Z, Power and X86 for Windows and Unix systems! I think we're ready for a full review!

@Leonardo2718

This comment has been minimized.

Copy link
Contributor

Leonardo2718 commented Oct 29, 2019

@genie-omr build all

@AidanHa AidanHa force-pushed the AidanHa:Issue4339 branch 2 times, most recently from 978156e to ffa758b Nov 1, 2019
@AidanHa

This comment has been minimized.

Copy link
Contributor Author

AidanHa commented Nov 1, 2019

@Leonardo2718 Could you run the PR builds again? There were some build issues on LinuxPPC and AIX, and I've pushed changes to fix these issues.

@AidanHa

This comment has been minimized.

Copy link
Contributor Author

AidanHa commented Nov 1, 2019

Also FYI, I've broken down the PR into several commits for clarity, but will merge them when the changes are approved!

@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Nov 1, 2019

@genie-omr build all

@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Nov 1, 2019

Aidan, for the new tests that you're adding, you should just use core gtest functionality, rather than using the port test helpers. Specifically, don't call reportTestEntry (or exit), and use ASSERT/EXPECT, rather than calling outputErrorMessage. As well, your tests should be in TEST blocks (I'm looking at testGetProcessorDescription).

@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Nov 1, 2019

This looks like a pretty straightforward copy/paste/rename job, so if you want to do clean up in a later PR, I would be fine with that, it's up to you.

@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Nov 1, 2019

The AIX failure is interesting, I don't think it's related to your change. EDIT: actually, it seems it is your problem.

@genie-omr build aix

The x86-64 build, however...

[2019-11-01T14:46:05.106Z] si.cpp:132:11: error: variable ‘rc’ set but not used [-Werror=unused-but-set-variable]
[2019-11-01T14:46:05.106Z]   uint32_t rc = 0;

The rc in question.

port/common/omrsysinfo.c Outdated Show resolved Hide resolved
port/win32/omrsysinfo.c Outdated Show resolved Hide resolved
@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Nov 4, 2019

@genie-omr build aix

@AidanHa

This comment has been minimized.

Copy link
Contributor Author

AidanHa commented Nov 4, 2019

Aidan, for the new tests that you're adding, you should just use core gtest functionality, rather than using the port test helpers. Specifically, don't call reportTestEntry (or exit), and use ASSERT/EXPECT, rather than calling outputErrorMessage. As well, your tests should be in TEST blocks (I'm looking at testGetProcessorDescription).

I removed testGetProcessorDescription and added the tests in a TEST block. Any reason why we want to be using ASSERTS rather than outputErrorMessage? When writing the tests, I was following the format of the other tests on si.cpp

@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Nov 4, 2019

Wherever possible, the port test helpers are being replaced with gtest. I opened an issue #4530. Any new tests should just be using gtest.

port/zos390/omrportpg.h Outdated Show resolved Hide resolved
setFeature(desc, OMRPORT_PPC_FEATURE_POWER4);
}
#endif /* !defined(J9OS_I5_V6R1) */
#if !defined(J9OS_I5_V7R2) && !defined(J9OS_I5_V6R1)

This comment has been minimized.

Copy link
@rwy0717

rwy0717 Nov 4, 2019

Member

These are never defined in OMR, and the code below fails to compile on aix 6.1.

@AidanHa AidanHa force-pushed the AidanHa:Issue4339 branch from ffa758b to cdde3bb Nov 4, 2019
@AidanHa AidanHa requested a review from 0xdaryl as a code owner Nov 4, 2019
fvtest/porttest/si.cpp Outdated Show resolved Hide resolved
fvtest/porttest/si.cpp Outdated Show resolved Hide resolved
@AidanHa AidanHa force-pushed the AidanHa:Issue4339 branch from 15b9bc5 to c0f7cdd Nov 8, 2019
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
fvtest/porttest/si.cpp Outdated Show resolved Hide resolved
port/common/omrsysinfo_helpers.c Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/common/omrsysinfo_helpers.h Outdated Show resolved Hide resolved
@AidanHa

This comment has been minimized.

Copy link
Contributor Author

AidanHa commented Nov 11, 2019

Changes are fixed! @rwy0717 When you are done reviewing, I'll merge all of my changes into 1 single commit.

@AidanHa AidanHa force-pushed the AidanHa:Issue4339 branch from 3bc26c9 to 9f2daf9 Nov 12, 2019
@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Nov 22, 2019

@genie-omr build all

@AidanHa AidanHa force-pushed the AidanHa:Issue4339 branch 4 times, most recently from 6856f77 to 0a8acf8 Nov 25, 2019
@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Nov 28, 2019

@genie-omr build all

@AidanHa AidanHa force-pushed the AidanHa:Issue4339 branch 4 times, most recently from 6b559c6 to 5a0de8b Nov 30, 2019
@fjeremic

This comment has been minimized.

Copy link
Contributor

fjeremic commented Dec 3, 2019

Let's make sure to squash the review addressing commits here before merging (after committer assigned has approved).

@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Dec 7, 2019

@genie-omr build all

@rwy0717
rwy0717 approved these changes Dec 7, 2019
@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Dec 7, 2019

@fjeremic when you say "Let's make sure to squash", are you asking @AidanHa to squash, or are you asking me to use the "squash and merge" button? Either way, I think this PR is ready to go.

Move required macros, enums and structs from OpenJ9 to OMR's port library
Add function signatures for processor/feature detection to OMR
Implement processor detection on Windows
Add processor and feature detection for Unix Includes Linux,
AIX PPC and ZOS systems
Add test to verify processor/feature detection.

Signed-off-by: Aidan Ha <qbha@edu.uwaterloo.ca>
@AidanHa AidanHa force-pushed the AidanHa:Issue4339 branch from 5a0de8b to 0f824c6 Dec 7, 2019
@AidanHa

This comment has been minimized.

Copy link
Contributor Author

AidanHa commented Dec 7, 2019

I've gone ahead and squashed my commits!

@rwy0717

This comment has been minimized.

Copy link
Member

rwy0717 commented Dec 7, 2019

@genie-omr build all

@rwy0717 rwy0717 merged commit 747e6a4 into eclipse:master Dec 7, 2019
14 checks passed
14 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/eclipse-omr/pr/aix_ppc-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_390-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_aarch64 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_arm Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_ppc-64_le_gcc Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_x86 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_x86-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/linux_x86-64_cmprssptrs Build finished.
Details
continuous-integration/eclipse-omr/pr/osx_x86-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/win_x86-64 Build finished.
Details
continuous-integration/eclipse-omr/pr/zos_390-64 Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.