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

[Adam Nemet via Phabricator via llvm-commits]

anemet added subscribers: ab, bogner.
anemet requested changes to this revision.
anemet added a comment.
This revision now requires changes to proceed.

In https://reviews.llvm.org/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().



================
Comment at: lib/CodeGen/StackProtector.cpp:239
+      "stack protection applied to function " + F->getName() + " due to ";
+  StringRef RemarkName = "stack-protector-reason";
+
----------------
Convention for this is camel-case: StackProtectorReason.  I think it's documented in the comments. If not feel free to add it.


================
Comment at: lib/CodeGen/StackProtector.cpp:242
   if (F->hasFnAttribute(Attribute::StackProtectReq)) {
+    if (!F->getBasicBlockList().empty())
+      ORE.emit(
----------------
We don't run function passes on declarations which I think is the only reason that this can be empty (i.e. F.isDeclaration()).


================
Comment at: lib/CodeGen/StackProtector.cpp:245
+          OptimizationRemark(DEBUG_TYPE, RemarkName, F->getSubprogram(), &F->front())
+          << (ReasonStub + "a function attribute or command-line switch").str());
     NeedsProtector = true;
----------------
You can just << these one by one:

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


================
Comment at: lib/CodeGen/StackProtector.cpp:259-261
+          OptimizationRemark Remark(DEBUG_TYPE, RemarkName, &I);
+          Twine Reason =
+              ReasonStub + "a call to alloca or use of a variable length array";
----------------
Since you create the remark unconditionally you may as well pipe the the reason into it.


https://reviews.llvm.org/D29023