[Stack Protection] Add diagnostic information for why stack protection was applied to a function

[jhenderson]

Summary

Stack Smash Protection is not completely free, so in hot code, the overhead it causes can cause performance issues. By adding diagnostic information for which function have SSP and why, a user can quickly determine what they can do to stop SSP being applied to a specific hot function.

This change adds a remark that is reported by the stack protection code when an instruction or attribute is encountered that causes SSP to be applied.

[anemet]

I think that you want to only add the new DiagnosticKind to the IR layer. The class itself should reside in CodeGen. You should then be able to keep the diagnostic and the code producing it closer. see what I did in D29004.

You also have seemingly unrelated test changes in this patch. Which reminds me; you need to add a test.

lib/IR/DiagnosticInfo.cpp

343–344 ↗

(On Diff #85368)

This will produce a pretty cryptic message.

[jhenderson]

Thanks for spotting the test file. I did not mean for that file to be in there (the perils of making local changes...).

I have moved the class into StackProtector.h as suggested. I've also converted the enum into a string when printing. I'm uploading these changes now, and will add tests to it next.

[george.burgess.iv]

Thanks for working on this!

lib/CodeGen/StackProtector.cpp

485 ↗

(On Diff #85409)

Nit: Can we use a StringRef here instead?

[jhenderson]

I have switched to using StringRef as suggested and added a test to check that the diagnostic is issued in each different case.

I also made several small changes based on comments in D29027: removed unused Unknown reason; added an end marker to the enum that can be used by the clang-side diagnostics when deciding if stack protection was applied due to a function attribute or command-line switch (i.e. -fstack-protector-all); finally, added to alloca case that it could be a variable length array that caused stack protection to be applied (this is indistinguishable at this point from an explicit use of allloca).

[jhenderson]

Ping

[jhenderson]

Ping!

[anemet]

I can't really judge the SSP part but the overall approach looks good to me. Maybe wait a day before committing.

[davidb]

The current implementation will result in a remark always being emitted for llc, so we need to make a change to prevent that.

[davidb]

remarks need to be suppressed by default.

[jhenderson]

It turns out that by default, when no handlers are provided, all diagnostic messages are produced, regardless of their severity. Optimization remarks are a special case that have explicit handling. I can think of five different ways to fix this for my case:

1. Make DiagnosticInfoSSP derive from DiagnosticInfoOptimizationBase, and then provide a new switch to explicitly enable it.
2. Remove DiagnosticInfoSSP entirely and use emitOptimizationRemark. The remarks would then be emitted by specifying something like -pass-remarks=stack-protector. I haven't investigated this, but this might make the clang-side changes unnecessary.
3. Move the isEnabled() virtual function of DiagnosticInfoOptimizationBase up the inheritance hierarchy so that a greater range of subclasses can use the relevant behaviour. Potentially, move it all the way to the top level DiagnosticInfo class and default it to always be enabled, allowing subclasses to override it as desired.
4. Disable all diagnostics with remark severity by default, either via the implementation in 3) or by querying the severity in "isDiagnosticEnabled" in LLVMContext.cpp, and explicitly enable them only as requested. This would then need to be paired up with a new option to enable all remarks, or at least a switch to enable the new diagnostic itself. This would be similar to what clang does.
5. Always emitting this diagnostic is actually fine and so the failing tests need fixing.

Stack Protection is a pass, but it isn't really an optimization (in fact the motivation behind the change is to enable users to analyse where it has been applied and find ways to suppress it), so using something with Optimization in the name to emit it doesn't feel right to me. To me, 3) or 4) seem like the overall correct approach. As far as I can see, the only DiagnosticInfo users with DS_Remark severity all go through the DiagnosticInfoOptimizationBase class, so there wouldn't be any unintentional side effects of disabling remarks by default, since these are disabled already.

I guess the question is, should remarks be disabled by default? What do people think?

[jhenderson]

After staring at this for a while, and discussing terminology, I have come to the conclusion that the best way to report these remarks is not via a new diagnostic class, but rather through simply using emitOptimizationRemark. This updated diff simplifies things substantially from the previous version and also gives us clang support for free via -Rpass=stack-protector.

