[PATCH] D119296: KCFI sanitizer

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Mostly looks good.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2296
+  // functions, which means it's safe to skip unusual names. Subset of
+  // MCAsmInfo::isAcceptableChar() and MCAsmInfoXCOFF::isAcceptableChar().
+  for (const char &C : Name) {
----------------
Use `llvm::all_of` or `llvm::any_of`


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:332
+
+  Register AddrReg = MI.getOperand(0).getReg();
+
----------------
Add const. Delete blank line after the declaration.


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:350
+
+  int64_t Type = MI.getOperand(1).getImm();
+  EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::MOVKWi)
----------------
Add const 


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:384
+  unsigned AddrIndex;
+
+  switch (AddrReg.id()) {
----------------
delete blank line


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:401
+  EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::BRK).addImm(ESR));
+
+  OutStreamer->emitLabel(Pass);
----------------
delete blank line


================
Comment at: llvm/lib/Target/AArch64/AArch64KCFI.cpp:62
+  unsigned CheckOpc = AArch64::KCFI_CHECK;
+
+  switch (MBBI->getOpcode()) {
----------------
delete blank line


================
Comment at: llvm/lib/Target/AArch64/AArch64KCFI.cpp:103
+  const Module *M = MF.getMMI().getModule();
+
+  if (!M->getModuleFlag("kcfi"))
----------------
delete blank line

llvm-project doesn't typically add a blank line after a variable declaration. please fix other places in this patch.


================
Comment at: llvm/lib/Target/AArch64/AArch64KCFI.cpp:111
+  bool Changed = false;
+
+  for (MachineBasicBlock &MBB : MF) {
----------------
delete blank line



================
Comment at: llvm/lib/Target/AArch64/AArch64KCFI.cpp:117
+      if (!MII->isCall() || !MII->getCFIType())
+        continue;
+
----------------
don't use early return if the `then` part is simple.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:1183
+  } else if (Info.CFIType)
+    MIB->setCFIType(MF, Info.CFIType->getZExtValue());
 
----------------
```
 } else if (Info.CFIType) {
   ...
 }
```


================
Comment at: llvm/lib/Target/X86/X86KCFI.cpp:101
+  ++NumKCFIChecksAdded;
+
+  return true;
----------------
delete blank line


================
Comment at: llvm/lib/Target/X86/X86KCFI.cpp:107
+  const Module *M = MF.getMMI().getModule();
+
+  if (!M->getModuleFlag("kcfi"))
----------------
	
MaskRayUnsubmittedDone
delete blank line


================
Comment at: llvm/lib/Target/X86/X86KCFI.cpp:115
+  bool Changed = false;
+
+  for (MachineBasicBlock &MBB : MF) {
----------------
delete blank line


================
Comment at: llvm/lib/Target/X86/X86KCFI.cpp:121
+      if (!MII->isCall() || !MII->getCFIType())
+        continue;
+
----------------
Fix this in a way similar to AArch64.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119296/new/

https://reviews.llvm.org/D119296