-
Notifications
You must be signed in to change notification settings - Fork 15.7k
[BOLT][BTI] Fix assertions checking getNumOperands #174600
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
|
@llvm/pr-subscribers-bolt Author: Gergely Bálint (bgergely0) ChangesUse MCPlus::getNumPrimeOperands to check number of non-annotation Full diff: https://github.com/llvm/llvm-project/pull/174600.diff 1 Files Affected:
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index 03fb4ddc2f238..a30799d5f45d3 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -2818,7 +2818,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
// x16 or x17. If the operand is not x16 or x17, it can be accepted by a BTI
// j or BTI jc (and not BTI c).
if (isIndirectBranch(Call)) {
- assert(Call.getNumOperands() == 1 &&
+ assert(MCPlus::getNumPrimeOperands(Call) == 1 &&
"Indirect branch needs to have 1 operand.");
assert(Call.getOperand(0).isReg() &&
"Indirect branch does not have a register operand.");
@@ -2856,7 +2856,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
// x16 or x17. If the operand is not x16 or x17, it can be accepted by a
// BTI j or BTI jc (and not BTI c).
if (isIndirectBranch(Call)) {
- assert(Call.getNumOperands() == 1 &&
+ assert(MCPlus::getNumPrimeOperands(Call) == 1 &&
"Indirect branch needs to have 1 operand.");
assert(Call.getOperand(0).isReg() &&
"Indirect branch does not have a register operand.");
|
peterwaller-arm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix sounds reasonable though it makes me wonder if other uses of getNumOperands are wrong: what about the IsExplicitBTI definition, for example?
that's a good point. Let me check other uses, and add a few relevant tests. |
Several BTI-related functions are checking that a call MCInst has one non-annotation operand. This patch changes these checks to use MCPlus::getNumPrimeOperands, instead of getNumOperands.
15dceca to
da54184
Compare
paschalis-mpeis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks Gergely, can you update the commit message to note that unit tests were updated to cover this?

Several BTI-related functions are checking that a call MCInst has one
non-annotation operand.
This patch changes these checks to use MCPlus::getNumPrimeOperands,
instead of getNumOperands.
Testing: added annotations to existing gtests to serve as regression tests.
These now also explicitly check getNumOperands and getNumPrimeOperands
usage on the annotated MCInsts.