https://github.com/PanTao2 updated https://github.com/llvm/llvm-project/pull/225626
>From d4ab2cc089c055101eec0b7bb5b069b464633b46 Mon Sep 17 00:00:00 2001 From: "Pan, Tao" <[email protected]> Date: Wed, 23 Sep 2026 14:52:49 +0800 Subject: [PATCH 1/2] [clang-cl] Use MSVC's "GS buffer" heuristic for the default /GS clang-cl enables /GS (Buffer Security Check) by default, but implemented it as -fstack-protector-strong. That heuristic protects considerably more functions than MSVC's does, most notably any function that merely takes the address of a local, which costs code size and performance relative to MSVC for no compatibility benefit. Add a new stack protector mode, LangOptions::SSPMSVC, which clang-cl now passes via -stack-protector 4 instead of the "strong" level. It still lowers to the sspstrong function attribute, but additionally emits a "stack-protector-gs-buffer"="true" function attribute that tells StackProtector to select on MSVC's rules: a function is protected only if it takes no variable argument list and allocates a "GS buffer", i.e. - an array larger than 4 bytes with more than two elements whose element type is not a pointer type, - an aggregate larger than 8 bytes that contains no pointers, - an alloca() buffer of any size, or - an aggregate containing any of the above. Also matching MSVC, no check is inserted at -O0. Stack layout rules are unchanged. /GS-, __declspec(safebuffers) and __declspec(strict_gs_check) keep their existing meanings; strict_gs_check continues to select the broader "strong" heuristic and still applies at -O0, since it is an explicit per-function opt-in. To keep inlining from weakening protection, adjustCallerSSPLevel() drops "stack-protector-gs-buffer" from the caller when a callee that was compiled with the stronger heuristic is inlined into it. Because clang-cl no longer defaults to a GCC-compatible level, it no longer predefines __SSP_STRONG__; /clang:-fstack-protector-strong restores the previous behavior. --- clang/docs/ReleaseNotes.md | 13 ++ clang/include/clang/Basic/LangOptions.def | 2 +- clang/include/clang/Basic/LangOptions.h | 2 +- clang/include/clang/Options/Options.td | 4 +- clang/lib/CodeGen/CodeGenModule.cpp | 18 +- clang/lib/CodeGen/CodeGenModule.h | 2 + clang/lib/Driver/ToolChains/Clang.cpp | 2 +- .../CodeGen/ms-stack-protector-gs-buffer.c | 48 +++++ clang/test/Driver/cl-options.c | 13 +- clang/test/Driver/cl-options.cu | 2 +- llvm/docs/LangRef.md | 24 +++ llvm/lib/CodeGen/StackProtector.cpp | 122 +++++++++++- llvm/lib/IR/Attributes.cpp | 9 + .../CodeGen/X86/stack-protector-gs-buffer.ll | 185 ++++++++++++++++++ .../Transforms/Inline/inline-ssp-gs-buffer.ll | 46 +++++ 15 files changed, 474 insertions(+), 18 deletions(-) create mode 100644 clang/test/CodeGen/ms-stack-protector-gs-buffer.c create mode 100644 llvm/test/CodeGen/X86/stack-protector-gs-buffer.ll create mode 100644 llvm/test/Transforms/Inline/inline-ssp-gs-buffer.ll diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index e5da258b9950a..24b8ee4f030aa 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -122,6 +122,19 @@ features cannot lower the translation-unit ABI level; target triples (except 32b arm targets). Can be disabled via `-fno-stack-clash-protection`. +- clang-cl's default `/GS` (Buffer Security Check) now uses MSVC's "GS buffer" + heuristic instead of the GCC-compatible `-fstack-protector-strong` one. A + function is protected if it allocates an array larger than 4 bytes with more + than two elements of non-pointer type, a pointer-free aggregate larger than 8 + bytes, an `_alloca` buffer, or an aggregate containing one of those. Matching + MSVC, a local merely having its address taken no longer protects a function, + varargs functions are never protected, and no protection is inserted when + optimizations are disabled. `/GS-` and `__declspec(safebuffers)` still + disable it, and `__declspec(strict_gs_check)` still selects the broader + heuristic. Since clang-cl no longer defaults to a GCC-compatible level it no + longer predefines `__SSP_STRONG__`; pass `/clang:-fstack-protector-strong` + for the previous behavior. + ### Clang Python Bindings Potentially Breaking Changes - `CompletionChunkKind` instance's `__str__` representation has been adapted to be consistent with other enums in the library. diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def index d7637f2dfd507..0e7fc5a30b5c0 100644 --- a/clang/include/clang/Basic/LangOptions.def +++ b/clang/include/clang/Basic/LangOptions.def @@ -369,7 +369,7 @@ ENUM_LANGOPT(ExternDeclNoDLLStorageClassVisibility, VisibilityFromDLLStorageClas LANGOPT(SemanticInterposition , 1, 0, Benign, "semantic interposition") LANGOPT(HalfNoSemanticInterposition, 1, 0, Benign, "Like -fno-semantic-interposition but don't use local aliases") -ENUM_LANGOPT(StackProtector, StackProtectorMode, 2, SSPOff, NotCompatible, +ENUM_LANGOPT(StackProtector, StackProtectorMode, 3, SSPOff, NotCompatible, "stack protector mode") ENUM_LANGOPT(TrivialAutoVarInit, TrivialAutoVarInitKind, 2, TrivialAutoVarInitKind::Uninitialized, Benign, "trivial automatic variable initialization") diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h index 7539e000d03f9..2b80946009070 100644 --- a/clang/include/clang/Basic/LangOptions.h +++ b/clang/include/clang/Basic/LangOptions.h @@ -96,7 +96,7 @@ class LangOptionsBase { }; enum GCMode { NonGC, GCOnly, HybridGC }; - enum StackProtectorMode { SSPOff, SSPOn, SSPStrong, SSPReq }; + enum StackProtectorMode { SSPOff, SSPOn, SSPStrong, SSPReq, SSPMSVC }; // Automatic variables live on the stack, and when trivial they're usually // uninitialized because it's undefined behavior to use them without diff --git a/clang/include/clang/Options/Options.td b/clang/include/clang/Options/Options.td index d4bf48040029f..4494a0ca8b15e 100644 --- a/clang/include/clang/Options/Options.td +++ b/clang/include/clang/Options/Options.td @@ -8984,9 +8984,9 @@ def static_define : Flag<["-"], "static-define">, MarshallingInfoFlag<LangOpts<"Static">>; def stack_protector : Separate<["-"], "stack-protector">, HelpText<"Enable stack protectors">, - Values<"0,1,2,3">, + Values<"0,1,2,3,4">, NormalizedValuesScope<"LangOptions">, - NormalizedValues<["SSPOff", "SSPOn", "SSPStrong", "SSPReq"]>, + NormalizedValues<["SSPOff", "SSPOn", "SSPStrong", "SSPReq", "SSPMSVC"]>, MarshallingInfoEnum<LangOpts<"StackProtector">, "SSPOff">; def stack_protector_buffer_size : Separate<["-"], "stack-protector-buffer-size">, HelpText<"Lower bound for a buffer to be considered for stack protection">, diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index 70445dd65ea2b..496efdb731fc0 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -1163,12 +1163,24 @@ static bool isStackProtectorOn(const LangOptions &LangOpts, return LangOpts.getStackProtector() == Mode; } +bool CodeGenModule::useMSVCGSBufferHeuristic(const Decl *D) const { + return isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPMSVC) && + !(D && D->hasAttr<StrictGuardStackCheckAttr>()); +} + std::optional<llvm::Attribute::AttrKind> CodeGenModule::StackProtectorAttribute(const Decl *D) const { + // MSVC does not insert buffer security checks when optimizations are + // disabled. __declspec(strict_gs_check) is an explicit per-function opt-in, + // so it still applies at -O0. + if (useMSVCGSBufferHeuristic(D) && CodeGenOpts.OptimizationLevel == 0) + return std::nullopt; + if (D && D->hasAttr<NoStackProtectorAttr>()) ; // Do nothing. else if (D && D->hasAttr<StrictGuardStackCheckAttr>() && - isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPOn)) + (isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPOn) || + isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPMSVC))) return llvm::Attribute::StackProtectStrong; else if (isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPOn)) return llvm::Attribute::StackProtect; @@ -1176,6 +1188,8 @@ CodeGenModule::StackProtectorAttribute(const Decl *D) const { return llvm::Attribute::StackProtectStrong; else if (isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPReq)) return llvm::Attribute::StackProtectReq; + else if (isStackProtectorOn(LangOpts, getTriple(), LangOptions::SSPMSVC)) + return llvm::Attribute::StackProtectStrong; return std::nullopt; } @@ -3170,6 +3184,8 @@ void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D, if (std::optional<llvm::Attribute::AttrKind> Attr = StackProtectorAttribute(D)) { B.addAttribute(*Attr); + if (useMSVCGSBufferHeuristic(D)) + B.addAttribute("stack-protector-gs-buffer", "true"); } if (!D) { diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h index 9e3f073c20f4a..02b5d5d96bdc0 100644 --- a/clang/lib/CodeGen/CodeGenModule.h +++ b/clang/lib/CodeGen/CodeGenModule.h @@ -1994,6 +1994,8 @@ class CodeGenModule : public CodeGenTypeCache { std::optional<llvm::Attribute::AttrKind> StackProtectorAttribute(const Decl *D) const; + bool useMSVCGSBufferHeuristic(const Decl *D) const; + std::string getPFPFieldName(const FieldDecl *FD); llvm::GlobalValue *getPFPDeactivationSymbol(const FieldDecl *FD); diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 6636a5fd6e655..3511b5630d59b 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -8894,7 +8894,7 @@ void Clang::AddClangCLArgs(const ArgList &Args, types::ID InputType, if (!isNVPTX && Args.hasFlag(options::OPT__SLASH_GS, options::OPT__SLASH_GS_, /*Default=*/true)) { CmdArgs.push_back("-stack-protector"); - CmdArgs.push_back(Args.MakeArgString(Twine(LangOptions::SSPStrong))); + CmdArgs.push_back(Args.MakeArgString(Twine(LangOptions::SSPMSVC))); } const Driver &D = getToolChain().getDriver(); diff --git a/clang/test/CodeGen/ms-stack-protector-gs-buffer.c b/clang/test/CodeGen/ms-stack-protector-gs-buffer.c new file mode 100644 index 0000000000000..aa536f1108932 --- /dev/null +++ b/clang/test/CodeGen/ms-stack-protector-gs-buffer.c @@ -0,0 +1,48 @@ +// Check the attributes clang-cl's default /GS emits: sspstrong paired with the +// "stack-protector-gs-buffer" marker that selects MSVC's GS-buffer heuristic. +// +// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -O2 \ +// RUN: -stack-protector 4 -emit-llvm %s -o - | FileCheck %s --check-prefix=GS +// +// MSVC does not insert buffer security checks when optimizations are disabled. +// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions \ +// RUN: -stack-protector 4 -emit-llvm %s -o - | FileCheck %s --check-prefix=NOOPT +// +// An explicit GCC-style level is unaffected by any of this. +// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -O2 \ +// RUN: -stack-protector 2 -emit-llvm %s -o - | FileCheck %s --check-prefix=STRONG + +void use(void *); + +// GS: define dso_local void @plain() {{.*}}#[[#PLAIN:]] { +// NOOPT: define dso_local void @plain() #[[#PLAIN:]] { +// STRONG: define dso_local void @plain() {{.*}}#[[#SPLAIN:]] { +void plain(void) { char buf[64]; use(buf); } + +// __declspec(safebuffers) opts out entirely, at every level. +// GS: define dso_local void @safe() {{.*}}#[[#SAFE:]] { +// STRONG: define dso_local void @safe() {{.*}}#[[#SSAFE:]] { +__declspec(safebuffers) void safe(void) { char buf[64]; use(buf); } + +// __declspec(strict_gs_check) asks for a cookie in a greater number of +// functions, so it opts back out of the narrower GS-buffer rules and is not +// suppressed at -O0. +// GS: define dso_local void @strict() {{.*}}#[[#STRICT:]] { +// NOOPT: define dso_local void @strict() #[[#STRICT:]] { +__declspec(strict_gs_check) void strict(void) { char buf[64]; use(buf); } + +// The string attribute sorts between "stack-protector-buffer-size" and +// "target-features", so matching those neighbours proves it is absent. +// +// GS: attributes #[[#PLAIN]] = { nounwind sspstrong {{.*}}"stack-protector-buffer-size"="8" "stack-protector-gs-buffer"="true" "target-features" +// GS: attributes #[[#SAFE]] = { nounwind "min-legal-vector-width" +// GS: attributes #[[#STRICT]] = { nounwind sspstrong {{.*}}"stack-protector-buffer-size"="8" "target-features" + +// At -O0 the GS-buffer default produces no stack protector attribute at all, +// so @plain shares an attribute group with @safe. +// NOOPT: attributes #[[#PLAIN]] = { noinline nounwind optnone "min-legal-vector-width" +// NOOPT: attributes #[[#STRICT]] = { noinline nounwind optnone sspstrong {{.*}}"stack-protector-buffer-size"="8" "target-features" + +// -stack-protector 2 keeps the GCC-compatible strong heuristic: no marker. +// STRONG: attributes #[[#SPLAIN]] = { nounwind sspstrong {{.*}}"stack-protector-buffer-size"="8" "target-features" +// STRONG: attributes #[[#SSAFE]] = { nounwind "min-legal-vector-width" diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c index 57d82622ef4de..4f9caa9d022e1 100644 --- a/clang/test/Driver/cl-options.c +++ b/clang/test/Driver/cl-options.c @@ -119,14 +119,23 @@ // Security Buffer Check is on by default. // RUN: %clang_cl -### -- %s 2>&1 | FileCheck -check-prefix=GS-default %s -// GS-default: "-stack-protector" "2" +// GS-default: "-stack-protector" "4" // RUN: %clang_cl /GS -### -- %s 2>&1 | FileCheck -check-prefix=GS %s -// GS: "-stack-protector" "2" +// GS: "-stack-protector" "4" // RUN: %clang_cl /GS- -### -- %s 2>&1 | FileCheck -check-prefix=GS_ %s // GS_-NOT: -stack-protector +// An explicit GCC-style level is rendered after the /GS default and wins. +// RUN: %clang_cl /clang:-fstack-protector-strong -### -- %s 2>&1 | FileCheck -check-prefix=GS-strong %s +// GS-strong: "-stack-protector" "4" +// GS-strong-SAME: "-stack-protector" "2" + +// RUN: %clang_cl /clang:-fstack-protector-all -### -- %s 2>&1 | FileCheck -check-prefix=GS-all %s +// GS-all: "-stack-protector" "4" +// GS-all-SAME: "-stack-protector" "3" + // RUN: %clang_cl /Gy -### -- %s 2>&1 | FileCheck -check-prefix=Gy %s // Gy: -ffunction-sections diff --git a/clang/test/Driver/cl-options.cu b/clang/test/Driver/cl-options.cu index b241ec6672d85..4c6d4aa9756cb 100644 --- a/clang/test/Driver/cl-options.cu +++ b/clang/test/Driver/cl-options.cu @@ -8,7 +8,7 @@ // GS-default: "-cc1" "-triple" "nvptx{{(64)?}}-nvidia-cuda" // GS-default-NOT: "-stack-protector" // GS-default: "-cc1" "-triple" -// GS-default: "-stack-protector" "2" +// GS-default: "-stack-protector" "4" // -exceptions should be passed to device-side compilation. // RUN: not %clang_cl /c /GX -### -nocudalib -nocudainc -- %s 2>&1 | FileCheck -check-prefix=GX %s diff --git a/llvm/docs/LangRef.md b/llvm/docs/LangRef.md index a4816d337e02b..40d9183279a2b 100644 --- a/llvm/docs/LangRef.md +++ b/llvm/docs/LangRef.md @@ -2836,6 +2836,30 @@ fn -> other_fn -> other_fn ; fn is norecurse function which has an `ssp` or `sspstrong` attribute, the calling function's attribute will be upgraded to `sspreq`. +`"stack-protector-gs-buffer"` +: This attribute replaces the heuristic used by the `ssp` and `sspstrong` + attributes with the one MSVC's `/GS` (Buffer Security Check) option + uses. It has no effect on a function that also has `sspreq`. It takes a + boolean value, and is emitted by clang-cl. + + Under this heuristic a function is protected if it takes no variable + argument list and it allocates a "GS buffer", which is any of: + + - an array that is larger than 4 bytes, has more than two elements, and + has an element type that is not a pointer type; + - a data structure whose size is more than 8 bytes and that contains no + pointers; + - a buffer allocated by `alloca()`, regardless of size; + - any class or structure that contains a GS buffer. + + Unlike `sspstrong`, a local variable merely having its address taken does + not cause a function to be protected. Stack layout rules are unchanged. + + If a function with an `ssp` or `sspstrong` attribute but no + `"stack-protector-gs-buffer"` attribute is inlined into a calling function + that has one, the attribute is dropped from the caller, so that inlining + cannot weaken the callee's protection. + (strictfp)= `strictfp` diff --git a/llvm/lib/CodeGen/StackProtector.cpp b/llvm/lib/CodeGen/StackProtector.cpp index 9ec1d0630952d..06158545e1215 100644 --- a/llvm/lib/CodeGen/StackProtector.cpp +++ b/llvm/lib/CodeGen/StackProtector.cpp @@ -63,6 +63,17 @@ static cl::opt<bool> EnableSelectionDAGSP("enable-selectiondag-sp", static cl::opt<bool> DisableCheckNoReturn("disable-check-noreturn-call", cl::init(false), cl::Hidden); +/// Function attribute selecting MSVC's /GS (Buffer Security Check) heuristic. +/// Emitted by clang-cl. +static constexpr const char *GSBufferAttr = "stack-protector-gs-buffer"; + +/// An array is only an MSVC "GS buffer" if it is strictly larger than this. +static constexpr uint64_t MinGSArraySize = 4; + +/// A pointer-free aggregate is only an MSVC "GS buffer" if it is strictly +/// larger than this. +static constexpr uint64_t MinGSAggregateSize = 8; + /// InsertStackProtectors - Insert code into the prologue and epilogue of the /// function. /// @@ -275,6 +286,75 @@ static bool ContainsProtectableArray(Type *Ty, Module *M, unsigned SSPBufferSize return NeedsProtector; } +/// Returns true if \p Ty holds a pointer anywhere within it. MSVC's GS-buffer +/// rules treat a pointer-free aggregate as something that may be used as a +/// buffer, so the presence of a pointer is what disqualifies it. +static bool ContainsPointer(Type *Ty) { + if (Ty->isPointerTy()) + return true; + if (auto *AT = dyn_cast<ArrayType>(Ty)) + return ContainsPointer(AT->getElementType()); + if (auto *ST = dyn_cast<StructType>(Ty)) + return any_of(ST->elements(), ContainsPointer); + return false; +} + +/// Returns true if \p Ty is, or contains, a "GS buffer" as defined by MSVC's +/// /GS (Buffer Security Check) documentation: +/// +/// - an array that is larger than 4 bytes, has more than two elements, and +/// has an element type that is not a pointer type; +/// - a data structure whose size is more than 8 bytes and that contains no +/// pointers; +/// - any class or structure that contains a GS buffer. +/// +/// \param [out] IsLarge is set to true if the GS buffer that was found is +/// "large" (>= ssp-buffer-size), so that it is laid out closest to the stack +/// guard. In an aggregate holding several GS buffers this is set if any of +/// them is large. +static bool ContainsGSBuffer(Type *Ty, Module *M, unsigned SSPBufferSize, + bool &IsLarge) { + if (!Ty) + return false; + + const DataLayout &DL = M->getDataLayout(); + auto Found = [&](Type *BufferTy) { + if (DL.getTypeAllocSize(BufferTy).getKnownMinValue() >= SSPBufferSize) + IsLarge = true; + return true; + }; + + if (auto *AT = dyn_cast<ArrayType>(Ty)) { + if (!AT->getElementType()->isPointerTy() && AT->getNumElements() > 2 && + DL.getTypeAllocSize(AT).getKnownMinValue() > MinGSArraySize) + return Found(AT); + + // The array itself is not a GS buffer, but its elements may still be or + // contain one, e.g. an array of two structs that each hold a buffer. + return ContainsGSBuffer(AT->getElementType(), M, SSPBufferSize, IsLarge); + } + + auto *ST = dyn_cast<StructType>(Ty); + if (!ST || ST->isOpaque()) + return false; + + if (DL.getTypeAllocSize(ST).getKnownMinValue() > MinGSAggregateSize && + !ContainsPointer(ST)) + return Found(ST); + + bool NeedsProtector = false; + for (Type *ET : ST->elements()) + if (ContainsGSBuffer(ET, M, SSPBufferSize, IsLarge)) { + // If a member is a large GS buffer then we are done. Otherwise keep + // looking, in case a later member is a large one. + if (IsLarge) + return true; + NeedsProtector = true; + } + + return NeedsProtector; +} + /// Maximum remaining allocation size observed for a phi node, and how often /// the allocation size has already been decreased. We only allow a limited /// number of decreases. @@ -404,7 +484,8 @@ static const CallInst *findStackProtectorIntrinsic(Function &F) { /// Check whether or not this function needs a stack protector based /// upon the stack protector level. /// -/// We use two heuristics: a standard (ssp) and strong (sspstrong). +/// We use three heuristics: a standard (ssp), a strong (sspstrong) and an +/// MSVC-compatible one selected by the "stack-protector-gs-buffer" attribute. /// The standard heuristic which will add a guard variable to functions that /// call alloca with a either a variable size or a size >= SSPBufferSize, /// functions with character buffers larger than SSPBufferSize, and functions @@ -413,12 +494,17 @@ static const CallInst *findStackProtectorIntrinsic(Function &F) { /// regardless of size, functions with any buffer regardless of type and size, /// functions with aggregates that contain any buffer regardless of type and /// size, and functions that contain stack-based variables that have had their -/// address taken. +/// address taken. The MSVC heuristic implements /GS (Buffer Security Check): +/// it adds a guard variable to functions that call alloca regardless of size +/// and to functions holding a "GS buffer" (see ContainsGSBuffer), but it does +/// not consider a variable's address being taken, and it never protects a +/// function that takes a variable argument list. bool SSPLayoutAnalysis::requiresStackProtector(Function *F, SSPLayoutMap *Layout) { Module *M = F->getParent(); bool Strong = false; bool NeedsProtector = false; + bool GSBuffer = false; // The set of PHI nodes visited when determining if a variable's reference has // been taken. This set is maintained to ensure we don't visit the same PHI @@ -447,9 +533,20 @@ bool SSPLayoutAnalysis::requiresStackProtector(Function *F, }); NeedsProtector = true; Strong = true; // Use the same heuristic as strong to determine SSPLayout - } else if (F->hasFnAttribute(Attribute::StackProtectStrong)) - Strong = true; - else if (!F->hasFnAttribute(Attribute::StackProtect)) + } else if (F->hasFnAttribute(Attribute::StackProtectStrong)) { + // clang-cl's default /GS asks for MSVC's GS-buffer rules rather than the + // GCC-compatible strong heuristic. An explicit sspreq (handled above) + // still wins over both. + GSBuffer = F->getFnAttribute(GSBufferAttr).getValueAsBool(); + Strong = !GSBuffer; + } else if (F->hasFnAttribute(Attribute::StackProtect)) { + GSBuffer = F->getFnAttribute(GSBufferAttr).getValueAsBool(); + } else { + return false; + } + + // MSVC's /GS does not protect functions that take a variable argument list. + if (GSBuffer && F->isVarArg()) return false; for (const BasicBlock &BB : *F) { @@ -479,8 +576,10 @@ bool SSPLayoutAnalysis::requiresStackProtector(Function *F, std::make_pair(AI, MachineFrameInfo::SSPLK_LargeArray)); ORE.emit(RemarkBuilder); NeedsProtector = true; - } else if (Strong) { - // Require protectors for all alloca calls in strong mode. + } else if (Strong || GSBuffer) { + // Require protectors for all alloca calls in strong mode. MSVC's + // /GS likewise treats every _alloca buffer as a GS buffer, with + // no size threshold. if (!Layout) return true; Layout->insert( @@ -501,8 +600,13 @@ bool SSPLayoutAnalysis::requiresStackProtector(Function *F, } bool IsLarge = false; - if (ContainsProtectableArray(AI->getAllocatedType(), M, SSPBufferSize, - IsLarge, Strong, false)) { + bool Protect = + GSBuffer ? ContainsGSBuffer(AI->getAllocatedType(), M, + SSPBufferSize, IsLarge) + : ContainsProtectableArray(AI->getAllocatedType(), M, + SSPBufferSize, IsLarge, Strong, + /*InStruct=*/false); + if (Protect) { if (!Layout) return true; Layout->insert(std::make_pair( diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp index 7f5f5f34af389..58c3bcaa41a5e 100644 --- a/llvm/lib/IR/Attributes.cpp +++ b/llvm/lib/IR/Attributes.cpp @@ -2625,6 +2625,15 @@ static void adjustCallerSSPLevel(Function &Caller, const Function &Callee) { if (!Caller.hasStackProtectorFnAttr()) return; + // The caller may be using MSVC's /GS heuristic, which protects fewer + // functions than "strong" does. If the callee asked for the stronger + // heuristic, drop the /GS marker so that inlining cannot weaken the + // protection the callee was compiled with. + if ((Callee.hasFnAttribute(Attribute::StackProtectStrong) || + Callee.hasFnAttribute(Attribute::StackProtectReq)) && + !Callee.hasFnAttribute("stack-protector-gs-buffer")) + Caller.removeFnAttr("stack-protector-gs-buffer"); + // If upgrading the SSP attribute, clear out the old SSP Attributes first. // Having multiple SSP attributes doesn't actually hurt, but it adds useless // clutter to the IR. diff --git a/llvm/test/CodeGen/X86/stack-protector-gs-buffer.ll b/llvm/test/CodeGen/X86/stack-protector-gs-buffer.ll new file mode 100644 index 0000000000000..a753e1b3bf65d --- /dev/null +++ b/llvm/test/CodeGen/X86/stack-protector-gs-buffer.ll @@ -0,0 +1,185 @@ +; Check MSVC's /GS (Buffer Security Check) heuristic, selected by the +; "stack-protector-gs-buffer" function attribute. +; +; A "GS buffer" is an array larger than 4 bytes with more than two elements and +; a non-pointer element type, a pointer-free aggregate larger than 8 bytes, an +; alloca of any size, or any aggregate containing one of those. +; +; RUN: llc -mtriple=x86_64-pc-windows-msvc < %s | FileCheck %s + +declare void @use(ptr) + +;; --- Arrays that are not GS buffers ------------------------------------------ + +; Exactly 4 bytes, so not larger than 4. +; CHECK-LABEL: array_4_bytes: +; CHECK-NOT: __security_cookie +; CHECK: .seh_endproc +define void @array_4_bytes() #0 { + %a = alloca [4 x i8] + call void @use(ptr %a) + ret void +} + +; 8 bytes, but only two elements. +; CHECK-LABEL: array_2_elements: +; CHECK-NOT: __security_cookie +; CHECK: .seh_endproc +define void @array_2_elements() #0 { + %a = alloca [2 x i32] + call void @use(ptr %a) + ret void +} + +; Large and many elements, but the element type is a pointer. +; CHECK-LABEL: array_of_pointers: +; CHECK-NOT: __security_cookie +; CHECK: .seh_endproc +define void @array_of_pointers() #0 { + %a = alloca [8 x ptr] + call void @use(ptr %a) + ret void +} + +;; --- Arrays that are GS buffers ---------------------------------------------- + +; CHECK-LABEL: array_8_bytes: +; CHECK: __security_cookie +define void @array_8_bytes() #0 { + %a = alloca [8 x i8] + call void @use(ptr %a) + ret void +} + +; 6 bytes and three elements: over both thresholds. +; CHECK-LABEL: array_3_shorts: +; CHECK: __security_cookie +define void @array_3_shorts() #0 { + %a = alloca [3 x i16] + call void @use(ptr %a) + ret void +} + +; Two elements, so the array itself is not a GS buffer, but each element is. +; CHECK-LABEL: array_2_buffers: +; CHECK: __security_cookie +define void @array_2_buffers() #0 { + %a = alloca [2 x [8 x i8]] + call void @use(ptr %a) + ret void +} + +;; --- Aggregates -------------------------------------------------------------- + +; 12 bytes with no pointers. +; CHECK-LABEL: struct_pointer_free: +; CHECK: __security_cookie +define void @struct_pointer_free() #0 { + %a = alloca { i32, i32, i32 } + call void @use(ptr %a) + ret void +} + +; 16 bytes, but it holds a pointer, so it is not itself a GS buffer. +; CHECK-LABEL: struct_with_pointer: +; CHECK-NOT: __security_cookie +; CHECK: .seh_endproc +define void @struct_with_pointer() #0 { + %a = alloca { ptr, i32, i32 } + call void @use(ptr %a) + ret void +} + +; Exactly 8 bytes, so not larger than 8. +; CHECK-LABEL: struct_8_bytes: +; CHECK-NOT: __security_cookie +; CHECK: .seh_endproc +define void @struct_8_bytes() #0 { + %a = alloca { i32, i32 } + call void @use(ptr %a) + ret void +} + +; Holds a pointer, but also contains a GS buffer. +; CHECK-LABEL: struct_containing_buffer: +; CHECK: __security_cookie +define void @struct_containing_buffer() #0 { + %a = alloca { ptr, [8 x i8] } + call void @use(ptr %a) + ret void +} + +; The GS buffer is two aggregates down. +; CHECK-LABEL: struct_nested_buffer: +; CHECK: __security_cookie +define void @struct_nested_buffer() #0 { + %a = alloca { ptr, { i32, [8 x i8] } } + call void @use(ptr %a) + ret void +} + +;; --- alloca ------------------------------------------------------------------ + +; CHECK-LABEL: dynamic_alloca: +; CHECK: __security_cookie +define void @dynamic_alloca(i64 %n) #0 { + %a = alloca i8, i64 %n + call void @use(ptr %a) + ret void +} + +; Even a small constant-sized alloca is a GS buffer. +; CHECK-LABEL: small_alloca: +; CHECK: __security_cookie +define void @small_alloca() #0 { + %a = alloca i8, i64 3 + call void @use(ptr %a) + ret void +} + +;; --- Exclusions -------------------------------------------------------------- + +; Unlike sspstrong, merely taking a local's address does not protect a function. +; CHECK-LABEL: address_taken: +; CHECK-NOT: __security_cookie +; CHECK: .seh_endproc +define void @address_taken() #0 { + %a = alloca i32 + call void @use(ptr %a) + ret void +} + +; MSVC never protects a function that takes a variable argument list, even one +; holding an obvious GS buffer. +; CHECK-LABEL: variadic: +; CHECK-NOT: __security_cookie +; CHECK: .seh_endproc +define void @variadic(i32 %n, ...) #0 { + %a = alloca [64 x i8] + call void @use(ptr %a) + ret void +} + +; sspreq overrides the heuristic entirely, so the varargs exclusion and the +; GS-buffer rules do not apply. +; CHECK-LABEL: sspreq_wins: +; CHECK: __security_cookie +define void @sspreq_wins(i32 %n, ...) #1 { + %a = alloca i32 + call void @use(ptr %a) + ret void +} + +; Without the marker, sspstrong keeps the GCC-compatible heuristic and does +; protect a small array. +; CHECK-LABEL: strong_still_protects: +; CHECK: __security_cookie +define void @strong_still_protects() #2 { + %a = alloca [4 x i8] + call void @use(ptr %a) + ret void +} + +attributes #0 = { sspstrong uwtable "stack-protector-gs-buffer"="true" } +attributes #1 = { sspreq uwtable "stack-protector-gs-buffer"="true" } +attributes #2 = { sspstrong uwtable } diff --git a/llvm/test/Transforms/Inline/inline-ssp-gs-buffer.ll b/llvm/test/Transforms/Inline/inline-ssp-gs-buffer.ll new file mode 100644 index 0000000000000..3e7f6b6a80a35 --- /dev/null +++ b/llvm/test/Transforms/Inline/inline-ssp-gs-buffer.ll @@ -0,0 +1,46 @@ +; RUN: opt -passes=inline %s -S | FileCheck %s +; Inlining must not leave a caller applying MSVC's narrower /GS heuristic to +; code that was compiled with the GCC-compatible strong heuristic. + +declare void @sink(ptr) + +define internal void @callee_strong() sspstrong { + %a = alloca [4 x i8] + call void @sink(ptr %a) + ret void +} + +define internal void @callee_gs() sspstrong "stack-protector-gs-buffer"="true" { + %a = alloca [64 x i8] + call void @sink(ptr %a) + ret void +} + +define internal void @callee_nossp() { + ret void +} + +; Inlining a genuinely-strong callee drops the caller's /GS marker. +; CHECK: define void @strong_into_gs() #[[STRONG:[0-9]+]] { +define void @strong_into_gs() sspstrong "stack-protector-gs-buffer"="true" { + call void @callee_strong() + ret void +} + +; Inlining another /GS function leaves the marker in place. +; CHECK: define void @gs_into_gs() #[[GS:[0-9]+]] { +define void @gs_into_gs() sspstrong "stack-protector-gs-buffer"="true" { + call void @callee_gs() + ret void +} + +; A callee with no stack protector attribute at all must not disturb the +; marker, since it does not ask for any protection of its own. +; CHECK: define void @nossp_into_gs() #[[GS]] { +define void @nossp_into_gs() sspstrong "stack-protector-gs-buffer"="true" { + call void @callee_nossp() + ret void +} + +; CHECK: attributes #[[STRONG]] = { sspstrong } +; CHECK: attributes #[[GS]] = { sspstrong "stack-protector-gs-buffer"="true" } >From 3d8d0e8ba5e48dd81ce24ab4c4631530d31b01e7 Mon Sep 17 00:00:00 2001 From: "Pan, Tao" <[email protected]> Date: Thu, 8 Oct 2026 10:09:30 +0800 Subject: [PATCH 2/2] Add "stack-protector" metadata on the alloca --- clang/docs/ReleaseNotes.md | 21 +- clang/lib/CodeGen/CGBuiltin.cpp | 17 + clang/lib/CodeGen/CGCall.cpp | 8 +- clang/lib/CodeGen/CGDecl.cpp | 338 +++++++++++++++++- clang/lib/CodeGen/CGExpr.cpp | 20 +- clang/lib/CodeGen/CodeGenFunction.cpp | 5 + clang/lib/CodeGen/CodeGenFunction.h | 54 ++- llvm/docs/LangRef.md | 48 ++- llvm/lib/CodeGen/StackProtector.cpp | 225 +++++++----- .../InstCombineLoadStoreAlloca.cpp | 3 + .../CodeGen/X86/stack-protector-gs-buffer.ll | 206 +++++++---- llvm/test/Transforms/InstCombine/alloca.ll | 18 + 12 files changed, 760 insertions(+), 203 deletions(-) diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 24b8ee4f030aa..0ebeea4af28e7 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -122,19 +122,6 @@ features cannot lower the translation-unit ABI level; target triples (except 32b arm targets). Can be disabled via `-fno-stack-clash-protection`. -- clang-cl's default `/GS` (Buffer Security Check) now uses MSVC's "GS buffer" - heuristic instead of the GCC-compatible `-fstack-protector-strong` one. A - function is protected if it allocates an array larger than 4 bytes with more - than two elements of non-pointer type, a pointer-free aggregate larger than 8 - bytes, an `_alloca` buffer, or an aggregate containing one of those. Matching - MSVC, a local merely having its address taken no longer protects a function, - varargs functions are never protected, and no protection is inserted when - optimizations are disabled. `/GS-` and `__declspec(safebuffers)` still - disable it, and `__declspec(strict_gs_check)` still selects the broader - heuristic. Since clang-cl no longer defaults to a GCC-compatible level it no - longer predefines `__SSP_STRONG__`; pass `/clang:-fstack-protector-strong` - for the previous behavior. - ### Clang Python Bindings Potentially Breaking Changes - `CompletionChunkKind` instance's `__str__` representation has been adapted to be consistent with other enums in the library. @@ -855,6 +842,14 @@ features cannot lower the translation-unit ABI level; subsequent base; Clang now does the same. ([#210174](https://github.com/llvm/llvm-project/issues/210174)) +- `clang-cl`'s `/GS` now uses MSVC's buffer heuristic instead of + `-fstack-protector-strong`. MSVC decides from the declared type of each stack + object, so it protects functions Clang previously left alone — an array of + pointers, or a struct whose pointer-free member array is only two bytes wide — + and leaves alone functions Clang previously protected, such as one whose only + address-taken local is a scalar. `/GS-`, and the GCC-style + `-fstack-protector*` options when passed through `/clang:`, are unaffected. + #### LoongArch Support - `loongarch32-*-none-elf` and `loongarch64-*-none-elf` targets now use the diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp index f500162532777..5d83675a11127 100644 --- a/clang/lib/CodeGen/CGBuiltin.cpp +++ b/clang/lib/CodeGen/CGBuiltin.cpp @@ -177,6 +177,21 @@ static void initializeAlloca(CodeGenFunction &CGF, AllocaInst *AI, Value *Size, I->addAnnotationMetadata("auto-init"); } +/// MSVC's /GS treats a buffer allocated by _alloca as a GS buffer whatever its +/// size, so mark \p AI as one. The element count that would let the backend +/// recognise it on its own does not survive InstCombine folding a +/// constant-sized alloca to an array type, so record it here. +static void markAllocaAsGSBuffer(CodeGenFunction &CGF, AllocaInst *AI, + Value *Size) { + unsigned SSPBufferSize = CGF.CGM.getCodeGenOpts().SSPBufferSize; + const auto *CI = dyn_cast<llvm::ConstantInt>(Size); + // Lay it out as the backend would if the element count were still visible: + // a variable size, or one of at least ssp-buffer-size bytes, goes closest to + // the stack guard. + CGF.MarkGSBufferAlloca(AI, !CI || CI->getLimitedValue(SSPBufferSize) >= + SSPBufferSize); +} + /// getBuiltinLibFunction - Given a builtin id for a function like /// "__builtin_fabsf", return a Function* for "fabsf". llvm::Constant *CodeGenModule::getBuiltinLibFunction(const FunctionDecl *FD, @@ -4813,6 +4828,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID, .getAsAlign(); AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size); AI->setAlignment(SuitableAlignmentInBytes); + markAllocaAsGSBuffer(*this, AI, Size); if (BuiltinID != Builtin::BI__builtin_alloca_uninitialized) initializeAlloca(*this, AI, Size, SuitableAlignmentInBytes); if (AI->getAddressSpace() != @@ -4834,6 +4850,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID, CGM.getContext().toCharUnitsFromBits(AlignmentInBits).getAsAlign(); AllocaInst *AI = Builder.CreateAlloca(Builder.getInt8Ty(), Size); AI->setAlignment(AlignmentInBytes); + markAllocaAsGSBuffer(*this, AI, Size); if (BuiltinID != Builtin::BI__builtin_alloca_with_align_uninitialized) initializeAlloca(*this, AI, Size, AlignmentInBytes); if (AI->getAddressSpace() != diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index 2193a89203f29..4acbbdeef9cbc 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -5293,9 +5293,11 @@ void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E, type->castAsRecordDecl()->isParamDestroyedInCallee()) { // If we're using inalloca, use the argument memory. Otherwise, use a // temporary. - AggValueSlot Slot = args.isUsingInAlloca() - ? createPlaceholderSlot(*this, type) - : CreateAggTemp(type, "agg.tmp"); + AggValueSlot Slot = + args.isUsingInAlloca() + ? createPlaceholderSlot(*this, type) + : CreateAggTemp(type, "agg.tmp", /*Alloca=*/nullptr, + GSObjectKind::ArgumentCopy); bool DestroyedInCallee = true, NeedsCleanup = true; if (const auto *RD = type->getAsCXXRecordDecl()) diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp index e1ed66ae71243..2eed9fd2cee75 100644 --- a/clang/lib/CodeGen/CGDecl.cpp +++ b/clang/lib/CodeGen/CGDecl.cpp @@ -1483,6 +1483,326 @@ static bool shouldExtendLifetime(const ASTContext &Context, return true; } +/// An array is only an MSVC "GS buffer" if it is strictly larger than this. +static constexpr int64_t MinGSArraySize = 4; + +/// A pointer-free aggregate is only an MSVC "GS buffer" if it is strictly +/// larger than this. +static constexpr int64_t MinGSAggregateSize = 8; + +/// Returns true if \p Ty is a pointer as far as MSVC's GS-buffer rules are +/// concerned, i.e. something that holds an address rather than data. +static bool isGSPointerType(QualType Ty) { + return Ty->isAnyPointerType() || Ty->isReferenceType() || + Ty->isBlockPointerType() || Ty->isMemberFunctionPointerType() || + Ty->isNullPtrType(); +} + +/// Returns true if \p Ty holds a pointer anywhere within it. MSVC's GS-buffer +/// rules treat a pointer-free aggregate as something that may be used as a +/// buffer, so the presence of a pointer is what disqualifies it. +/// +/// A pointer a class inherits does not count, at any depth: cl.exe sees +/// `struct B { void *p; }; struct D : B { long long a; };` as a GS buffer, and +/// likewise an aggregate with a `D` member, while the same shapes built out of +/// `B` members are not. The base classes get their own chance to make the +/// derived class a buffer, in isGSBuffer. +static bool containsGSPointer(const ASTContext &Ctx, QualType Ty, + llvm::SmallPtrSetImpl<const RecordDecl *> &Seen) { + if (isGSPointerType(Ty)) + return true; + if (const ArrayType *AT = Ctx.getAsArrayType(Ty)) + return containsGSPointer(Ctx, AT->getElementType(), Seen); + if (const auto *AT = Ty->getAs<AtomicType>()) + return containsGSPointer(Ctx, AT->getValueType(), Seen); + + const RecordDecl *RD = Ty->getAsRecordDecl(); + if (!RD || !RD->isCompleteDefinition() || !Seen.insert(RD).second) + return false; + + // A polymorphic class stores a vptr, and a virtual base is reached through a + // pointer, neither of which shows up as a field. Both are properties of the + // class itself rather than of a base, even when the vptr physically lives in + // a base subobject. + if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) + if (CXXRD->isDynamicClass() || CXXRD->getNumVBases() != 0) + return true; + + return llvm::any_of(RD->fields(), [&](const FieldDecl *FD) { + return containsGSPointer(Ctx, FD->getType(), Seen); + }); +} + +static bool containsGSPointer(const ASTContext &Ctx, QualType Ty) { + llvm::SmallPtrSet<const RecordDecl *, 4> Seen; + return containsGSPointer(Ctx, Ty, Seen); +} + +/// Returns true if \p RD is laid out as plain, publicly accessible data, which +/// is what MSVC's GS-buffer rules require of an aggregate before its size alone +/// makes it a buffer. MSDN does not mention this condition, but cl.exe applies +/// it: `struct S { long long a, b; };` is a GS buffer while the otherwise +/// identical `class C { long long a, b; };` is not. +/// +/// No standard type trait captures this; __is_pod, __is_trivial, +/// __is_standard_layout and __is_aggregate each disagree with cl.exe on at +/// least one case, so it is computed here. +static bool isGSPlainData(const ASTContext &Ctx, const RecordDecl *RD, + llvm::DenseMap<const RecordDecl *, bool> &Cache) { + if (!RD || !RD->isCompleteDefinition()) + return false; + + auto [It, Inserted] = Cache.try_emplace(RD, true); + if (!Inserted) + return It->second; + + auto Reject = [&] { + Cache[RD] = false; + return false; + }; + + if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) { + // Anything that needs a constructor, a destructor or a non-trivial copy is + // not a raw data buffer as far as cl.exe is concerned. This covers the base + // classes too, since a class with a non-trivial base is not trivial. + if (!CXXRD->isTrivial() || CXXRD->isDynamicClass() || + CXXRD->getNumVBases() != 0) + return Reject(); + } + + // Access control only exists in C++; a C struct has no AS_public on its + // fields to find. Only the class's own fields are checked: a base class is + // not, and neither is the access specifier it is inherited with, so + // `struct D : private B {}` is plain data even when `B` is not. + bool CheckAccess = isa<CXXRecordDecl>(RD); + for (const FieldDecl *FD : RD->fields()) { + if (CheckAccess && FD->getAccess() != AS_public) + return Reject(); + // A member that is too small to be a buffer in its own right is just + // bytes, so cl.exe does not care whether it is plain data. A larger one + // disqualifies the enclosing aggregate. An array member is exempt: cl.exe + // treats `struct { Opaque m[1]; }` as plain data but `struct { Opaque m; }` + // not. + QualType FieldTy = FD->getType(); + const RecordDecl *FieldRD = FieldTy->getAsRecordDecl(); + if (FieldRD && !FieldTy->isArrayType() && + Ctx.getTypeSizeInChars(FieldTy).getQuantity() > MinGSAggregateSize && + !isGSPlainData(Ctx, FieldRD, Cache)) + return Reject(); + } + return true; +} + +static bool isGSPlainData(const ASTContext &Ctx, const RecordDecl *RD) { + llvm::DenseMap<const RecordDecl *, bool> Cache; + return isGSPlainData(Ctx, RD, Cache); +} + +/// Where in a type a candidate GS buffer sits. cl.exe applies a different array +/// rule below the top level, dropping the element-count condition and adding +/// one on the element type, so `struct { int m[1]; }` is a GS buffer while +/// `struct { void *m[4]; }` is not -- the other way round from how the two +/// types fare as locals. +enum class GSTypePosition { + /// The type of the stack object itself. + TopLevel, + /// The type of a non-static data member, or of a nested array's elements. + Nested, +}; + +/// Returns true if \p Ty is an array of more than two elements, counting a +/// multidimensional array as the flat array it is laid out as: cl.exe sees +/// `void *a[2][2]` as four elements, not two. +static bool hasMoreThanTwoGSElements(const ASTContext &Ctx, QualType Ty) { + uint64_t N = 1; + while (const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(Ty)) { + // N is at most two here, or the previous round would have returned, so + // this cannot overflow. + N *= CAT->getSize().getLimitedValue(3); + if (N > 2) + return true; + Ty = CAT->getElementType(); + } + return false; +} + +/// Returns true if \p Ty is, or contains, a "GS buffer" as MSVC's /GS (Buffer +/// Security Check) defines one. MSDN describes it as: +/// +/// - an array that is larger than 4 bytes, has more than two elements, and +/// has an element type that is not a pointer type; +/// - a data structure whose size is more than 8 bytes and that contains no +/// pointers; +/// - any class or structure that contains a GS buffer. +/// +/// cl.exe differs from that description in three ways, each confirmed against +/// cl.exe 19.44 and each reproduced here: +/// +/// - the two array conditions do not both apply in both positions. For the +/// type of the stack object itself only the element count matters, so +/// `void *a[8]` is a GS buffer; for the type of a member only the element +/// type matters, so `struct { int m[1]; }` is one but +/// `struct { void *m[4]; }` is not (see GSTypePosition); +/// - the element count of a multidimensional array is the flattened one, so +/// `void *a[2][2]` is a GS buffer; +/// - the size rule for an aggregate additionally requires it to be plain +/// data (see isGSPlainData). +/// +/// (A buffer allocated by _alloca is also a GS buffer, but that one is visible +/// in the IR and so is left to the backend.) +/// +/// \param [out] IsLarge is set to true if the GS buffer that was found is +/// "large" (>= ssp-buffer-size), so that it is laid out closest to the stack +/// guard. In an aggregate holding several GS buffers this is set if any of +/// them is large. +static bool isGSBuffer(const ASTContext &Ctx, QualType Ty, + unsigned SSPBufferSize, bool &IsLarge, + GSTypePosition Position = GSTypePosition::TopLevel) { + if (Ty.isNull() || Ty->isDependentType() || Ty->isIncompleteType()) + return false; + + auto Found = [&](QualType BufferTy) { + if (Ctx.getTypeSizeInChars(BufferTy).getQuantity() >= + static_cast<int64_t>(SSPBufferSize)) + IsLarge = true; + return true; + }; + + if (const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(Ty)) { + QualType ElemTy = CAT->getElementType(); + if (Ctx.getTypeSizeInChars(Ty).getQuantity() > MinGSArraySize && + (Position == GSTypePosition::TopLevel ? hasMoreThanTwoGSElements(Ctx, Ty) + : !isGSPointerType(ElemTy))) + return Found(Ty); + + // The array itself is not a GS buffer, but its elements may still be or + // contain one, e.g. an array of two structs that each hold a buffer. + return isGSBuffer(Ctx, ElemTy, SSPBufferSize, IsLarge, + GSTypePosition::Nested); + } + + const RecordDecl *RD = Ty->getAsRecordDecl(); + if (!RD || !RD->isCompleteDefinition()) + return false; + + if (Ctx.getTypeSizeInChars(Ty).getQuantity() > MinGSAggregateSize && + !containsGSPointer(Ctx, Ty) && isGSPlainData(Ctx, RD)) + return Found(Ty); + + // If a member is a large GS buffer then we are done. Otherwise keep looking, + // in case a later member is a large one. + bool NeedsProtector = false; + auto Check = [&](QualType MemberTy) { + if (isGSBuffer(Ctx, MemberTy, SSPBufferSize, IsLarge, + GSTypePosition::Nested)) + NeedsProtector = true; + return IsLarge; + }; + if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) + for (const CXXBaseSpecifier &Base : CXXRD->bases()) + if (Check(Base.getType())) + return true; + for (const FieldDecl *FD : RD->fields()) { + // cl.exe does not look inside an anonymous struct or union, so + // `struct { union { char b[16]; }; void *p; }` is not a GS buffer while + // the same thing with a named union member is. Its size still counts + // towards the enclosing aggregate, and a pointer in it still disqualifies + // the aggregate. + if (FD->isAnonymousStructOrUnion()) + continue; + if (Check(FD->getType())) + return true; + } + + return NeedsProtector; +} + +void CodeGenFunction::MarkGSBufferAlloca(llvm::AllocaInst *AI, bool IsLarge, + bool IsTrivial) { + if (!MarkGSBuffers || !AI) + return; + + // An explicit opt-out (see StackProtectorIgnoreAttr) has the last word. + if (AI->getMetadata("stack-protector")) + return; + + // 2 is a large buffer and 1 a small one. The second operand says the object is + // trivial, and is left out when it is not, which is the common case; see the + // "stack-protector" metadata in LLVM's LangRef. + llvm::LLVMContext &Ctx = Builder.getContext(); + SmallVector<llvm::Metadata *, 2> Ops = { + llvm::ConstantAsMetadata::get(Builder.getInt32(IsLarge ? 2 : 1))}; + if (IsTrivial) + Ops.push_back(llvm::ConstantAsMetadata::get(Builder.getTrue())); + AI->setMetadata("stack-protector", llvm::MDNode::get(Ctx, Ops)); +} + +void CodeGenFunction::EmitGSBufferStackProtectorMD(llvm::Value *AllocaPtr, + QualType Ty, + GSObjectKind Kind) { + if (!MarkGSBuffers) + return; + + auto *AI = dyn_cast_or_null<llvm::AllocaInst>(AllocaPtr); + if (!AI) + return; + + // An ABI slot holding a trivial object is anonymous storage, which cl.exe + // does not analyse; it protects the callee instead (see + // EmitGSBufferParamCopy). Once the type needs a constructor or a destructor + // the temporary becomes a real object, and cl.exe covers it. + bool IsTrivial = Ty.isTrivialType(getContext()); + if (Kind != GSObjectKind::Named && IsTrivial) + return; + + // For an outgoing by-value argument that is a real object, cl.exe only gets + // to put it inside the guarded region if it can then copy it into the + // argument area bitwise. Otherwise it has to construct it there directly, + // where the guard does not reach. The destructor plays no part: an object + // with a trivial copy constructor and a user-provided destructor is copied, + // and the destructor runs on the original. + if (Kind == GSObjectKind::ArgumentCopy) { + const auto *RD = Ty->getAsCXXRecordDecl(); + if (RD && !(RD->hasSimpleCopyConstructor() && + RD->hasTrivialCopyConstructor())) + return; + } + + bool IsLarge = false; + if (!isGSBuffer(getContext(), Ty, CGM.getCodeGenOpts().SSPBufferSize, + IsLarge)) + return; + + MarkGSBufferAlloca(AI, IsLarge, IsTrivial); +} + +Address CodeGenFunction::EmitGSBufferParamCopy(const VarDecl &D, + Address ParamAddr) { + QualType Ty = D.getType(); + bool IsLarge = false; + if (!MarkGSBuffers || + !isGSBuffer(getContext(), Ty, CGM.getCodeGenOpts().SSPBufferSize, + IsLarge)) + return ParamAddr; + + // Only copy what can be copied by memcpy and needs no destructor, so that + // the copy is indistinguishable from the original. cl.exe likewise leaves + // parameters of non-trivial type in the caller's storage. + if (!Ty.isTriviallyCopyableType(getContext()) || Ty.isDestructedType()) + return ParamAddr; + + RawAddress GSCopy = CreateMemTempWithoutCast( + Ty, getContext().getDeclAlign(&D), D.getName() + "$GSCopy$"); + EmitAggregateCopy(MakeAddrLValue(GSCopy, Ty), MakeAddrLValue(ParamAddr, Ty), + Ty, AggValueSlot::DoesNotOverlap); + // This copy exists only so that the object sits inside the guarded region, + // so it is never a candidate for the unguarded temporary area. + MarkGSBufferAlloca( + dyn_cast<llvm::AllocaInst>(GSCopy.getPointer()->stripPointerCasts()), + IsLarge, /*IsTrivial=*/false); + return GSCopy; +} + /// EmitAutoVarAlloca - Emit the alloca and debug information for a /// local variable. Does not emit initialization or destruction. CodeGenFunction::AutoVarEmission @@ -1720,6 +2040,8 @@ CodeGenFunction::EmitAutoVarAlloca(const VarDecl &D) { EmitAndRegisterVariableArrayDimensions(DI, D, EmitDebugInfo); } + EmitGSBufferStackProtectorMD(address.getBasePointer(), Ty); + setAddrOfLocalVar(&D, address); emission.Addr = address; emission.AllocaAddr = AllocaAddr; @@ -2721,12 +3043,23 @@ void CodeGenFunction::EmitParmDecl(const VarDecl &D, ParamValue Arg, if (Arg.isIndirect()) { DeclPtr = Arg.getIndirectAddress(); DeclPtr = DeclPtr.withElementType(ConvertTypeForMem(Ty)); + ABIArgInfo ArgInfo = CurFnInfo->arguments()[ArgNo - 1].info; + + // An indirectly passed parameter lives in storage the caller allocated, so + // this frame's stack guard does not cover it. When it is a GS buffer, + // relocate it the way cl.exe does. Only `byval` and plain `indirect` own + // their storage; for `byref` the callee's writes are visible to the caller, + // so a copy would change behaviour. + if (MarkGSBuffers && !CurFuncIsThunk && isa<ParmVarDecl>(&D) && + ArgInfo.isIndirect() && + CurFnInfo->getExtParameterInfo(ArgNo - 1).getABI() == + ParameterABI::Ordinary) + DeclPtr = EmitGSBufferParamCopy(D, DeclPtr); auto *V = DeclPtr.emitRawPointer(*this); AllocaPtr = RawAddress(V, DeclPtr.getElementType(), DeclPtr.getAlignment()); // For truly ABI indirect arguments -- those that are not `byval` -- store // the address of the argument on the stack to preserve debug information. - ABIArgInfo ArgInfo = CurFnInfo->arguments()[ArgNo - 1].info; if (ArgInfo.isIndirect()) UseIndirectDebugAddress = !ArgInfo.getIndirectByVal(); if (UseIndirectDebugAddress) { @@ -2773,6 +3106,9 @@ void CodeGenFunction::EmitParmDecl(const VarDecl &D, ParamValue Arg, // Otherwise, create a casted temporary to hold the value. DeclPtr = CreateMemTemp(Ty, getContext().getDeclAlign(&D), D.getName() + ".addr", &AllocaPtr); + // This alloca holds a parameter, not an anonymous ABI slot, and it is on + // this frame, so cl.exe's GS analysis covers it. + EmitGSBufferStackProtectorMD(AllocaPtr.getPointer(), Ty); } DoStore = true; } diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp index aca0415d7f544..76eae97ada2f0 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -203,11 +203,15 @@ RawAddress CodeGenFunction::CreateMemTemp(QualType Ty, const Twine &Name, RawAddress CodeGenFunction::CreateMemTemp(QualType Ty, CharUnits Align, const Twine &Name, - RawAddress *Alloca) { + RawAddress *Alloca, + GSObjectKind GSKind) { RawAddress Result = CreateTempAlloca(ConvertTypeForMem(Ty), Ty.getAddressSpace(), Align, Name, /*ArraySize=*/nullptr, Alloca); + EmitGSBufferStackProtectorMD( + Alloca ? Alloca->getPointer() : Result.getPointer(), Ty, GSKind); + if (Ty->isConstantMatrixType()) { auto *ArrayTy = cast<llvm::ArrayType>(Result.getElementType()); auto *ArrayElementTy = ArrayTy->getElementType(); @@ -228,7 +232,11 @@ RawAddress CodeGenFunction::CreateMemTemp(QualType Ty, CharUnits Align, RawAddress CodeGenFunction::CreateMemTempWithoutCast(QualType Ty, CharUnits Align, const Twine &Name) { - return CreateTempAllocaWithoutCast(ConvertTypeForMem(Ty), Align, Name); + RawAddress Result = + CreateTempAllocaWithoutCast(ConvertTypeForMem(Ty), Align, Name); + EmitGSBufferStackProtectorMD(Result.getPointer(), Ty, + GSObjectKind::Temporary); + return Result; } RawAddress CodeGenFunction::CreateMemTempWithoutCast(QualType Ty, @@ -504,7 +512,13 @@ static RawAddress createReferenceTemporary(CodeGenFunction &CGF, // FIXME: Should we put the new global into a COMDAT? return RawAddress(C, GV->getValueType(), alignment); } - return CGF.CreateMemTemp(Ty, "ref.tmp", Alloca); + RawAddress Object = CGF.CreateMemTemp(Ty, "ref.tmp", Alloca); + // A temporary bound to a reference outlives the full-expression and has an + // address the program can pass around, so cl.exe treats it as a real local + // object rather than an anonymous ABI slot. + CGF.EmitGSBufferStackProtectorMD( + Alloca ? Alloca->getPointer() : Object.getPointer(), Ty); + return Object; } case SD_Thread: case SD_Static: diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp index 745423945ca83..81685f8c3c883 100644 --- a/clang/lib/CodeGen/CodeGenFunction.cpp +++ b/clang/lib/CodeGen/CodeGenFunction.cpp @@ -756,6 +756,11 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy, if (FD && FD->usesSEHTry()) CurSEHParent = GD; CurFuncDecl = (D ? D->getNonClosureContext() : nullptr); + // Under MSVC's /GS heuristic the frontend decides which allocas are buffers, + // so mark them as they are created. Only worth doing if the function ends up + // with a stack protector attribute at all. + MarkGSBuffers = CGM.useMSVCGSBufferHeuristic(D) && + CGM.StackProtectorAttribute(D).has_value(); FnRetTy = RetTy; CurFn = Fn; CurFnInfo = &FnInfo; diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index 653e883012229..404296cb9c262 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -1572,6 +1572,11 @@ class CodeGenFunction : public CodeGenTypeCache { /// calling llvm.stacksave for multiple VLAs in the same scope. bool DidCallStackSave = false; + /// Whether this function is compiled with MSVC's /GS heuristic, so that its + /// allocas have to be classified here rather than in the backend. See + /// EmitGSBufferStackProtectorMD. + bool MarkGSBuffers = false; + /// IndirectBranch - The first time an indirect goto is seen we create a block /// with an indirect branch. Every time we see the address of a label taken, /// we add the label to the indirect goto. Every subsequent indirect goto is @@ -2992,6 +2997,23 @@ class CodeGenFunction : public CodeGenTypeCache { /// ConvertType instead of ConvertTypeForMem. RawAddress CreateIRTempWithoutCast(QualType T, const Twine &Name = "tmp"); + /// What kind of stack object an alloca holds, for MSVC's /GS heuristic. + enum class GSObjectKind { + /// An object the source declares: a local variable, a by-value parameter, + /// or a temporary bound to a reference. Always classified. + Named, + /// A slot clang materialises to satisfy the ABI, such as an sret return + /// slot. cl.exe keeps these out of its GS analysis unless the type needs a + /// constructor or destructor to run, in which case it becomes a real local + /// object. + Temporary, + /// The copy of an outgoing by-value argument. Like Temporary, except that + /// cl.exe also leaves it out when the object cannot be copied bitwise: + /// then it has to be constructed in the outgoing argument area directly, + /// which lies outside the guarded region. + ArgumentCopy, + }; + /// CreateMemTemp - Create a temporary memory object of the given type, with /// appropriate alignmen and cast it to the default address space. Returns /// the original alloca instruction by \p Alloca if it is not nullptr. @@ -2999,7 +3021,8 @@ class CodeGenFunction : public CodeGenTypeCache { RawAddress *Alloca = nullptr); RawAddress CreateMemTemp(QualType T, CharUnits Align, const Twine &Name = "tmp", - RawAddress *Alloca = nullptr); + RawAddress *Alloca = nullptr, + GSObjectKind GSKind = GSObjectKind::Temporary); /// CreateMemTemp - Create a temporary memory object of the given type, with /// appropriate alignmen without casting it to the default address space. @@ -3010,8 +3033,10 @@ class CodeGenFunction : public CodeGenTypeCache { /// CreateAggTemp - Create a temporary memory object for the given /// aggregate type. AggValueSlot CreateAggTemp(QualType T, const Twine &Name = "tmp", - RawAddress *Alloca = nullptr) { - RawAddress Addr = CreateMemTemp(T, Name, Alloca); + RawAddress *Alloca = nullptr, + GSObjectKind GSKind = GSObjectKind::Temporary) { + RawAddress Addr = CreateMemTemp(T, getContext().getTypeAlignInChars(T), + Name, Alloca, GSKind); return AggValueSlot::forAddr( Addr, T.getQualifiers(), AggValueSlot::IsNotDestructed, AggValueSlot::DoesNotNeedGCBarriers, AggValueSlot::IsNotAliased, @@ -3587,6 +3612,29 @@ class CodeGenFunction : public CodeGenTypeCache { } }; AutoVarEmission EmitAutoVarAlloca(const VarDecl &var); + + /// If this function uses MSVC's /GS (Buffer Security Check) heuristic, + /// attach "stack-protector" metadata to \p AllocaPtr when \p Ty is a "GS + /// buffer". Whether it is depends on source-level type information that + /// lowering to IR does not preserve, so the backend cannot work it out. + void EmitGSBufferStackProtectorMD(llvm::Value *AllocaPtr, QualType Ty, + GSObjectKind Kind = GSObjectKind::Named); + + /// If this function uses MSVC's /GS (Buffer Security Check) heuristic and + /// \p Ty is a "GS buffer", copy the by-value parameter at \p ParamAddr into + /// a fresh local so that it is covered by this frame's stack guard, and + /// return the copy. cl.exe does the same, naming the copy "<param>$GSCopy$". + Address EmitGSBufferParamCopy(const VarDecl &D, Address ParamAddr); + + /// If this function uses MSVC's /GS (Buffer Security Check) heuristic, mark + /// \p AI as a "GS buffer", so that the function requires a stack protector. + /// \p IsLarge says the buffer should be laid out closest to the guard. + /// \p IsTrivial says the allocation holds an object of trivial type, which + /// cl.exe is willing to leave in the unguarded temporary area; see the + /// "stack-protector" metadata in LLVM's LangRef. + void MarkGSBufferAlloca(llvm::AllocaInst *AI, bool IsLarge, + bool IsTrivial = false); + void EmitAutoVarInit(const AutoVarEmission &emission); void EmitAutoVarCleanups(const AutoVarEmission &emission); void emitAutoVarTypeCleanup(const AutoVarEmission &emission, diff --git a/llvm/docs/LangRef.md b/llvm/docs/LangRef.md index 40d9183279a2b..99bc6e1c49248 100644 --- a/llvm/docs/LangRef.md +++ b/llvm/docs/LangRef.md @@ -2843,17 +2843,16 @@ fn -> other_fn -> other_fn ; fn is norecurse boolean value, and is emitted by clang-cl. Under this heuristic a function is protected if it takes no variable - argument list and it allocates a "GS buffer", which is any of: - - - an array that is larger than 4 bytes, has more than two elements, and - has an element type that is not a pointer type; - - a data structure whose size is more than 8 bytes and that contains no - pointers; - - a buffer allocated by `alloca()`, regardless of size; - - any class or structure that contains a GS buffer. + argument list and it allocates a "GS buffer", which is either an `alloca` + with a non-constant or non-one element count, or an alloca that the + frontend marked with non-zero `stack-protector` metadata. Whether a given + variable is a GS buffer depends on its source-level type, which lowering + to IR does not preserve, so classifying the fixed-size allocations is the + frontend's job. Unlike `sspstrong`, a local variable merely having its address taken does - not cause a function to be protected. Stack layout rules are unchanged. + not cause a function to be protected, and no alloca is classified from its + IR type. Stack layout rules are unchanged. If a function with an `ssp` or `sspstrong` attribute but no `"stack-protector-gs-buffer"` attribute is inlined into a calling function @@ -9211,18 +9210,37 @@ call ptr @malloc(i64 64), !alloc_token !0 #### '`stack-protector`' Metadata -The `stack-protector` metadata may be attached to alloca instructions. An -alloca instruction with this metadata and value `i32 0` will be skipped when -deciding whether a given function requires a stack protector. The function -may still use a stack protector, if other criteria determine it needs one. +The `stack-protector` metadata may be attached to alloca instructions. It +overrides, for that one allocation, the heuristic that the `ssp`, `sspstrong` +and `"stack-protector-gs-buffer"` function attributes would otherwise apply. +The function may still use a stack protector, if other criteria determine it +needs one. + +The metadata contains an integer, and optionally a second, boolean operand: + +- `0` opts the given alloca out of requiring a stack protector. +- `1` marks the given alloca as a buffer that requires a stack protector, and + that is laid out like a small (`< ssp-buffer-size`) array. +- `2` marks the given alloca as a buffer that requires a stack protector, and + that is laid out like a large (`>= ssp-buffer-size`) array. + +The non-zero values let a frontend classify an allocation itself, which is +necessary when the decision depends on source-level information that lowering +to IR does not preserve. -The metadata contains an integer, where a 0 value opts the given alloca out -of requiring a stack protector. +A second operand of `i1 true` says the allocation holds an object of trivial +type, one the backend is therefore free to relocate. Only the +`"stack-protector-gs-buffer"` heuristic reads it, and only to leave out an +allocation whose address is used for nothing but receiving the indirect return +value of a call: MSVC gives such an object a slot outside the region the stack +cookie guards. The operand may be omitted, and is then taken as `false`. ``` %a = alloca [1000 x i8], align 1, !stack-protector !0 + %b = alloca [1000 x i8], align 1, !stack-protector !1 !0 = !{i32 0} +!1 = !{i32 2, i1 true} ``` #### '`implicit.ref`' Metadata diff --git a/llvm/lib/CodeGen/StackProtector.cpp b/llvm/lib/CodeGen/StackProtector.cpp index 06158545e1215..05e6fa85fdd0e 100644 --- a/llvm/lib/CodeGen/StackProtector.cpp +++ b/llvm/lib/CodeGen/StackProtector.cpp @@ -67,12 +67,100 @@ static cl::opt<bool> DisableCheckNoReturn("disable-check-noreturn-call", /// Emitted by clang-cl. static constexpr const char *GSBufferAttr = "stack-protector-gs-buffer"; -/// An array is only an MSVC "GS buffer" if it is strictly larger than this. -static constexpr uint64_t MinGSArraySize = 4; +/// Values of the "stack-protector" metadata that may be attached to an alloca +/// to override the heuristic for that allocation. See LangRef. +enum SSPAllocaMD : uint32_t { + /// Ignore this alloca when deciding whether the function needs a protector. + SSPAMD_Ignore = 0, + /// This alloca is a buffer that requires a protector, and is laid out like a + /// small (< ssp-buffer-size) array. + SSPAMD_SmallBuffer = 1, + /// This alloca is a buffer that requires a protector, and is laid out like a + /// large (>= ssp-buffer-size) array. + SSPAMD_LargeBuffer = 2, +}; + +/// Read the "stack-protector" metadata of \p AI, if it has any. \p IsTrivial is +/// set from the optional second operand, which says the allocation holds an +/// object of trivial type. +static std::optional<uint32_t> getSSPAllocaMD(const AllocaInst &AI, + bool &IsTrivial) { + IsTrivial = false; + const MDNode *MD = AI.getMetadata("stack-protector"); + if (!MD || MD->getNumOperands() < 1 || MD->getNumOperands() > 2) + return std::nullopt; + const auto *CI = mdconst::dyn_extract<ConstantInt>(MD->getOperand(0)); + if (!CI) + return std::nullopt; + if (MD->getNumOperands() == 2) + if (const auto *Flag = mdconst::dyn_extract<ConstantInt>(MD->getOperand(1))) + IsTrivial = !Flag->isZero(); + // Clamp, so that a value from a newer producer errs towards more protection + // rather than less. + return CI->getLimitedValue(SSPAMD_LargeBuffer); +} -/// A pointer-free aggregate is only an MSVC "GS buffer" if it is strictly -/// larger than this. -static constexpr uint64_t MinGSAggregateSize = 8; +/// Returns true if the only thing \p AI's address is ever used for, besides +/// being read and written here, is receiving the indirect return value of a +/// call. +/// +/// cl.exe gives such an allocation a slot in the frame's temporary area, which +/// the cookie does not cover, and leaves it out of the GS analysis: the only +/// code that writes it from outside is a callee, and a callee writes exactly one +/// object of the type it returns. As soon as the address reaches anywhere else, +/// cl.exe relocates the object into the protected region instead, so look for +/// that here rather than in the frontend, which cannot see the uses. +/// +/// Only an object cl.exe is free to relocate can end up in that area, which is +/// why the caller also requires the allocation to hold a trivial object. +static bool isIndirectReturnSlot(const AllocaInst &AI) { + bool FoundSRet = false; + SmallVector<const Instruction *, 8> Worklist = {&AI}; + SmallPtrSet<const Instruction *, 8> Visited = {&AI}; + + while (!Worklist.empty()) { + for (const Use &U : Worklist.pop_back_val()->uses()) { + const auto *User = cast<Instruction>(U.getUser()); + + if (const auto *CB = dyn_cast<CallBase>(User)) { + const auto *II = dyn_cast<IntrinsicInst>(CB); + if (II && (II->isLifetimeStartOrEnd() || II->isAssumeLikeIntrinsic())) + continue; + // A memory intrinsic reads or writes through the pointer, just as a + // load or a store does, and does not let it escape. + if (isa_and_nonnull<MemIntrinsic>(II)) + continue; + if (!CB->isArgOperand(&U) || + !CB->paramHasAttr(CB->getArgOperandNo(&U), Attribute::StructRet)) + return false; + FoundSRet = true; + continue; + } + + switch (User->getOpcode()) { + case Instruction::Load: + continue; + case Instruction::Store: + // Writing through the pointer is fine; writing the pointer itself + // somewhere else lets it escape. + if (U.getOperandNo() != StoreInst::getPointerOperandIndex()) + return false; + continue; + case Instruction::BitCast: + case Instruction::AddrSpaceCast: + case Instruction::GetElementPtr: + case Instruction::PHI: + case Instruction::Select: + if (Visited.insert(User).second) + Worklist.push_back(User); + continue; + default: + return false; + } + } + } + return FoundSRet; +} /// InsertStackProtectors - Insert code into the prologue and epilogue of the /// function. @@ -286,75 +374,6 @@ static bool ContainsProtectableArray(Type *Ty, Module *M, unsigned SSPBufferSize return NeedsProtector; } -/// Returns true if \p Ty holds a pointer anywhere within it. MSVC's GS-buffer -/// rules treat a pointer-free aggregate as something that may be used as a -/// buffer, so the presence of a pointer is what disqualifies it. -static bool ContainsPointer(Type *Ty) { - if (Ty->isPointerTy()) - return true; - if (auto *AT = dyn_cast<ArrayType>(Ty)) - return ContainsPointer(AT->getElementType()); - if (auto *ST = dyn_cast<StructType>(Ty)) - return any_of(ST->elements(), ContainsPointer); - return false; -} - -/// Returns true if \p Ty is, or contains, a "GS buffer" as defined by MSVC's -/// /GS (Buffer Security Check) documentation: -/// -/// - an array that is larger than 4 bytes, has more than two elements, and -/// has an element type that is not a pointer type; -/// - a data structure whose size is more than 8 bytes and that contains no -/// pointers; -/// - any class or structure that contains a GS buffer. -/// -/// \param [out] IsLarge is set to true if the GS buffer that was found is -/// "large" (>= ssp-buffer-size), so that it is laid out closest to the stack -/// guard. In an aggregate holding several GS buffers this is set if any of -/// them is large. -static bool ContainsGSBuffer(Type *Ty, Module *M, unsigned SSPBufferSize, - bool &IsLarge) { - if (!Ty) - return false; - - const DataLayout &DL = M->getDataLayout(); - auto Found = [&](Type *BufferTy) { - if (DL.getTypeAllocSize(BufferTy).getKnownMinValue() >= SSPBufferSize) - IsLarge = true; - return true; - }; - - if (auto *AT = dyn_cast<ArrayType>(Ty)) { - if (!AT->getElementType()->isPointerTy() && AT->getNumElements() > 2 && - DL.getTypeAllocSize(AT).getKnownMinValue() > MinGSArraySize) - return Found(AT); - - // The array itself is not a GS buffer, but its elements may still be or - // contain one, e.g. an array of two structs that each hold a buffer. - return ContainsGSBuffer(AT->getElementType(), M, SSPBufferSize, IsLarge); - } - - auto *ST = dyn_cast<StructType>(Ty); - if (!ST || ST->isOpaque()) - return false; - - if (DL.getTypeAllocSize(ST).getKnownMinValue() > MinGSAggregateSize && - !ContainsPointer(ST)) - return Found(ST); - - bool NeedsProtector = false; - for (Type *ET : ST->elements()) - if (ContainsGSBuffer(ET, M, SSPBufferSize, IsLarge)) { - // If a member is a large GS buffer then we are done. Otherwise keep - // looking, in case a later member is a large one. - if (IsLarge) - return true; - NeedsProtector = true; - } - - return NeedsProtector; -} - /// Maximum remaining allocation size observed for a phi node, and how often /// the allocation size has already been decreased. We only allow a limited /// number of decreases. @@ -496,9 +515,15 @@ static const CallInst *findStackProtectorIntrinsic(Function &F) { /// size, and functions that contain stack-based variables that have had their /// address taken. The MSVC heuristic implements /GS (Buffer Security Check): /// it adds a guard variable to functions that call alloca regardless of size -/// and to functions holding a "GS buffer" (see ContainsGSBuffer), but it does -/// not consider a variable's address being taken, and it never protects a -/// function that takes a variable argument list. +/// and to functions with an alloca that the frontend marked as a "GS buffer" +/// using "stack-protector" metadata, but it does not consider a variable's +/// address being taken, and it never protects a function that takes a variable +/// argument list. Deciding what a GS buffer is needs source-level type +/// information that lowering to IR does not preserve, which is why that part +/// of the heuristic lives in the frontend. +/// +/// In every mode, "stack-protector" metadata on an alloca overrides the +/// heuristic for that allocation. bool SSPLayoutAnalysis::requiresStackProtector(Function *F, SSPLayoutMap *Layout) { Module *M = F->getParent(); @@ -552,11 +577,37 @@ bool SSPLayoutAnalysis::requiresStackProtector(Function *F, for (const BasicBlock &BB : *F) { for (const Instruction &I : BB) { if (const AllocaInst *AI = dyn_cast<AllocaInst>(&I)) { - if (const MDNode *MD = AI->getMetadata("stack-protector")) { - const auto *CI = mdconst::dyn_extract<ConstantInt>(MD->getOperand(0)); - if (CI->isZero()) + bool IsTrivial = false; + if (std::optional<uint32_t> MD = getSSPAllocaMD(*AI, IsTrivial)) { + if (*MD == SSPAMD_Ignore) continue; + + // MSVC leaves a trivial object that is only an indirect-return + // destination out of its GS analysis. + if (GSBuffer && IsTrivial && isIndirectReturnSlot(*AI)) + continue; + + // The frontend classified this allocation as a buffer, which is + // final: it knows things about the allocation that the IR no longer + // records. This is how clang-cl communicates MSVC's "GS buffer" + // analysis. + if (!Layout) + return true; + Layout->insert( + std::make_pair(AI, *MD == SSPAMD_LargeBuffer + ? MachineFrameInfo::SSPLK_LargeArray + : MachineFrameInfo::SSPLK_SmallArray)); + ORE.emit([&]() { + return OptimizationRemark(DEBUG_TYPE, "StackProtectorBuffer", &I) + << "Stack protection applied to function " + << ore::NV("Function", F) + << " due to a stack allocated buffer or struct containing a " + "buffer"; + }); + NeedsProtector = true; + continue; } + if (AI->isArrayAllocation()) { auto RemarkBuilder = [&]() { return OptimizationRemark(DEBUG_TYPE, "StackProtectorAllocaOrArray", @@ -599,14 +650,14 @@ bool SSPLayoutAnalysis::requiresStackProtector(Function *F, continue; } + // Under MSVC's /GS the frontend has already picked out the buffers, so + // an unmarked alloca never requires a protector. + if (GSBuffer) + continue; + bool IsLarge = false; - bool Protect = - GSBuffer ? ContainsGSBuffer(AI->getAllocatedType(), M, - SSPBufferSize, IsLarge) - : ContainsProtectableArray(AI->getAllocatedType(), M, - SSPBufferSize, IsLarge, Strong, - /*InStruct=*/false); - if (Protect) { + if (ContainsProtectableArray(AI->getAllocatedType(), M, SSPBufferSize, + IsLarge, Strong, /*InStruct=*/false)) { if (!Layout) return true; Layout->insert(std::make_pair( diff --git a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp index 74322d599bd28..3d4090b6f0ae1 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp @@ -199,6 +199,9 @@ static Instruction *simplifyAllocaArraySize(InstCombinerImpl &IC, nullptr, AI.getName()); New->setAlignment(AI.getAlign()); New->setUsedWithInAlloca(AI.isUsedWithInAlloca()); + // It is the same allocation, so it keeps what was said about it, e.g. + // "stack-protector". + New->copyMetadata(AI); replaceAllDbgUsesWith(AI, *New, *New, DT); return IC.replaceInstUsesWith(AI, New); diff --git a/llvm/test/CodeGen/X86/stack-protector-gs-buffer.ll b/llvm/test/CodeGen/X86/stack-protector-gs-buffer.ll index a753e1b3bf65d..c87a14b2186b2 100644 --- a/llvm/test/CodeGen/X86/stack-protector-gs-buffer.ll +++ b/llvm/test/CodeGen/X86/stack-protector-gs-buffer.ll @@ -1,167 +1,202 @@ ; Check MSVC's /GS (Buffer Security Check) heuristic, selected by the ; "stack-protector-gs-buffer" function attribute. ; -; A "GS buffer" is an array larger than 4 bytes with more than two elements and -; a non-pointer element type, a pointer-free aggregate larger than 8 bytes, an -; alloca of any size, or any aggregate containing one of those. +; Under this heuristic the only allocas that require a protector are calls to +; alloca and the ones the frontend marked with "stack-protector" metadata; +; nothing is inferred from the alloca's type. ; ; RUN: llc -mtriple=x86_64-pc-windows-msvc < %s | FileCheck %s declare void @use(ptr) +declare void @make(ptr sret([64 x i8])) +declare void @llvm.memcpy.p0.p0.i64(ptr, ptr, i64, i1) -;; --- Arrays that are not GS buffers ------------------------------------------ +;; --- Unmarked allocas -------------------------------------------------------- -; Exactly 4 bytes, so not larger than 4. -; CHECK-LABEL: array_4_bytes: +; An obvious buffer is not protected unless the frontend says so, because the +; IR type is not what the heuristic is defined in terms of. +; CHECK-LABEL: unmarked_array: ; CHECK-NOT: __security_cookie ; CHECK: .seh_endproc -define void @array_4_bytes() #0 { - %a = alloca [4 x i8] +define void @unmarked_array() #0 { + %a = alloca [64 x i8] call void @use(ptr %a) ret void } -; 8 bytes, but only two elements. -; CHECK-LABEL: array_2_elements: +; Unlike sspstrong, merely taking a local's address does not protect a function. +; CHECK-LABEL: address_taken: ; CHECK-NOT: __security_cookie ; CHECK: .seh_endproc -define void @array_2_elements() #0 { - %a = alloca [2 x i32] +define void @address_taken() #0 { + %a = alloca i32 call void @use(ptr %a) ret void } -; Large and many elements, but the element type is a pointer. -; CHECK-LABEL: array_of_pointers: -; CHECK-NOT: __security_cookie -; CHECK: .seh_endproc -define void @array_of_pointers() #0 { - %a = alloca [8 x ptr] +;; --- Marked allocas ---------------------------------------------------------- + +; CHECK-LABEL: marked_small: +; CHECK: __security_cookie +define void @marked_small() #0 { + %a = alloca [6 x i8], !stack-protector !1 call void @use(ptr %a) ret void } -;; --- Arrays that are GS buffers ---------------------------------------------- - -; CHECK-LABEL: array_8_bytes: +; CHECK-LABEL: marked_large: ; CHECK: __security_cookie -define void @array_8_bytes() #0 { - %a = alloca [8 x i8] +define void @marked_large() #0 { + %a = alloca [64 x i8], !stack-protector !2 call void @use(ptr %a) ret void } -; 6 bytes and three elements: over both thresholds. -; CHECK-LABEL: array_3_shorts: +; The mark is what matters, not the type: an i32 carrying it is protected. +; CHECK-LABEL: marked_scalar: ; CHECK: __security_cookie -define void @array_3_shorts() #0 { - %a = alloca [3 x i16] +define void @marked_scalar() #0 { + %a = alloca i32, !stack-protector !1 call void @use(ptr %a) ret void } -; Two elements, so the array itself is not a GS buffer, but each element is. -; CHECK-LABEL: array_2_buffers: -; CHECK: __security_cookie -define void @array_2_buffers() #0 { - %a = alloca [2 x [8 x i8]] +; A zero mark opts out, as it does in every other mode. +; CHECK-LABEL: marked_ignore: +; CHECK-NOT: __security_cookie +; CHECK: .seh_endproc +define void @marked_ignore() #0 { + %a = alloca [64 x i8], !stack-protector !0 call void @use(ptr %a) ret void } -;; --- Aggregates -------------------------------------------------------------- +;; --- alloca ------------------------------------------------------------------ -; 12 bytes with no pointers. -; CHECK-LABEL: struct_pointer_free: +; A call to alloca is a GS buffer, and is visible as such in the IR, so it +; needs no mark. +; CHECK-LABEL: dynamic_alloca: ; CHECK: __security_cookie -define void @struct_pointer_free() #0 { - %a = alloca { i32, i32, i32 } +define void @dynamic_alloca(i64 %n) #0 { + %a = alloca i8, i64 %n call void @use(ptr %a) ret void } -; 16 bytes, but it holds a pointer, so it is not itself a GS buffer. -; CHECK-LABEL: struct_with_pointer: +; Even a small constant-sized alloca is a GS buffer. +; CHECK-LABEL: small_alloca: +; CHECK: __security_cookie +define void @small_alloca() #0 { + %a = alloca i8, i64 3 + call void @use(ptr %a) + ret void +} + +;; --- Indirect return slots --------------------------------------------------- + +; MSVC gives an object it is free to relocate and that only ever receives a +; call's indirect return value a frame slot the cookie does not guard. The +; frontend cannot see that, so the second metadata operand tells the backend the +; object is trivial and the backend checks the uses. +; CHECK-LABEL: sret_only: ; CHECK-NOT: __security_cookie ; CHECK: .seh_endproc -define void @struct_with_pointer() #0 { - %a = alloca { ptr, i32, i32 } - call void @use(ptr %a) +define void @sret_only() #0 { + %a = alloca [64 x i8], !stack-protector !3 + call void @make(ptr sret([64 x i8]) %a) + %v = load i8, ptr %a + store i8 %v, ptr %a ret void } -; Exactly 8 bytes, so not larger than 8. -; CHECK-LABEL: struct_8_bytes: +; Reading and writing through the pointer, including with a memcpy, is not an +; escape, and several returns into the same slot are still just returns. +; CHECK-LABEL: sret_twice_and_memcpy: ; CHECK-NOT: __security_cookie ; CHECK: .seh_endproc -define void @struct_8_bytes() #0 { - %a = alloca { i32, i32 } - call void @use(ptr %a) +define void @sret_twice_and_memcpy(ptr %p) #0 { + %a = alloca [64 x i8], !stack-protector !3 + call void @make(ptr sret([64 x i8]) %a) + call void @make(ptr sret([64 x i8]) %a) + call void @llvm.memcpy.p0.p0.i64(ptr %p, ptr %a, i64 64, i1 false) ret void } -; Holds a pointer, but also contains a GS buffer. -; CHECK-LABEL: struct_containing_buffer: +; As soon as the address reaches anywhere else, MSVC moves the object into the +; guarded region instead. +; CHECK-LABEL: sret_escapes: ; CHECK: __security_cookie -define void @struct_containing_buffer() #0 { - %a = alloca { ptr, [8 x i8] } +define void @sret_escapes() #0 { + %a = alloca [64 x i8], !stack-protector !3 + call void @make(ptr sret([64 x i8]) %a) call void @use(ptr %a) ret void } -; The GS buffer is two aggregates down. -; CHECK-LABEL: struct_nested_buffer: +; A derived pointer escaping counts just the same. +; CHECK-LABEL: sret_gep_escapes: ; CHECK: __security_cookie -define void @struct_nested_buffer() #0 { - %a = alloca { ptr, { i32, [8 x i8] } } - call void @use(ptr %a) +define void @sret_gep_escapes() #0 { + %a = alloca [64 x i8], !stack-protector !3 + call void @make(ptr sret([64 x i8]) %a) + %g = getelementptr [64 x i8], ptr %a, i64 0, i64 8 + call void @use(ptr %g) ret void } -;; --- alloca ------------------------------------------------------------------ - -; CHECK-LABEL: dynamic_alloca: +; Storing the pointer itself is an escape; storing through it is not. +; CHECK-LABEL: sret_pointer_stored: ; CHECK: __security_cookie -define void @dynamic_alloca(i64 %n) #0 { - %a = alloca i8, i64 %n - call void @use(ptr %a) +define void @sret_pointer_stored(ptr %p) #0 { + %a = alloca [64 x i8], !stack-protector !3 + call void @make(ptr sret([64 x i8]) %a) + store ptr %a, ptr %p ret void } -; Even a small constant-sized alloca is a GS buffer. -; CHECK-LABEL: small_alloca: +; Without an indirect return there is nothing to exempt. +; CHECK-LABEL: trivial_without_sret: ; CHECK: __security_cookie -define void @small_alloca() #0 { - %a = alloca i8, i64 3 - call void @use(ptr %a) +define void @trivial_without_sret() #0 { + %a = alloca [64 x i8], !stack-protector !3 + store i8 0, ptr %a ret void } -;; --- Exclusions -------------------------------------------------------------- +; The exemption needs the object to be one MSVC may relocate, which is what the +; second operand says. Without it the slot is protected. +; CHECK-LABEL: nontrivial_sret: +; CHECK: __security_cookie +define void @nontrivial_sret() #0 { + %a = alloca [64 x i8], !stack-protector !2 + call void @make(ptr sret([64 x i8]) %a) + ret void +} -; Unlike sspstrong, merely taking a local's address does not protect a function. -; CHECK-LABEL: address_taken: -; CHECK-NOT: __security_cookie -; CHECK: .seh_endproc -define void @address_taken() #0 { - %a = alloca i32 - call void @use(ptr %a) +; The exemption is specific to the /GS heuristic. +; CHECK-LABEL: strong_sret: +; CHECK: __security_cookie +define void @strong_sret() #2 { + %a = alloca [64 x i8], !stack-protector !3 + call void @make(ptr sret([64 x i8]) %a) ret void } +;; --- Exclusions -------------------------------------------------------------- + ; MSVC never protects a function that takes a variable argument list, even one ; holding an obvious GS buffer. ; CHECK-LABEL: variadic: ; CHECK-NOT: __security_cookie ; CHECK: .seh_endproc define void @variadic(i32 %n, ...) #0 { - %a = alloca [64 x i8] + %a = alloca [64 x i8], !stack-protector !2 call void @use(ptr %a) ret void } -; sspreq overrides the heuristic entirely, so the varargs exclusion and the -; GS-buffer rules do not apply. +; sspreq overrides the heuristic entirely, so the varargs exclusion does not +; apply. ; CHECK-LABEL: sspreq_wins: ; CHECK: __security_cookie define void @sspreq_wins(i32 %n, ...) #1 { @@ -180,6 +215,21 @@ define void @strong_still_protects() #2 { ret void } +; The marker is honoured under the other heuristics too: sspstrong would not +; protect this function on its own. +; CHECK-LABEL: strong_honours_mark: +; CHECK: __security_cookie +define void @strong_honours_mark() #2 { + %a = alloca i32, !stack-protector !1 + store i32 0, ptr %a + ret void +} + attributes #0 = { sspstrong uwtable "stack-protector-gs-buffer"="true" } attributes #1 = { sspreq uwtable "stack-protector-gs-buffer"="true" } attributes #2 = { sspstrong uwtable } + +!0 = !{i32 0} +!1 = !{i32 1} +!2 = !{i32 2} +!3 = !{i32 2, i1 true} diff --git a/llvm/test/Transforms/InstCombine/alloca.ll b/llvm/test/Transforms/InstCombine/alloca.ll index 15b43e780f149..e2113fa648509 100644 --- a/llvm/test/Transforms/InstCombine/alloca.ll +++ b/llvm/test/Transforms/InstCombine/alloca.ll @@ -259,3 +259,21 @@ define void @test_inalloca_with_element_count(ptr %a) { call void @test9_aux(ptr inalloca(%struct_type) %alloca) ret void } + +; Folding the element count into the allocated type must not lose what was +; said about the allocation. +define void @test_metadata_is_preserved() { +; ALL-LABEL: @test_metadata_is_preserved( +; ALL-NEXT: [[ALLOCA1:%.*]] = alloca [10 x i8], align 1, !stack-protector [[META0:![0-9]+]] +; ALL-NEXT: call void (...) @use(ptr nonnull [[ALLOCA1]]) +; ALL-NEXT: ret void +; + %alloca = alloca i8, i32 10, !stack-protector !0 + call void (...) @use(ptr %alloca) + ret void +} + +!0 = !{i32 2} +;. +; ALL: [[META0]] = !{i32 2} +;. _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
