Skip to content

Commit ba4b47f

Browse files
bgergely0bgergely0
committed
[BOLT] Add changes from code review (#120064)
1 parent 3bf41a2 commit ba4b47f

File tree

8 files changed

+70
-66
lines changed

8 files changed

+70
-66
lines changed

‎bolt/include/bolt/Core/BinaryFunction.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1654,9 +1654,8 @@ class BinaryFunction {
16541654
Offset = I->first;
16551655
}
16561656
assert(I->first == Offset && "CFI pointing to unknown instruction");
1657-
if (I == Instructions.begin()) {
1657+
if (I == Instructions.begin())
16581658
return {};
1659-
}
16601659

16611660
--I;
16621661
while (I != Instructions.begin() && BC.MIB->isNoop(I->second)) {
@@ -1673,7 +1672,7 @@ class BinaryFunction {
16731672
I--;
16741673

16751674
switch (CFIOpcode) {
1676-
case dwarf::DW_CFA_GNU_window_save:
1675+
case dwarf::DW_CFA_AARCH64_negate_ra_state:
16771676
BC.MIB->setNegateRAState(I->second);
16781677
break;
16791678
case dwarf::DW_CFA_remember_state:

‎bolt/include/bolt/Core/MCPlusBuilder.h

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,49 +1275,49 @@ class MCPlusBuilder {
12751275
/// Return true if the instruction is a tail call.
12761276
bool isTailCall(const MCInst &Inst) const;
12771277

1278-
/// Stores NegateRAState annotation on Inst.
1278+
/// Stores NegateRAState annotation on \p Inst.
12791279
void setNegateRAState(MCInst &Inst) const;
12801280

1281-
/// Return true if Inst has NegateRAState annotation.
1281+
/// Return true if \p Inst has NegateRAState annotation.
12821282
bool hasNegateRAState(const MCInst &Inst) const;
12831283

1284-
/// Sets RememberState annotation on Inst.
1284+
/// Sets RememberState annotation on \p Inst.
12851285
void setRememberState(MCInst &Inst) const;
12861286

1287-
/// Return true if Inst has RememberState annotation.
1287+
/// Return true if \p Inst has RememberState annotation.
12881288
bool hasRememberState(const MCInst &Inst) const;
12891289

1290-
/// Stores RestoreState annotation on Inst.
1290+
/// Stores RestoreState annotation on \p Inst.
12911291
void setRestoreState(MCInst &Inst) const;
12921292

1293-
/// Return true if Inst has RestoreState annotation.
1293+
/// Return true if \p Inst has RestoreState annotation.
12941294
bool hasRestoreState(const MCInst &Inst) const;
12951295

1296-
/// Stores RA Signed annotation on Inst.
1296+
/// Stores RA Signed annotation on \p Inst.
12971297
void setRASigned(MCInst &Inst) const;
12981298

1299-
/// Return true if Inst has Signed RA annotation.
1299+
/// Return true if \p Inst has Signed RA annotation.
13001300
bool isRASigned(const MCInst &Inst) const;
13011301

1302-
/// Stores RA Signing annotation on Inst.
1302+
/// Stores RA Signing annotation on \p Inst.
13031303
void setRASigning(MCInst &Inst) const;
13041304

1305-
/// Return true if Inst has Signing RA annotation.
1305+
/// Return true if \p Inst has Signing RA annotation.
13061306
bool isRASigning(const MCInst &Inst) const;
13071307

1308-
/// Stores Authenticating annotation on Inst.
1308+
/// Stores Authenticating annotation on \p Inst.
13091309
void setAuthenticating(MCInst &Inst) const;
13101310

1311-
/// Return true if Inst has Authenticating annotation.
1311+
/// Return true if \p Inst has Authenticating annotation.
13121312
bool isAuthenticating(const MCInst &Inst) const;
13131313

1314-
/// Stores RA Unsigned annotation on Inst.
1314+
/// Stores RA Unsigned annotation on \p Inst.
13151315
void setRAUnsigned(MCInst &Inst) const;
13161316

1317-
/// Return true if Inst has Unsigned RA annotation.
1317+
/// Return true if \p Inst has Unsigned RA annotation.
13181318
bool isRAUnsigned(const MCInst &Inst) const;
13191319

1320-
/// Return true if Inst doesn't have any annotation related to RA state.
1320+
/// Return true if \p Inst doesn't have any annotation related to RA state.
13211321
bool isRAStateUnknown(const MCInst &Inst) const;
13221322

13231323
/// Return true if the instruction is a call with an exception handling info.

‎bolt/include/bolt/Passes/InsertNegateRAStatePass.h

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
//===- bolt/Passes/InsertNegateRAStatePass.cpp ----------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
//
9+
// This file implements the InsertNegateRAStatePass class.
10+
//
11+
//===----------------------------------------------------------------------===//
112
#ifndef BOLT_PASSES_INSERT_NEGATE_RA_STATE_PASS
213
#define BOLT_PASSES_INSERT_NEGATE_RA_STATE_PASS
314

@@ -16,7 +27,15 @@ class InsertNegateRAState : public BinaryFunctionPass {
1627
/// Pass entry point
1728
Error runOnFunctions(BinaryContext &BC) override;
1829
void runOnFunction(BinaryFunction &BF);
30+
31+
private:
32+
/// Loops over all instructions and adds OpNegateRAState CFI
33+
/// after any pointer signing or authenticating instructions.
34+
/// Returns true, if any OpNegateRAState CFIs were added.
1935
bool addNegateRAStateAfterPacOrAuth(BinaryFunction &BF);
36+
/// Because states are tracked as MCAnnotations on individual instructions,
37+
/// newly inserted instructions do not have a state associated with them.
38+
/// New states are "inherited" from the last known state.
2039
void fixUnknownStates(BinaryFunction &BF);
2140
};
2241

‎bolt/include/bolt/Passes/MarkRAStates.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
//===- bolt/Passes/MarkRAStates.cpp ---------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
//
9+
// This file implements the MarkRAStates class.
10+
//
11+
//===----------------------------------------------------------------------===//
112
#ifndef BOLT_PASSES_MARK_RA_STATES
213
#define BOLT_PASSES_MARK_RA_STATES
314

‎bolt/lib/Core/Exceptions.cpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -640,16 +640,16 @@ bool CFIReaderWriter::fillCFIInfoFor(BinaryFunction &Function) const {
640640
BC.errs() << "BOLT-WARNING: DW_CFA_MIPS_advance_loc unimplemented\n";
641641
return false;
642642
case DW_CFA_GNU_window_save:
643-
// DW_CFA_GNU_window_save and DW_CFA_GNU_NegateRAState just use the same
644-
// id but mean different things. The latter is used in AArch64.
643+
// DW_CFA_GNU_window_save and DW_CFA_AARCH64_negate_ra_state just use the
644+
// same id but mean different things. The latter is used in AArch64.
645645
if (Function.getBinaryContext().isAArch64()) {
646-
// Not adding OpNegateRAState since the location they are needed
646+
// The location OpNegateRAState CFIs are needed
647647
// depends on the order of BasicBlocks, which changes during
648-
// optimizations. Instead, an annotation is added to the instruction, to
649-
// mark that the instruction modifies the RA State. The actual state for
650-
// instructions are worked out in MarkRAStates based on these
651-
// annotations.
652-
Function.setInstModifiesRAState(DW_CFA_GNU_window_save, Offset);
648+
// optimizations. Instead of adding OpNegateRAState CFIs, an annotation
649+
// is added to the instruction, to mark that the instruction modifies
650+
// the RA State. The actual state for instructions are worked out in
651+
// MarkRAStates based on these annotations.
652+
Function.setInstModifiesRAState(DW_CFA_AARCH64_negate_ra_state, Offset);
653653
break;
654654
}
655655
if (opts::Verbosity >= 1)

‎bolt/lib/Core/MCPlusBuilder.cpp

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,7 @@ void MCPlusBuilder::setNegateRAState(MCInst &Inst) const {
153153
}
154154

155155
bool MCPlusBuilder::hasNegateRAState(const MCInst &Inst) const {
156-
if (hasAnnotation(Inst, MCAnnotation::kNegateState))
157-
return true;
158-
return false;
156+
return hasAnnotation(Inst, MCAnnotation::kNegateState);
159157
}
160158

161159
void MCPlusBuilder::setRememberState(MCInst &Inst) const {
@@ -164,9 +162,7 @@ void MCPlusBuilder::setRememberState(MCInst &Inst) const {
164162
}
165163

166164
bool MCPlusBuilder::hasRememberState(const MCInst &Inst) const {
167-
if (hasAnnotation(Inst, MCAnnotation::kRememberState))
168-
return true;
169-
return false;
165+
return hasAnnotation(Inst, MCAnnotation::kRememberState);
170166
}
171167

172168
void MCPlusBuilder::setRestoreState(MCInst &Inst) const {
@@ -175,9 +171,7 @@ void MCPlusBuilder::setRestoreState(MCInst &Inst) const {
175171
}
176172

177173
bool MCPlusBuilder::hasRestoreState(const MCInst &Inst) const {
178-
if (hasAnnotation(Inst, MCAnnotation::kRestoreState))
179-
return true;
180-
return false;
174+
return hasAnnotation(Inst, MCAnnotation::kRestoreState);
181175
}
182176

183177
void MCPlusBuilder::setRASigned(MCInst &Inst) const {
@@ -186,9 +180,7 @@ void MCPlusBuilder::setRASigned(MCInst &Inst) const {
186180
}
187181

188182
bool MCPlusBuilder::isRASigned(const MCInst &Inst) const {
189-
if (hasAnnotation(Inst, MCAnnotation::kSigned))
190-
return true;
191-
return false;
183+
return hasAnnotation(Inst, MCAnnotation::kSigned);
192184
}
193185

194186
void MCPlusBuilder::setRASigning(MCInst &Inst) const {
@@ -197,9 +189,7 @@ void MCPlusBuilder::setRASigning(MCInst &Inst) const {
197189
}
198190

199191
bool MCPlusBuilder::isRASigning(const MCInst &Inst) const {
200-
if (hasAnnotation(Inst, MCAnnotation::kSigning))
201-
return true;
202-
return false;
192+
return hasAnnotation(Inst, MCAnnotation::kSigning);
203193
}
204194

205195
void MCPlusBuilder::setAuthenticating(MCInst &Inst) const {
@@ -208,9 +198,7 @@ void MCPlusBuilder::setAuthenticating(MCInst &Inst) const {
208198
}
209199

210200
bool MCPlusBuilder::isAuthenticating(const MCInst &Inst) const {
211-
if (hasAnnotation(Inst, MCAnnotation::kAuthenticating))
212-
return true;
213-
return false;
201+
return hasAnnotation(Inst, MCAnnotation::kAuthenticating);
214202
}
215203

216204
void MCPlusBuilder::setRAUnsigned(MCInst &Inst) const {
@@ -219,16 +207,12 @@ void MCPlusBuilder::setRAUnsigned(MCInst &Inst) const {
219207
}
220208

221209
bool MCPlusBuilder::isRAUnsigned(const MCInst &Inst) const {
222-
if (hasAnnotation(Inst, MCAnnotation::kUnsigned))
223-
return true;
224-
return false;
210+
return hasAnnotation(Inst, MCAnnotation::kUnsigned);
225211
}
226212

227213
bool MCPlusBuilder::isRAStateUnknown(const MCInst &Inst) const {
228-
if (hasAnnotation(Inst, MCAnnotation::kUnsigned) ||
229-
hasAnnotation(Inst, MCAnnotation::kSigned) ||
230-
hasAnnotation(Inst, MCAnnotation::kSigning) ||
231-
hasAnnotation(Inst, MCAnnotation::kAuthenticating))
214+
if (isRAUnsigned(Inst) || isRASigned(Inst) || isRASigning(Inst) ||
215+
isAuthenticating(Inst))
232216
return false;
233217
return true;
234218
}

‎bolt/lib/Passes/InsertNegateRAStatePass.cpp

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,8 @@ namespace bolt {
2727
void InsertNegateRAState::runOnFunction(BinaryFunction &BF) {
2828
BinaryContext &BC = BF.getBinaryContext();
2929

30-
if (BF.getState() == BinaryFunction::State::Empty) {
30+
if (BF.getState() == BinaryFunction::State::Empty)
3131
return;
32-
}
3332

3433
if (BF.getState() != BinaryFunction::State::CFG &&
3534
BF.getState() != BinaryFunction::State::CFG_Finalized) {
@@ -91,13 +90,7 @@ bool InsertNegateRAState::addNegateRAStateAfterPacOrAuth(BinaryFunction &BF) {
9190
for (BinaryBasicBlock &BB : BF) {
9291
for (auto Iter = BB.begin(); Iter != BB.end(); ++Iter) {
9392
MCInst &Inst = *Iter;
94-
if (BC.MIB->isPSign(Inst)) {
95-
Iter = BF.addCFIInstruction(
96-
&BB, Iter + 1, MCCFIInstruction::createNegateRAState(nullptr));
97-
FoundAny = true;
98-
}
99-
100-
if (BC.MIB->isPAuth(Inst)) {
93+
if (BC.MIB->isPSign(Inst) || BC.MIB->isPAuth(Inst)) {
10194
Iter = BF.addCFIInstruction(
10295
&BB, Iter + 1, MCCFIInstruction::createNegateRAState(nullptr));
10396
FoundAny = true;
@@ -111,9 +104,7 @@ void InsertNegateRAState::fixUnknownStates(BinaryFunction &BF) {
111104
BinaryContext &BC = BF.getBinaryContext();
112105
bool FirstIter = true;
113106
MCInst PrevInst;
114-
for (auto BBIt = BF.begin(); BBIt != BF.end(); ++BBIt) {
115-
BinaryBasicBlock &BB = *BBIt;
116-
107+
for (BinaryBasicBlock &BB : BF) {
117108
for (auto It = BB.begin(); It != BB.end(); ++It) {
118109

119110
MCInst &Inst = *It;

‎bolt/lib/Passes/MarkRAStates.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
// - NegateRAState flips the RA State,
1212
// - RememberState pushes the RA State to a stack,
1313
// - RestoreState pops the RA State from the stack.
14-
// These were saved as MCAnnotations on instructions they refer to at CFI
14+
// These are saved as MCAnnotations on instructions they refer to at CFI
1515
// reading (in CFIReaderWriter::fillCFIInfoFor). In this pass, we can work out
1616
// the RA State of each instruction, and save it as new MCAnnotations. The new
1717
// annotations are Signing, Signed, Authenticating and Unsigned. After
@@ -95,9 +95,9 @@ void MarkRAStates::runOnFunction(BinaryFunction &BF) {
9595
continue;
9696
if (Annotation == MCPlus::MCAnnotation::kNegateState)
9797
RAState = !RAState;
98-
if (Annotation == MCPlus::MCAnnotation::kRememberState)
98+
else if (Annotation == MCPlus::MCAnnotation::kRememberState)
9999
RAStateStack.push(RAState);
100-
if (Annotation == MCPlus::MCAnnotation::kRestoreState) {
100+
else if (Annotation == MCPlus::MCAnnotation::kRestoreState) {
101101
RAState = RAStateStack.top();
102102
RAStateStack.pop();
103103
}

0 commit comments

Comments
 (0)