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.

Sign up for GitHub

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

Jump to bottom

Misc fixes #7098

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Misc fixes #7098

merged 2 commits into from
Aug 24, 2023

Conversation

mstoodle
Copy link
Contributor

Two changes, both of which have potential to break builds:

  1. the tril project doesn't currently check its dependence on testcompiler so now it will assert if not also enabled (not sure our build scripts currently do this)
  2. set the linkage on all JitBuilder method symbols to use TR_System convention (currently uses default which, at least on some platforms, is TR_Private).

@mstoodle
tril project should require OMR_COMPILER_TEST to be enabled
52e4b6a 
Some nondeterminism here if tril is built first then some
needed header files needed aren't present and the build will
fail. This commit ensures that OMR_COMPILER_TEST has been
set if the fvtest/tril project is to be enabled.

Signed-off-by: Mark Stoodley <mstoodle@ca.ibm.com>
@mstoodle
JitBuilder method symbols should use TR_System linkage convention
0ab1886 
Discovered on Aarch64 but it's something we should do consistently
for all platforms.

Signed-off-by: Mark Stoodley <mstoodle@ca.ibm.com>
@mstoodle
Copy link
Contributor Author

Hoping these won't broadly break the builds as they're both Good Thing(tm)s to do...

@mstoodle
Copy link
Contributor Author

jenkins build all

@mstoodle
Copy link
Contributor Author

I have run this PR locally on MacOS Aarch64, so I think this should be good to go. Maybe @0xdaryl could review, when you have a moment, please?

@AdamBrousseau
Copy link
Contributor

Sorry, going to hijack your PR to test that amac has been added to all. All platforms in this PR are green as of now.

@AdamBrousseau
Copy link
Contributor

jenkins build all

@AdamBrousseau
Copy link
Contributor

Looks good.

@0xdaryl
Copy link
Contributor

0xdaryl commented Aug 24, 2023

I remember coming across the default TR_Private vs TR_System difference a few years ago and tried to fix it in the obvious place [1] then (since there really isn't a notion of "private linkage" in OMR). However, I recall running into complications (perhaps in OpenJ9) that were discouraging enough to prevent me from proceeding, but I really don't remember many details beyond that. I do remember thinking this should be corrected with a refactoring of the MethodSymbol/ResolvedMethodSymbol hierarchies.

Your approach to correct it at the call-site seems like it would be a safer fix and unlikely to cause problems in OpenJ9.

[1] https://github.com/eclipse/omr/blob/2148df2e8c76592cf7da37019e0060eab3c202d5/compiler/il/OMRResolvedMethodSymbol.cpp#L111

@0xdaryl 0xdaryl self-assigned this Aug 24, 2023
@0xdaryl 0xdaryl merged commit 9659cbc into eclipse-omr:master Aug 24, 2023
5 checks passed
@mstoodle mstoodle deleted the miscFixes branch August 31, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xdaryl 0xdaryl 0xdaryl approved these changes

@Leonardo2718 Leonardo2718 Awaiting requested review from Leonardo2718

@fjeremic fjeremic Awaiting requested review from fjeremic

Assignees

@0xdaryl 0xdaryl

Labels
comp:jitbuilder comp:tril
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mstoodle @AdamBrousseau @0xdaryl