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

[Stephen Crane via Phabricator via llvm-commits]

rinon added inline comments.


================
Comment at: docs/LangRef.rst:1557
+    This attribute indicates that the function is compatible with pagerando and
+    will be located in a section which the dynamic loader can place at an
+    independent offset from any other section in the binary.
----------------
peter.smith wrote:
> Is section the right word to use here? The compiler will place the function in a section, the linker will place that in an output section that will presumably be described, at least in ELF terms as a segment.
I'm open to either section or segment. I wasn't sure which one made more sense, since the compiler only cares about sections, and in this case the section and segment need to be the same.


================
Comment at: lib/Transforms/IPO/PagerandoWrappers.cpp:101
+//
+// TODO: Support COMDAT
+static bool skipFunction(const Function &F) {
----------------
peter.smith wrote:
> Are the issues surrounding comdat that at code-generation time that we can't guarantee at link time which comdat group is selected and it may not be the one with wrappers? If I'm write it would be useful to know, although not necessarily in this review, what ideas you have to resolve this?  
Not quite. We don't really care if non-pagerando comdat groups are selected since this implementation should only refer to wrapped, external functions and cannot call functions in pagerando bins directly. To make sure that the wrappers are selected if their corresponding implementations are selected, wrappers need to go in the same comdat group as their implementation function, which is easy. We would need to treat all comdat functions as externally visible and potentially replaced to handle the case where the external implementation is selected.

However, if we mix comdat groups and pagerando bins, it turns out that we cannot control the layout of the resulting section precisely. The linker may (and in my testing did) reorder resolved comdat sections when merging them into our named output section (pagerando bin), which ruins the section offsets we computed during code generation for referencing binned functions.

Looking closer at the gold plugin interface specifically, the gold documentation seems to promise that all comdat groups will be resolved to the IR input file version, and no comdat symbols should be in the LTO output file (see COMDAT Sections in https://gcc.gnu.org/wiki/whopr/driver). So, as long as we're doing full LTO with gold, we should be able to resolve comdat before assigning to bins and the whole issue goes away. However, as far as I can tell, LLVM doesn't actually do that right now. I built some simple C++ with full (gold) LTO, saved temps, and the LTO output still included COMDAT sections.

Basically, I think we either need cooperation from the linker in how comdat sections are merged together, or we need to resolve and remove comdat during LTO. Alternatively, if we add linker support for new relocations (section->symbol offset and POT entry index), I think this issue goes away, but that is more involved and requires upstreaming to multiple projects.


https://reviews.llvm.org/D37581