[anemet]

In D29023#683484, @jhenderson wrote:

After staring at this for a while, and discussing terminology, I have come to the conclusion that the best way to report these remarks is not via a new diagnostic class, but rather through simply using emitOptimizationRemark. This updated diff simplifies things substantially from the previous version and also gives us clang support for free via -Rpass=stack-protector.

I was going to propose the same thing but I am pretty biased toward opt-remarks so I decided against it. I am glad you came to the same conclusion independently ;).

One thing that you should use is the OptimizationRemarkEmitter facility instead of using the C API. This allows exporting opt remarks into a YAML file with -fsave-optimization-record. Then you can visualize with opt-viewer like this:

https://androm3da.github.io/optviewer-demo/output_analysis/cpython/._Modules_hashtable.c.html#L493

Looks like LoopInfo/DomintorTree is not available in SSP so it's probably more efficient for now to instantiate ORE on the fly rather than using it as analysis pass. Once something like D30128 is implemented for IR-level ORE, I can come back to this and make it use the analysis pass. The same approach is used by the loop passes currently, see LegacyLICMPass::runOnLoop for an example.

[jhenderson]

Thanks, I've updated accordingly. I also added a test case to check that the remark is not emitted if the switch isn't specified.

I had to constify an argument to one of the OptimizationRemarkEmitter::emitOptimizationRemark overloads, since it was missing. I'm happy to add const to the others as appropriate as well, but that's probably a different change.

[anemet]

lib/CodeGen/StackProtector.cpp

226 ↗

