[PATCH] D88194: [X86] CET endbr enhance

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Warning: I had no idea what "CET" means before seeing this patch, so feel free to discount my feedback.
The patch title and description are not clear to me.
Is the goal of this patch to change *the compiler* binary itself? (rather than an application binary)
It would be good to pre-commit a minimal test at least so we can see exactly what asm *change* is introduced by this patch.



================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:209
 
+    bool EndbrImmAntiAttack(SDNode *N);
     bool foldOffsetIntoAddress(uint64_t Offset, X86ISelAddressMode &AM);
----------------
Formatting: function name should be 'lowerCamelCase'


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1344
+
+  uint8_t Bytes[] = {0x26, 0x2e, 0x36, 0x3e, 0x64,
+                     0x65, 0x66, 0x67, 0xf0, 0xf2};
----------------
Bytes -> OptionalPrefixBytes ?


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1374
+// into multiple operations, let it not shows in the binary.
+bool X86DAGToDAGISel::EndbrImmAntiAttack(SDNode *N) {
+  unsigned Opc = N->getMachineOpcode();
----------------
The function name does not describe what it actually does.
Would "obscureEndbrOpcodeImmediate" be more accurate?


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1386-1387
+    case X86::MOV32ri: // *32ri
+      Idx = 0;
+      LLVM_FALLTHROUGH;
+    case X86::ADC32ri:
----------------
This code structure with fallthrough is difficult to follow. Create a separate static helper function that just returns the constant operand index for a given opcode?


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

https://reviews.llvm.org/D88194