Skip to content

[JITLink][AArch32] Run all error unittests through the main entrypoints applyFixup() and readAddend() #72091

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

Conversation

weliveindetail
Copy link
Member

@weliveindetail weliveindetail commented Nov 13, 2023

First commit: Reading implicit addend from a relocation site doesn't require a complete LinkGraph edge. The operation is independent from TargetSymbol, but constructing an Edge instance required one. This patch fixes the inconsistency and simplifies some setup code from the error unittests.

The second commit is in preparation for the Arm/Thumb/Data helper functions to be turned into implementation details. Exposing them in the API causes unfortunate inconsistencies that we don't want to error-check all the time, e.g. passing Thumb_Call to readAddendArm().

…unctions

Reading implicit addend from a relocation site doesn't require a complete
LinkGraph edge. It's independent from the TargetSymbol, but constructing an
edge instance required one. This patch fixes the inconsistency and simplifies
some setup code from the error unittests.
…ts applyFixup() and readAddend()

This patch is in preparation for the Arm/Thumb/Data helper functions to be
turned into implementation details. Exposing them in the API causes unfortunate
inconsistencies that we don't want to error-check all the time, e.g. passing
Thumb_Call to readAddendArm().
@weliveindetail weliveindetail requested a review from eymay November 13, 2023 08:53
@weliveindetail weliveindetail changed the title [JITLink][AArch32] Drop target symbol requirement from readAddend() function [JITLink][AArch32] Run all error unittests through the main entrypoints applyFixup() and readAddend() Nov 13, 2023
Copy link
Contributor

@eymay eymay left a comment

Choose a reason for hiding this comment

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

Thanks this removed coupling to Edge. LGTM!

@weliveindetail weliveindetail merged commit 808caa9 into llvm:main Nov 15, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…lvm#72091)

Reading implicit addend from a relocation site doesn't require a complete
`LinkGraph` edge. The operation is independent from `TargetSymbol`,
but constructing an `Edge` instance required one. This patch fixes the
inconsistency and simplifies some setup code from the error unittests.

Furthermore this patch prepares for the `Arm`/`Thumb`/`Data` helper
functions to be turned into implementation details. Exposing them in the
API causes unfortunate inconsistencies that we don't want to error-check
all the time, e.g. passing `Thumb_Call` to `readAddendArm()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants