[PATCH] D60242: Add IR support, ELF section and user documentation for partitioning feature.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've put some suggestions for the documentation. Feel free to ignore them if you prefer the original. I can't see anything wrong with the LLVM changes at a cursory look.



================
Comment at: lld/docs/Partitions.rst:10
+
+LLD's partitioning feature allows a program to be split into multiple pieces,
+or partitions. A partitioned program consists of a main partition together
----------------
Program is an interesting choice of word here. From the initial RFC I think this is largely aimed at executables. I don't think that it would be impossible to make this work with shared-libraries, however I'd expect the constraints would make it difficult to use effectively. I'm thinking of an executable calling a function in the shared library that happens to be in a partition not loaded.

Maybe replace program with executable if that is the only option.


================
Comment at: lld/docs/Partitions.rst:16
+share a virtual address space, and each loadable partition is assigned a
+fixed load address relative to the load address of the main partition. This
+allows the loadable partitions to refer to code and data in the main partition
----------------
I tend to think of an address as an absolute address, I think "fixed load address relative to the main partition" is meant to be an offset". A suggestion:
"and each loadable partition is assigned a fixed offset from the main partition."


================
Comment at: lld/docs/Partitions.rst:19
+directly by address without the binary size and performance overhead of PLTs,
+GOTs or symbol table entries.
+
----------------
I think that you could remove the "by address" if offset were used earlier.


================
Comment at: lld/docs/Partitions.rst:25
+A program that uses the partitioning feature must decide which symbols are
+going to be used as "entry points" to each partition. An entry point could,
+for example, be the equivalent of the partition's ``main`` function, or there
----------------
Just a suggestion, to be used as the "entry points" for each partition.


================
Comment at: lld/docs/Partitions.rst:65
+
+  $ clang -ffunction-sections -fdata-sections -c main.c
+  $ clang -ffunction-sections -fdata-sections -fsymbol-partition=libfeature.so -c feature.c
----------------
If I've understood it correctly you have to know ahead of time which source files are in which partition, the linker won't automatically create these for you. May be worth putting a shell comment before the main.c and before the feature.c emphasising that these are likely to be two separate build steps. I'm thinking that a common tool flow would be to build each partition into a separate static archive.


================
Comment at: lld/docs/Partitions.rst:67
+  $ clang -ffunction-sections -fdata-sections -fsymbol-partition=libfeature.so -c feature.c
+  $ clang main.o feature.o -fuse-ld=lld -shared -o libcombined.so -Wl,-soname,libmain.so -Wl,--gc-sections
+  $ llvm-objcopy libcombined.so libmain.so --extract-main-partition
----------------
Just a thought, is --gc-sections compulsory? I don't think that it is but if you are hanging this off the --gc-sections it might be easier to automatically enable it if LLD encounters a partition. Or maybe have a --combined-object option that forces --gc-sections, and if not present drops all the partition information and produces a regular output?


================
Comment at: lld/docs/Partitions.rst:70
+  $ llvm-objcopy libcombined.so libfeature.so --extract-partition=libfeature.so
+
+In order to allow a program to discover the names of its loadable partitions
----------------
This is an llvm-objcopy thought, perhaps a --extract-partition=all option would be useful. In this case the name of the partition would be the name of the filename as in your example above.


================
Comment at: lld/docs/Partitions.rst:83
+
+The ``name_relptr`` field is a relative pointer to a null-terminated string
+containing the soname of the partition, the ``addr_relptr`` field is a
----------------
Could offset be used instead of relptr?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60242/new/

https://reviews.llvm.org/D60242