Skip to content

Add a new symbol flag to mark addresses within method bounds #6230

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
Merged
0xdaryl merged 1 commit into from
Nov 9, 2021
Merged

Add a new symbol flag to mark addresses within method bounds #6230

0xdaryl merged 1 commit into from
Nov 9, 2021

Conversation

@dmitry-ten
Copy link
Contributor

@dmitry-ten dmitry-ten commented Nov 5, 2021

A new flag AddressWithinMethodBounds indicates that an address
stored in a static symbol is inside method bounds, and can be
accessed using RIP addressing during an out-of-process compilation.

@dmitry-ten dmitry-ten force-pushed the address-within-bounds branch from 3838ef7 to 6858b5f Compare November 5, 2021 16:08
@dmitry-ten
Copy link
Contributor Author

dmitry-ten commented Nov 5, 2021

Related to eclipse-openj9/openj9#13458.
@mpirvu could you review?
You can notice that the flag value is reused in other places, but so are other static symbol flag values, so it probably means they cannot be both true at the same time.

mpirvu
mpirvu reviewed Nov 5, 2021
View reviewed changes
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

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

LGTM. I cannot tell what the best location for the new flag is (between flags and flags2), so maybe someone with more OMR experience can pitch in.

@mpirvu
Copy link
Contributor

mpirvu commented Nov 8, 2021

@0xdaryl Could you please review this PR or delegate? Thanks

@0xdaryl
Copy link
Contributor

0xdaryl commented Nov 8, 2021

  • Can you add a comment in the code documenting what the semantics of this flag are?

  • Perhaps it should be called "StaticAddressWithinMethodBounds" if you are restricting this to static symbols. There are other non-static symbols where this flag could apply. For example, LabelSymbols, which are always within a method's bounds but are not static symbols.

  • Did you verify that all the other static symbols that we create have this new flag set appropriately for that kind of symbol?

@dmitry-ten
Add a new symbol flag to mark addresses within method bounds
f797475 
A new flag StaticAddressWithinMethodBounds indicates that an address
stored in a static symbol is inside method bounds, and can be
accessed using RIP addressing during an out-of-process compilation.

Signed-off-by: Dmitry Ten <Dmitry.Ten@ibm.com>
@dmitry-ten dmitry-ten force-pushed the address-within-bounds branch from 6858b5f to f797475 Compare November 9, 2021 08:29
@dmitry-ten
Copy link
Contributor Author

dmitry-ten commented Nov 9, 2021

Added comment and renamed the flag.

Marius and I looked at places where NotDataAddress flag is set for static symbols. There seem to be just 2 locations where the address is within method bounds. For other static addresses, they describe some data, so they are most likely not going to be within method bounds.

@0xdaryl
Copy link
Contributor

0xdaryl commented Nov 9, 2021

Jenkins build all

@0xdaryl 0xdaryl self-assigned this Nov 9, 2021
@0xdaryl 0xdaryl merged commit 02c5106 into eclipse-omr:master Nov 9, 2021
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

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

@vijaysun-omr vijaysun-omr Awaiting requested review from vijaysun-omr vijaysun-omr is a code owner

+1 more reviewer

@mpirvu mpirvu mpirvu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@0xdaryl 0xdaryl

Labels

arch:x86 comp:compiler

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dmitry-ten @mpirvu @0xdaryl