[PATCH] D37581: Implement pagerando wrapper functions to initialize POT register

[Stephen Crane via Phabricator via llvm-commits]

rinon marked 3 inline comments as done.
rinon added a comment.

Thanks for taking a look at this, Nick. I think this revision should address all comments so far.



================
Comment at: include/llvm/IR/Attributes.td:182-186
+/// Function is a Pagerando wrapper.
+def PagerandoWrapper : EnumAttr<"pagerando_wrapper">;
+
+/// Function is placed into a Pagerando bin.
+def PagerandoBinned : EnumAttr<"pagerando_binned">;
----------------
nlewycky wrote:
> I don't see a matching LangRef change.
> 
> I don't see a matching Verifier change. These are only applicable to functions (as opposed to return values or arguments)? Are these two attributes mutually exclusive?
I added a LangRef description.

After giving attributes a second look, I removed the PagerandoWrapper attribute as we don't need it at this point. The Pagerando attribute (formerly PagerandoBinned) is checked in the Verifier under isFuncOnlyAttr. I don't think we need to verify any other properties of the attribute, now that PagerandoWrapper is gone.


================
Comment at: lib/Transforms/IPO/PagerandoWrappers.cpp:77
+      || F.hasComdat()  // TODO: Support COMDAT
+      || isa<UnreachableInst>(F.getEntryBlock().getTerminator());
+      // Above condition is different from F.doesNotReturn(), which we do not
----------------
nlewycky wrote:
> rinon wrote:
> > vlad.tsyrklevich wrote:
> > > Could you explain this condition?
> > The last condition handles functions that consist of a single unreachable instruction. We don't every want to create a wrapper for these functions, since they shouldn't be emitted. I'll add a better comment here.
> That's not quite what this tests for. This would return true for a function that does work but ends in unreachable, say printing a fatal error message and calling abort. If you want to check for the 1-instruction function with unreachable, use isa<UnreachableInst>(&F.getEntryBlock().front()).
Good point. I intended to catch functions like the following (generated for abstract, non-base destructors):
```
  call void @llvm.dbg.value...
  tail call void @llvm.trap()
  unreachable
```
Randomizing the page address of such functions is pointless, so we can save code size by not enabling pagerando for functions like these and thus not need wrappers. I've now made this optimization more explicit. If you think we should just leave this out for the sake of simplicity, I'm fine with that as well.


https://reviews.llvm.org/D37581