(On Diff #89491)

Please add a comment that we're constructing ORE on the fly rather than using through the analysis pass to avoid building DominatorTree and LoopInfo which is not available this late in the IR pipeline.

231–233 ↗

(On Diff #89491)

OK, clearly we had too many APIs driving this functionality, and not too surprisingly, you didn't pick the right ones. So I went ahead and cleaned up the APIs (e.g. r296019, r296037).

This is not the ctor you want to use (and after my changes, you can't). Please use one of the two that are available now.

254 ↗

(On Diff #89491)

Same here, I removed this legacy interface. Please use ORE.emit().

[jhenderson]

Addressed comments.

I'm a little unsure about the comment I've added, as I am not familiar with those aspects of the compiler, being new to the scene. Can you confirm that it looks ok? I noticed in particular that there is some DominatorTree information being passed around elsewhere in the stack protector code, but I don't know if this is relevant at all.

Also, I'm not a particular fan of what I've had to do to emit the "function attribute or command-line switch" remark. In the event that there are no basic blocks, the remark is not emitted, but the rest of the stack protection code is still run (although I don't think it does much - there's perhaps a separate opportunity to bail out early, but I don't want to risk changing behaviour in this change). That particular version of the remark feels like a function-level remark, whereas from what I can see, the two constructors only really support basic block-level and instruction-level remarks. Would it make sense to have a third constructor in OptimizationRemark that takes a Function instead? It seems to be easy to add, but I don't know if it fits the wider architecture, from your point of view.

[anemet]

In D29023#685673, @jhenderson wrote:

Addressed comments.

I'm a little unsure about the comment I've added, as I am not familiar with those aspects of the compiler, being new to the scene. Can you confirm that it looks ok? I noticed in particular that there is some DominatorTree information being passed around elsewhere in the stack protector code, but I don't know if this is relevant at all.

Looks good. Yes, DT is used *if* it's available and currently it's not available by the nature of how the passes are ordered.

Also, I'm not a particular fan of what I've had to do to emit the "function attribute or command-line switch" remark. In the event that there are no basic blocks, the remark is not emitted, but the rest of the stack protection code is still run (although I don't think it does much - there's perhaps a separate opportunity to bail out early, but I don't want to risk changing behaviour in this change). That particular version of the remark feels like a function-level remark, whereas from what I can see, the two constructors only really support basic block-level and instruction-level remarks. Would it make sense to have a third constructor in OptimizationRemark that takes a Function instead? It seems to be easy to add, but I don't know if it fits the wider architecture, from your point of view.

Yes it's a good idea to add it. We should probably assert in there that !F.empty().

lib/CodeGen/StackProtector.cpp

239 ↗

(On Diff #89655)

Convention for this is camel-case: StackProtectorReason. I think it's documented in the comments. If not feel free to add it.

242 ↗

(On Diff #89655)

We don't run function passes on declarations which I think is the only reason that this can be empty (i.e. F.isDeclaration()).

245 ↗

(On Diff #89655)

You can just << these one by one:

<< ReasonStub << "a func... ";

259–261 ↗

(On Diff #89655)

Since you create the remark unconditionally you may as well pipe the the reason into it.

[jhenderson]

Addressed review comments.

[jhenderson]

lib/CodeGen/StackProtector.cpp

239 ↗

(On Diff #89655)

Done. Turns out it was commented in one place, but not in the place I used. I've added it to every instance.

245 ↗

(On Diff #89655)

Done. I originally used this because the operator<< is not overloaded for Twine arguments, so I assumed it would be better to concatenate them together first.

[anemet]

LGTM with these changes.

lib/CodeGen/StackProtector.cpp

238 ↗

(On Diff #89856)

Start with upper case.

240 ↗

(On Diff #89856)

You want different remark names for these. That helps calculating statistics on them without parsing the text.

lib/IR/DiagnosticInfo.cpp

239 ↗

(On Diff #89856)

Convention is to start function names with a verb, i.e. getFirstFunctionBlock.

test/CodeGen/X86/stack-protector-remarks.ll

3 ↗

(On Diff #89856)

This should be two lines down. (CHECK-NOTs are matches only within the partition set by the neighboring CHECKs.)

[jhenderson]

Thanks for the help. I've made the changes locally. I'll get a colleague with commit access to commit this later today hopefully.

I suspect that I might not be the only one to misunderstand CHECK-NOT!

[jhenderson]

Addressed review comments prior to committing.

[jhenderson]

So this change went in, but the PPC build bots failed because the test does not specify a target triple and llc caused the alloca commands to get discarded due to running different passes, before the stack protector pass was run. This meant that the test failed, as the remarks were never emitted. We fixed that by specifying the X86 target triple, but a colleague pointed out that this test isn't really X86 specific, so should be moved to the generic CodeGen folder. They also suggested using opt to run just the stack protector pass, to allow the test to be target independent. Unfortunately, due to the stack protector pass requiring a target to be run and opt not following the approach of treating unknown or unspecified targets as the current machine (unlike llc), attempting to run the pass causes a crash in opt, unless a target triple is specified (which defeats the purpose of using opt anyway). Indeed, I cannot see a way of modifying anything to get this pass to work with opt, short of changing opt's behaviour when no triple is specified to explicitly calculate one.

@anemet - do you have any thoughts on how to make the testing more generic?

[anemet]

In D29023#696542, @jhenderson wrote:

So this change went in, but the PPC build bots failed because the test does not specify a target triple and llc caused the alloca commands to get discarded due to running different passes, before the stack protector pass was run. This meant that the test failed, as the remarks were never emitted. We fixed that by specifying the X86 target triple, but a colleague pointed out that this test isn't really X86 specific, so should be moved to the generic CodeGen folder. They also suggested using opt to run just the stack protector pass, to allow the test to be target independent. Unfortunately, due to the stack protector pass requiring a target to be run and opt not following the approach of treating unknown or unspecified targets as the current machine (unlike llc), attempting to run the pass causes a crash in opt, unless a target triple is specified (which defeats the purpose of using opt anyway). Indeed, I cannot see a way of modifying anything to get this pass to work with opt, short of changing opt's behaviour when no triple is specified to explicitly calculate one.

Were these public bots?

@anemet - do you have any thoughts on how to make the testing more generic?

I think that we should just pin it to a target. This is not testing the basic functionality but whether we provide diagnostics about it.

[jhenderson]

In D29023#696643, @anemet wrote:

Were these public bots?

Yes - it was some of the PPC bots, although I don't know which ones off the top of my head; possibly all of them.

@anemet - do you have any thoughts on how to make the testing more generic?

I think that we should just pin it to a target. This is not testing the basic functionality but whether we provide diagnostics about it.

Ok, that's what we ended up doing (we put a REQUIRES: X86 in the tests along with relevant target triple).

llvm/trunk/include/llvm/IR/DiagnosticInfo.h

struct Argument {
     Argument(StringRef Key, const Value *V);
     Argument(StringRef Key, const Type *T);
     Argument(StringRef Key, int N);
     Argument(StringRef Key, unsigned N);
     Argument(StringRef Key, bool B) : Key(Key), Val(B ? "true" : "false") {}
   };
 
   /// \p PassName is the name of the pass emitting this diagnostic. \p
-  /// RemarkName is a textual identifier for the remark.  \p Fn is the function
-  /// where the diagnostic is being emitted. \p Loc is the location information
-  /// to use in the diagnostic. If line table information is available, the
-  /// diagnostic will include the source code location.
+  /// RemarkName is a textual identifier for the remark (single-word,
+  /// camel-case). \p Fn is the function where the diagnostic is being emitted.
+  /// \p Loc is the location information to use in the diagnostic. If line table
+  /// information is available, the diagnostic will include the source code
+  /// location.
   DiagnosticInfoOptimizationBase(enum DiagnosticKind Kind,
                                  enum DiagnosticSeverity Severity,
                                  const char *PassName, StringRef RemarkName,
                                  const Function &Fn,
                                  const DiagnosticLocation &Loc)
       : DiagnosticInfoWithLocationBase(Kind, Severity, Fn, Loc),
         PassName(PassName), RemarkName(RemarkName) {}
 
public:
   }
 
 protected:
   /// Name of the pass that triggers this report. If this matches the
   /// regular expression given in -Rpass=regexp, then the remark will
   /// be emitted.
   const char *PassName;
 
-  /// Textual identifier for the remark.  Can be used by external tools reading
-  /// the YAML output file for optimization remarks to identify the remark.
+  /// Textual identifier for the remark (single-word, camel-case). Can be used
+  /// by external tools reading the YAML output file for optimization remarks to
+  /// identify the remark.
   StringRef RemarkName;
 
   /// If profile information is available, this is the number of times the
   /// corresponding code was executed in a profile instrumentation run.
   Optional<uint64_t> Hotness;
 
   /// Arguments collected via the streaming interface.
   SmallVector<Argument, 4> Args;
protected:
   friend struct yaml::MappingTraits<DiagnosticInfoOptimizationBase *>;
 };
 
 /// \brief Common features for diagnostics dealing with optimization remarks
 /// that are used by IR passes.
 class DiagnosticInfoIROptimization : public DiagnosticInfoOptimizationBase {
 public:
   /// \p PassName is the name of the pass emitting this diagnostic. \p
-  /// RemarkName is a textual identifier for the remark.  \p Fn is the function
-  /// where the diagnostic is being emitted. \p Loc is the location information
-  /// to use in the diagnostic. If line table information is available, the
-  /// diagnostic will include the source code location. \p CodeRegion is IR
-  /// value (currently basic block) that the optimization operates on.  This is
-  /// currently used to provide run-time hotness information with PGO.
+  /// RemarkName is a textual identifier for the remark (single-word,
+  /// camel-case). \p Fn is the function where the diagnostic is being emitted.
+  /// \p Loc is the location information to use in the diagnostic. If line table
+  /// information is available, the diagnostic will include the source code
+  /// location. \p CodeRegion is IR value (currently basic block) that the
+  /// optimization operates on. This is currently used to provide run-time
+  /// hotness information with PGO.
   DiagnosticInfoIROptimization(enum DiagnosticKind Kind,
                                enum DiagnosticSeverity Severity,
                                const char *PassName, StringRef RemarkName,
                                const Function &Fn,
                                const DiagnosticLocation &Loc,
                                const Value *CodeRegion = nullptr)
       : DiagnosticInfoOptimizationBase(Kind, Severity, PassName, RemarkName, Fn,
                                        Loc),
private:
   const Value *CodeRegion;
 };
 
 /// Diagnostic information for applied optimization remarks.
 class OptimizationRemark : public DiagnosticInfoIROptimization {
 public:
   /// \p PassName is the name of the pass emitting this diagnostic. If this name
   /// matches the regular expression given in -Rpass=, then the diagnostic will
-  /// be emitted.  \p RemarkName is a textual identifier for the remark.  \p
-  /// Loc is the debug location and \p CodeRegion is the region that the
-  /// optimization operates on (currently on block is supported).
+  /// be emitted. \p RemarkName is a textual identifier for the remark (single-
+  /// word, camel-case). \p Loc is the debug location and \p CodeRegion is the
+  /// region that the optimization operates on (currently only block is
+  /// supported).
   OptimizationRemark(const char *PassName, StringRef RemarkName,
                      const DiagnosticLocation &Loc, const Value *CodeRegion);
 
-  /// Same as above but the debug location and code region is derived from \p
+  /// Same as above, but the debug location and code region are derived from \p
   /// Instr.
   OptimizationRemark(const char *PassName, StringRef RemarkName,
-                     Instruction *Inst);
+                     const Instruction *Inst);
+
+  /// Same as above, but the debug location and code region are derived from \p
+  /// Func.
+  OptimizationRemark(const char *PassName, StringRef RemarkName,
+                     const Function *Func);
 
   static bool classof(const DiagnosticInfo *DI) {
     return DI->getKind() == DK_OptimizationRemark;
   }
 
   static bool isEnabled(StringRef PassName);
 
   /// \see DiagnosticInfoOptimizationBase::isEnabled.
friend void emitOptimizationRemark(LLVMContext &Ctx, const char *PassName,
                                      const Twine &Msg);
 };
 
 /// Diagnostic information for missed-optimization remarks.
 class OptimizationRemarkMissed : public DiagnosticInfoIROptimization {
 public:
   /// \p PassName is the name of the pass emitting this diagnostic. If this name
   /// matches the regular expression given in -Rpass-missed=, then the
-  /// diagnostic will be emitted.  \p RemarkName is a textual identifier for the
-  /// remark.  \p Loc is the debug location and \p CodeRegion is the region
-  /// that the optimization operates on (currently on block is supported).
+  /// diagnostic will be emitted. \p RemarkName is a textual identifier for the
+  /// remark (single-word, camel-case). \p Loc is the debug location and \p
+  /// CodeRegion is the region that the optimization operates on (currently only
+  /// block is supported).
   OptimizationRemarkMissed(const char *PassName, StringRef RemarkName,
                            const DiagnosticLocation &Loc,
                            const Value *CodeRegion);
 
   /// \brief Same as above but \p Inst is used to derive code region and debug
   /// location.
   OptimizationRemarkMissed(const char *PassName, StringRef RemarkName,
                            const Instruction *Inst);
friend void emitOptimizationRemarkMissed(LLVMContext &Ctx,
                                            const Twine &Msg);
 };
 
 /// Diagnostic information for optimization analysis remarks.
 class OptimizationRemarkAnalysis : public DiagnosticInfoIROptimization {
 public:
   /// \p PassName is the name of the pass emitting this diagnostic. If this name
   /// matches the regular expression given in -Rpass-analysis=, then the
-  /// diagnostic will be emitted.  \p RemarkName is a textual identifier for the
-  /// remark.  \p Loc is the debug location and \p CodeRegion is the region
-  /// that the optimization operates on (currently on block is supported).
+  /// diagnostic will be emitted. \p RemarkName is a textual identifier for the
+  /// remark (single-word, camel-case). \p Loc is the debug location and \p
+  /// CodeRegion is the region that the optimization operates on (currently only
+  /// block is supported).
   OptimizationRemarkAnalysis(const char *PassName, StringRef RemarkName,
                              const DiagnosticLocation &Loc,
                              const Value *CodeRegion);
 
   /// \brief This is ctor variant allows a pass to build an optimization remark
   /// from an existing remark.
   ///
   /// This is useful when a transformation pass (e.g LV) wants to emit a remark

 };
 
 /// Diagnostic information for optimization analysis remarks related to
 /// floating-point non-commutativity.
 class OptimizationRemarkAnalysisFPCommute : public OptimizationRemarkAnalysis {
 public:
   /// \p PassName is the name of the pass emitting this diagnostic. If this name
   /// matches the regular expression given in -Rpass-analysis=, then the
-  /// diagnostic will be emitted.  \p RemarkName is a textual identifier for the
-  /// remark.  \p Loc is the debug location and \p CodeRegion is the region
-  /// that the optimization operates on (currently on block is supported). The
-  /// front-end will append its own message related to options that address
-  /// floating-point non-commutativity.
+  /// diagnostic will be emitted. \p RemarkName is a textual identifier for the
+  /// remark (single-word, camel-case). \p Loc is the debug location and \p
+  /// CodeRegion is the region that the optimization operates on (currently only
+  /// block is supported). The front-end will append its own message related to
+  /// options that address floating-point non-commutativity.
   OptimizationRemarkAnalysisFPCommute(const char *PassName,
                                       StringRef RemarkName,
                                       const DiagnosticLocation &Loc,
                                       const Value *CodeRegion)
       : OptimizationRemarkAnalysis(DK_OptimizationRemarkAnalysisFPCommute,
                                    PassName, RemarkName, Loc, CodeRegion) {}
 
   static bool classof(const DiagnosticInfo *DI) {

 };
 
 /// Diagnostic information for optimization analysis remarks related to
 /// pointer aliasing.
 class OptimizationRemarkAnalysisAliasing : public OptimizationRemarkAnalysis {
 public:
   /// \p PassName is the name of the pass emitting this diagnostic. If this name
   /// matches the regular expression given in -Rpass-analysis=, then the
-  /// diagnostic will be emitted.  \p RemarkName is a textual identifier for the
-  /// remark.  \p Loc is the debug location and \p CodeRegion is the region
-  /// that the optimization operates on (currently on block is supported). The
-  /// front-end will append its own message related to options that address
-  /// pointer aliasing legality.
+  /// diagnostic will be emitted. \p RemarkName is a textual identifier for the
+  /// remark (single-word, camel-case). \p Loc is the debug location and \p
+  /// CodeRegion is the region that the optimization operates on (currently only
+  /// block is supported). The front-end will append its own message related to
+  /// options that address pointer aliasing legality.
   OptimizationRemarkAnalysisAliasing(const char *PassName, StringRef RemarkName,
                                      const DiagnosticLocation &Loc,
                                      const Value *CodeRegion)
       : OptimizationRemarkAnalysis(DK_OptimizationRemarkAnalysisAliasing,
                                    PassName, RemarkName, Loc, CodeRegion) {}
 
   static bool classof(const DiagnosticInfo *DI) {
     return DI->getKind() == DK_OptimizationRemarkAnalysisAliasing;

llvm/trunk/lib/CodeGen/StackProtector.cpp

 // changed, then there was some sort of violation and the program aborts.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/EHPersonalities.h"
+#include "llvm/Analysis/OptimizationDiagnosticInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/StackProtector.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DebugLoc.h"

 STATISTIC(NumAddrTaken, "Number of local variables that have their address"
                         " taken.");
 
 static cl::opt<bool> EnableSelectionDAGSP("enable-selectiondag-sp",
                                           cl::init(true), cl::Hidden);
 
 char StackProtector::ID = 0;
 INITIALIZE_TM_PASS(StackProtector, "stack-protector", "Insert stack protectors",
                    false, true)
 
 FunctionPass *llvm::createStackProtectorPass(const TargetMachine *TM) {
   return new StackProtector(TM);
 }
 
 StackProtector::SSPLayoutKind
 StackProtector::getSSPLayout(const AllocaInst *AI) const {
   return AI ? Layout.lookup(AI) : SSPLK_None;
for (const Instruction &I : BB)
         if (CI->getCalledFunction() ==
             Intrinsic::getDeclaration(F->getParent(),
                                       Intrinsic::stackprotector))
           HasPrologue = true;
 
   if (F->hasFnAttribute(Attribute::SafeStack))
     return false;
 
+  // We are constructing the OptimizationRemarkEmitter on the fly rather than
+  // using the analysis pass to avoid building DominatorTree and LoopInfo which
+  // are not available this late in the IR pipeline.
+  OptimizationRemarkEmitter ORE(F);
+  auto ReasonStub =
+      Twine("Stack protection applied to function " + F->getName() + " due to ")
+          .str();
+
   if (F->hasFnAttribute(Attribute::StackProtectReq)) {
+    ORE.emit(OptimizationRemark(DEBUG_TYPE, "StackProtectorRequested", F)
+             << ReasonStub << "a function attribute or command-line switch");
     NeedsProtector = true;
     Strong = true; // Use the same heuristic as strong to determine SSPLayout
   } else if (F->hasFnAttribute(Attribute::StackProtectStrong))
     Strong = true;
   else if (HasPrologue)
     NeedsProtector = true;
   else if (!F->hasFnAttribute(Attribute::StackProtect))
     return false;
 
   for (const BasicBlock &BB : *F) {
     for (const Instruction &I : BB) {
       if (const AllocaInst *AI = dyn_cast<AllocaInst>(&I)) {
         if (AI->isArrayAllocation()) {
+          OptimizationRemark Remark(DEBUG_TYPE, "StackProtectorAllocaOrArray",
+                                    &I);
+          Remark << ReasonStub
+                 << "a call to alloca or use of a variable length array";
           if (const auto *CI = dyn_cast<ConstantInt>(AI->getArraySize())) {
             if (CI->getLimitedValue(SSPBufferSize) >= SSPBufferSize) {
               // A call to alloca with size >= SSPBufferSize requires
               // stack protectors.
               Layout.insert(std::make_pair(AI, SSPLK_LargeArray));
+              ORE.emit(Remark);
               NeedsProtector = true;
             } else if (Strong) {
               // Require protectors for all alloca calls in strong mode.
               Layout.insert(std::make_pair(AI, SSPLK_SmallArray));
+              ORE.emit(Remark);
               NeedsProtector = true;
             }
           } else {
             // A call to alloca with a variable size requires protectors.
             Layout.insert(std::make_pair(AI, SSPLK_LargeArray));
+            ORE.emit(Remark);
             NeedsProtector = true;
           }
           continue;
         }
 
         bool IsLarge = false;
         if (ContainsProtectableArray(AI->getAllocatedType(), IsLarge, Strong)) {
           Layout.insert(std::make_pair(AI, IsLarge ? SSPLK_LargeArray
                                                    : SSPLK_SmallArray));
+          ORE.emit(OptimizationRemark(DEBUG_TYPE, "StackProtectorBuffer", &I)
+                   << ReasonStub
+                   << "a stack allocated buffer or struct containing a buffer");
           NeedsProtector = true;
           continue;
         }
 
         if (Strong && HasAddressTaken(AI)) {
           ++NumAddrTaken;
           Layout.insert(std::make_pair(AI, SSPLK_AddrOf));
+          ORE.emit(
+              OptimizationRemark(DEBUG_TYPE, "StackProtectorAddressTaken", &I)
+              << ReasonStub << "the address of a local variable being taken");
           NeedsProtector = true;
         }
       }
     }
   }
 
   return NeedsProtector;
 }

llvm/trunk/lib/IR/DiagnosticInfo.cpp

OptimizationRemark::OptimizationRemark(const char *PassName,
                                        StringRef RemarkName,
                                        const DiagnosticLocation &Loc,
                                        const Value *CodeRegion)
     : DiagnosticInfoIROptimization(
           DK_OptimizationRemark, DS_Remark, PassName, RemarkName,
           *cast<BasicBlock>(CodeRegion)->getParent(), Loc, CodeRegion) {}
 
 OptimizationRemark::OptimizationRemark(const char *PassName,
-                                       StringRef RemarkName, Instruction *Inst)
+                                       StringRef RemarkName,
+                                       const Instruction *Inst)
     : DiagnosticInfoIROptimization(DK_OptimizationRemark, DS_Remark, PassName,
                                    RemarkName, *Inst->getParent()->getParent(),
                                    Inst->getDebugLoc(), Inst->getParent()) {}
 
+// Helper to allow for an assert before attempting to return an invalid
+// reference.
+static const BasicBlock &getFirstFunctionBlock(const Function *Func) {
+  assert(!Func->empty() && "Function does not have a body");
+  return Func->front();
+}
+
+OptimizationRemark::OptimizationRemark(const char *PassName,
+                                       StringRef RemarkName,
+                                       const Function *Func)
+    : DiagnosticInfoIROptimization(DK_OptimizationRemark, DS_Remark, PassName,
+                                   RemarkName, *Func, Func->getSubprogram(),
+                                   &getFirstFunctionBlock(Func)) {}
+
 bool OptimizationRemark::isEnabled(StringRef PassName) {
   return PassRemarksOptLoc.Pattern &&
          PassRemarksOptLoc.Pattern->match(PassName);
 }
 
 OptimizationRemarkMissed::OptimizationRemarkMissed(
     const char *PassName, StringRef RemarkName, const DiagnosticLocation &Loc,
     const Value *CodeRegion)

llvm/trunk/test/CodeGen/X86/stack-protector-remarks.ll

+; RUN: llc %s -pass-remarks=stack-protector -o /dev/null 2>&1 | FileCheck %s
+; CHECK-NOT: nossp
+; CHECK: function attribute_ssp
+; CHECK-SAME: a function attribute or command-line switch
+; CHECK-NOT: alloca_fixed_small_nossp
+; CHECK: function alloca_fixed_small_ssp
+; CHECK-SAME: a call to alloca or use of a variable length array
+; CHECK: function alloca_fixed_large_ssp
+; CHECK-SAME: a call to alloca or use of a variable length array
+; CHECK: function alloca_variable_ssp
+; CHECK-SAME: a call to alloca or use of a variable length array
+; CHECK: function buffer_ssp
+; CHECK-SAME: a stack allocated buffer or struct containing a buffer
+; CHECK: function struct_ssp
+; CHECK-SAME: a stack allocated buffer or struct containing a buffer
+; CHECK: function address_ssp
+; CHECK-SAME: the address of a local variable being taken
+; CHECK: function multiple_ssp
+; CHECK-SAME: a function attribute or command-line switch
+; CHECK: function multiple_ssp
+; CHECK-SAME: a stack allocated buffer or struct containing a buffer
+; CHECK: function multiple_ssp
+; CHECK-SAME: a stack allocated buffer or struct containing a buffer
+; CHECK: function multiple_ssp
+; CHECK-SAME: the address of a local variable being taken
+; CHECK: function multiple_ssp
+; CHECK-SAME: a call to alloca or use of a variable length array
+
+; Check that no remark is emitted when the switch is not specified.
+; RUN: llc %s -o /dev/null 2>&1 | FileCheck %s -check-prefix=NOREMARK -allow-empty
+; NOREMARK-NOT: ssp
+
+define void @nossp() ssp {
+  ret void
+}
+
+define void @attribute_ssp() sspreq {
+  ret void
+}
+
+define void @alloca_fixed_small_nossp() ssp {
+  %1 = alloca i8, i64 2, align 16
+  ret void
+}
+
+define void @alloca_fixed_small_ssp() sspstrong {
+  %1 = alloca i8, i64 2, align 16
+  ret void
+}
+
+define void @alloca_fixed_large_ssp() ssp {
+  %1 = alloca i8, i64 64, align 16
+  ret void
+}
+
+define void @alloca_variable_ssp(i64 %x) ssp {
+  %1 = alloca i8, i64 %x, align 16
+  ret void
+}
+
+define void @buffer_ssp() sspstrong {
+  %x = alloca [64 x i32], align 16
+  ret void
+}
+
+%struct.X = type { [64 x i32] }
+define void @struct_ssp() sspstrong {
+  %x = alloca %struct.X, align 4
+  ret void
+}
+
+define void @address_ssp() sspstrong {
+entry:
+  %x = alloca i32, align 4
+  %y = alloca i32*, align 8
+  store i32 32, i32* %x, align 4
+  store i32* %x, i32** %y, align 8
+  ret void
+}
+
+define void @multiple_ssp() sspreq {
+entry:
+  %x = alloca %struct.X, align 4
+  %y = alloca [64 x i32], align 16
+  %a = alloca i32, align 4
+  %b = alloca i32*, align 8
+  %0 = alloca i8, i64 2, align 16
+  store i32 32, i32* %a, align 4
+  store i32* %a, i32** %b, align 8
+  ret void
+}