PR feedback

Author: sivadeilra

clang/lib/CodeGen/CGCall.cpp

@@ -2674,7 +2674,8 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
     if (!MSHotPatchFunctions.empty()) {
       bool IsHotPatched = llvm::binary_search(MSHotPatchFunctions, Name);
       if (IsHotPatched)
-        FuncAttrs.addAttribute(llvm::Attribute::MarkedForWindowsHotPatching);
+        FuncAttrs.addAttribute(
+            llvm::Attribute::MarkedForWindowsSecureHotPatching);
     }
   }
 

llvm/include/llvm/Bitcode/LLVMBitCodes.h

@@ -799,7 +799,7 @@ enum AttributeKindCodes {
   ATTR_KIND_SANITIZE_TYPE = 101,
   ATTR_KIND_CAPTURES = 102,
   ATTR_KIND_ALLOW_DIRECT_ACCESS_IN_HOT_PATCH_FUNCTION = 103,
-  ATTR_KIND_MARKED_FOR_WINDOWS_HOT_PATCHING = 104,
+  ATTR_KIND_MARKED_FOR_WINDOWS_SECURE_HOT_PATCHING = 104,
 };
 
 enum ComdatSelectionKindCodes {

llvm/include/llvm/IR/Attributes.td

@@ -390,7 +390,7 @@ def CoroDestroyOnlyWhenComplete : EnumAttr<"coro_only_destroy_when_complete", In
 def CoroElideSafe : EnumAttr<"coro_elide_safe", IntersectPreserve, [FnAttr]>;
 
 /// Function is marked for Windows Hot Patching
-def MarkedForWindowsHotPatching
+def MarkedForWindowsSecureHotPatching
     : EnumAttr<"marked_for_windows_hot_patching", IntersectPreserve, [FnAttr]>;
 
 /// Global variable should not be accessed through a "__ref_" global variable in

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

@@ -2246,8 +2246,8 @@ static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
     return Attribute::Captures;
   case bitc::ATTR_KIND_ALLOW_DIRECT_ACCESS_IN_HOT_PATCH_FUNCTION:
     return Attribute::AllowDirectAccessInHotPatchFunction;
-  case bitc::ATTR_KIND_MARKED_FOR_WINDOWS_HOT_PATCHING:
-    return Attribute::MarkedForWindowsHotPatching;
+  case bitc::ATTR_KIND_MARKED_FOR_WINDOWS_SECURE_HOT_PATCHING:
+    return Attribute::MarkedForWindowsSecureHotPatching;
   }
 }
 

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

@@ -940,8 +940,8 @@ static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
     return bitc::ATTR_KIND_CAPTURES;
   case Attribute::AllowDirectAccessInHotPatchFunction:
     return bitc::ATTR_KIND_ALLOW_DIRECT_ACCESS_IN_HOT_PATCH_FUNCTION;
-  case Attribute::MarkedForWindowsHotPatching:
-    return bitc::ATTR_KIND_MARKED_FOR_WINDOWS_HOT_PATCHING;
+  case Attribute::MarkedForWindowsSecureHotPatching:
+    return bitc::ATTR_KIND_MARKED_FOR_WINDOWS_SECURE_HOT_PATCHING;
   case Attribute::EndAttrKinds:
     llvm_unreachable("Can not encode end-attribute kinds marker.");
   case Attribute::None:

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

