[PATCH] D49366: Add support for /FUNCTIONPADMIN command-line option

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

Probably the best way to write the test would be to have it create an input file with a `.text` section containing the entry point and a non-code section and relocations pointing from the `.text` section to itself as well as to the non-code section, and use `llvm-objdump` to test that (1) the `.text` section has an appropriate amount of padding behind it in the output file, (2) the non-code section does not receive padding and (3) the relocation points to the address of the section (i.e. not the address of the padding). You can see an example of this sort of test in `lld/test/COFF/string-tail-merge.s`. By using `llvm-mc` to assemble the test with different target triples it should be possible to write it in such a way that you don't need a separate test for each target.



================
Comment at: COFF/Config.h:177
+  // Used for /functionpadmin.
+  uint32_t Padding = 0;
+
----------------
Nit: we tend to name configuration fields after the flag that they're derived from. So this one should be called `FunctionPadMin` I think. With that you don't need the comment either because it becomes redundant with the name of the field, and you can group it with the other `uint32_t` fields below.


================
Comment at: COFF/DriverUtils.cpp:271-273
+    } else {
+      error("/functionpadmin: invalid argument for this machine: " + Arg);
+    }
----------------
stefan_reinalter wrote:
> pcc wrote:
> > stefan_reinalter wrote:
> > > zturner wrote:
> > > > What about ARM64?  The documentation doesn't say what the default value is here, but it should be easy to figure out.
> > > Understood, I will try figuring this out.
> > And ARM32.
> When building for ARM using MSVC/link.exe, using /FUNCTIONPADMIN yields:
> LINK : fatal error LNK1146: no argument specified with option '/FUNCTIONPADMIN'
> 
> Seems like there is no default for these platforms.
I wouldn't add a padding amount for IA64 here since the rest of the linker doesn't support it so it is basically dead code.


================
Comment at: COFF/Writer.cpp:760
+      // Apply padding to SectionChunks only.
+      if ((Padding != 0) && dyn_cast<SectionChunk>(C)) {
+        RawSize += Padding;
----------------
It looks like you don't need to protect this code with an if statement because there's no harm in padding compiler-generated sections, and link.exe does it too (try running `link.exe /functionpadmin:4096 /entry:ExitProcess /subsystem:console /out:exit.exe kernel32.lib` and disassembling `exit.exe`).


================
Comment at: COFF/Writer.cpp:761
+      if ((Padding != 0) && dyn_cast<SectionChunk>(C)) {
+        RawSize += Padding;
+        VirtualSize += Padding;
----------------
Taking a closer look at the code, it looks like `RawSize` is updated automatically at the end of the loop, so you don't need to update it here.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D49366