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

[Stephen Crane via Phabricator via llvm-commits]

rinon added a comment.

Thanks for the comments. I'll fix the nits right away.



================
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
----------------
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.


================
Comment at: lib/Transforms/IPO/PagerandoWrappers.cpp:129
+
+  F->setSection("");
+  F->addFnAttr(Attribute::PagerandoBinned);
----------------
vlad.tsyrklevich wrote:
> What's the purpose of setting the Function section to the default here?
We want to assign these functions to unique section "bins" later in pagerando. Because we want to control the section that pagerando'd functions end up in to make sure it can be randomly placed, we don't respect the assigned section for the wrapped function. However, we do preserve any section assignment for the wrapper function that takes the given function's place (since we copy attributes (line 156).


https://reviews.llvm.org/D37581