@@ -814,7 +814,7 @@ void CodeViewDebug::emitSecureHotPatchInformation() {
 
   for (const auto &F : MMI->getModule()->functions()) {
     if (!F.isDeclarationForLinker() &&
-        F.hasFnAttribute(Attribute::MarkedForWindowsHotPatching)) {
+        F.hasFnAttribute(Attribute::MarkedForWindowsSecureHotPatching)) {
       if (hotPatchInfo == nullptr)
         hotPatchInfo = beginCVSubsection(DebugSubsectionKind::Symbols);
       MCSymbol *HotPatchEnd = beginSymbolRecord(SymbolKind::S_HOTPATCHFUNC);

llvm/lib/CodeGen/WindowsSecureHotPatching.cpp

@@ -52,6 +52,54 @@
 //   behavior can be disabled for these globals by marking them with the
 //   `AllowDirectAccessInHotPatchFunction`.
 //
+// Rewriting references to global variables has some complexity.
+//
+// For ordinary instructions that reference GlobalVariables, we rewrite the
+// operand of the instruction to a Load of the __ref_* variable.
+//
+// For constant expressions, we have to convert the constant expression (and
+// transitively all constant expressions in its parent chain) to non-constant
+// expressions, i.e. to a sequence of instructions.
+//
+// Pass 1:
+//   * Enumerate all instructions in all basic blocks.
+//
+//   * If an instruction references a GlobalVariable (and it is not marked
+//     as being ignored), then we create (if necessary) the __ref_* variable
+//     for the GlobalVariable reference. However, we do not yet modify the
+//     Instruction.
+//
+//   * If an instruction has an operand that is a ConstantExpr and the
+//     ConstantExpression tree contains a reference to a GlobalVariable, then
+//     we similarly create __ref_*. Similarly, we do not yet modify the
+//     Instruction or the ConstantExpr tree.
+//
+// After Pass 1 completes, we will know whether we found any references to
+// globals in this pass.  If the function does not use any globals (and most
+// functions do not use any globals), then we return immediately.
+//
+// If a function does reference globals, then we iterate the list of globals
+// used by this function and we generate Load instructions for each (unique)
+// global.
+//
+// Next, we do another pass over all instructions:
+//
+// Pass 2:
+//   * Re-visit the instructions that were found in Pass 1.
+//
+//   * If an instruction operand is a GlobalVariable, then look up the
+//   replacement
+//     __ref_* global variable and the Value that came from the Load instruction
+//     for it.  Replace the operand of the GlobalVariable with the Load Value.
+//
+//   * If an instruction operand is a ConstantExpr, then recursively examine the
+//     operands of all instructions in the ConstantExpr tree.  If an operand is
+//     a GlobalVariable, then replace the operand with the result of the load
+//     *and* convert the ConstantExpr to a non-constant instruction.  This
+//     instruction will need to be inserted into the BB of the instruction whose
+//     operand is being modified, ideally immediately before the instruction
+//     being modified.
+//
 // References
 //
 // * "Hotpatching on Windows":
@@ -184,14 +232,14 @@ bool WindowsSecureHotPatching::doInitialization(Module &M) {
         continue;
 
       if (HotPatchFunctionsSet.contains(F.getName()))
-        F.addFnAttr(Attribute::MarkedForWindowsHotPatching);
+        F.addFnAttr(Attribute::MarkedForWindowsSecureHotPatching);
     }
   }
 
   SmallDenseMap<GlobalVariable *, GlobalVariable *> RefMapping;
   bool MadeChanges = false;
   for (auto &F : M.functions()) {
-    if (F.hasFnAttribute(Attribute::MarkedForWindowsHotPatching)) {
+    if (F.hasFnAttribute(Attribute::MarkedForWindowsSecureHotPatching)) {
       if (runOnFunction(F, RefMapping))
         MadeChanges = true;
     }
@@ -249,56 +297,6 @@ static bool globalVariableNeedsRedirect(GlobalVariable *GV) {
   return TypeContainsPointers(GV->getValueType());
 }
 
-/*
-
-Rewriting references to global variables has some complexity.
-
-For ordinary instructions that reference GlobalVariables, we rewrite the
-operand of the instruction to a Load of the __ref_* variable.
-
-For constant expressions, we have to convert the constant expression (and
-transitively all constant expressions in its parent chain) to non-constant
-expressions, i.e. to a sequence of instructions.
-
-Pass 1:
-  * Enumerate all instructions in all basic blocks.
-
-  * If an instruction references a GlobalVariable (and it is not marked
-    as being ignored), then we create (if necessary) the __ref_* variable
-    for the GlobalVariable reference. However, we do not yet modify the
-    Instruction.
-
-  * If an instruction has an operand that is a ConstantExpr and the
-    ConstantExpression tree contains a reference to a GlobalVariable, then
-    we similarly create __ref_*. Similarly, we do not yet modify the Instruction
-    or the ConstantExpr tree.
-
-After Pass 1 completes, we will know whether we found any references to
-globals in this pass.  If the function does not use any globals (and most
-functions do not use any globals), then we return immediately.
-
-If a function does reference globals, then we iterate the list of globals
-used by this function and we generate Load instructions for each (unique)
-global.
-
-Next, we do another pass over all instructions:
-
-Pass 2:
-  * Re-visit the instructions that were found in Pass 1.
-
-  * If an instruction operand is a GlobalVariable, then look up the replacement
-    __ref_* global variable and the Value that came from the Load instruction
-    for it.  Replace the operand of the GlobalVariable with the Load Value.
-
-  * If an instruction operand is a ConstantExpr, then recursively examine the
-    operands of all instructions in the ConstantExpr tree.  If an operand is
-    a GlobalVariable, then replace the operand with the result of the load
-    *and* convert the ConstantExpr to a non-constant instruction.  This
-    instruction will need to be inserted into the BB of the instruction whose
-    operand is being modified, ideally immediately before the instruction
-    being modified.
-*/
-
 // Get or create a new global variable that points to the old one and whose
 // name begins with `__ref_`.
 //
@@ -308,7 +306,7 @@ Pass 2:
 // that all code (both unpatched and patched) is using the same instances of
 // global variables.
 //
-// The Windows hot-patch infrastructure handles initializing these __ref_*
+// The Windows hot-patch infrastructure handles modifying these __ref_*
 // variables. By default, they are initialized with pointers to the equivalent
 // global variables, so when a hot-patch module is loaded *as* a base image
 // (such as after a system reboot), hot-patch functions will access the
@@ -428,27 +426,6 @@ static Value *rewriteGlobalVariablesInConstant(
   return NewInst;
 }
 
-// Processes a function that is marked for hot-patching.
-//
-// If a function is marked for hot-patching, we generate an S_HOTPATCHFUNC
-// CodeView debug symbol. Tools that generate hot-patches look for
-// S_HOTPATCHFUNC in final PDBs so that they can find functions that have been
-// hot-patched and so that they can distinguish hot-patched functions from
-// non-hot-patched functions.
-//
-// Also, in functions that are hot-patched, we must indirect all access to
-// (mutable) global variables through a pointer. This pointer may point into the
-// unpatched ("base") binary or may point into the patched image, depending on
-// whether a hot-patch was loaded as a patch or as a base image.  These
-// indirections go through a new global variable, named `__ref_<Foo>` where
-// `<Foo>` is the original symbol name of the global variable.
-//
-// This function handles rewriting accesses to global variables, but the
-// generation of S_HOTPATCHFUNC occurs in
-// CodeViewDebug::emitHotPatchInformation().
-//
-// Returns true if any global variable references were found.
-
 static bool searchConstantExprForGlobalVariables(
     Value *V, SmallDenseMap<GlobalVariable *, Value *> &GVLoadMap,
     SmallVector<GlobalVariableUse> &GVUses) {
@@ -479,6 +456,26 @@ static bool searchConstantExprForGlobalVariables(
   }
 }
 
+// Processes a function that is marked for hot-patching.
+//
+// If a function is marked for hot-patching, we generate an S_HOTPATCHFUNC
+// CodeView debug symbol. Tools that generate hot-patches look for
+// S_HOTPATCHFUNC in final PDBs so that they can find functions that have been
+// hot-patched and so that they can distinguish hot-patched functions from
+// non-hot-patched functions.
+//
+// Also, in functions that are hot-patched, we must indirect all access to
+// (mutable) global variables through a pointer. This pointer may point into the
+// unpatched ("base") binary or may point into the patched image, depending on
+// whether a hot-patch was loaded as a patch or as a base image.  These
+// indirections go through a new global variable, named `__ref_<Foo>` where
+// `<Foo>` is the original symbol name of the global variable.
+//
+// This function handles rewriting accesses to global variables, but the
+// generation of S_HOTPATCHFUNC occurs in
+// CodeViewDebug::emitHotPatchInformation().
+//
+// Returns true if any global variable references were found and rewritten.
 bool WindowsSecureHotPatching::runOnFunction(
     Function &F,
     SmallDenseMap<GlobalVariable *, GlobalVariable *> &RefMapping) {

llvm/lib/Transforms/Utils/CodeExtractor.cpp

@@ -938,7 +938,7 @@ Function *CodeExtractor::constructFunctionDeclaration(
       case Attribute::CoroDestroyOnlyWhenComplete:
       case Attribute::CoroElideSafe:
       case Attribute::NoDivergenceSource:
-      case Attribute::MarkedForWindowsHotPatching:
+      case Attribute::MarkedForWindowsSecureHotPatching:
       case Attribute::AllowDirectAccessInHotPatchFunction:
         continue;
       // Those attributes should be safe to propagate to the extracted function.