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

Fix link bug in insnCodeGen::loadImmIntoReg on aarch64 #1405

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

hainest
Copy link
Contributor

@hainest hainest commented Apr 3, 2023

The function template has a separate declaration and "definition" AND is called from translation units where neither is located. This only happens to work correctly when the TUs are linked in a specific order.

The function template has a separate declaration and "definition" AND is
called from translation units where neither is located. This only happens
to work correctly when the TUs are linked in a specific order.
@hainest hainest added bug ARMv8 dyninstAPI This issue is directly related to dyninstAPI labels Apr 3, 2023
@hainest hainest requested a review from kupsch April 3, 2023 19:14
@hainest hainest self-assigned this Apr 3, 2023
@hainest
Copy link
Contributor Author

hainest commented Apr 3, 2023

@kupsch This is now building on Zeroah. Now running test suite.

Copy link
Contributor

@kupsch kupsch left a comment

Choose a reason for hiding this comment

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

Looks good, only suggestion is to eliminate the unnecessary cast.

return;
}
else {
restoreGPRtoGPR(gen, ra, dest);
}
} else {
insnCodeGen::loadImmIntoReg<long int>(gen, dest, 0);
insnCodeGen::loadImmIntoReg(gen, dest, static_cast<Address>(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but I was trying to make it more obvious what was happening at the callsite since the template parameter should have been an Address. I guess I could have used Address{0}, but I'm going to say it's still better than what was there.

@hainest hainest merged commit 2f7af10 into master Apr 4, 2023
@hainest hainest deleted the fix_aarch64_insnCodeGen_loadImmIntoReg_link_bug branch April 4, 2023 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMv8 bug dyninstAPI This issue is directly related to dyninstAPI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants