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

[Stefan Reinalter via Phabricator via llvm-commits]

stefan_reinalter added inline comments.


================
Comment at: COFF/DriverUtils.cpp:271-273
+    } else {
+      error("/functionpadmin: invalid argument for this machine: " + Arg);
+    }
----------------
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.


================
Comment at: COFF/PDB.cpp:118-122
+  Expected<const CVIndexMap &> mergeDebugT(ObjFile *File,
+                                           CVIndexMap &ObjectIndexMap);
 
-  Expected<const CVIndexMap&> maybeMergeTypeServerPDB(ObjFile *File,
-                                                      TypeServer2Record &TS);
+  Expected<const CVIndexMap &> maybeMergeTypeServerPDB(ObjFile *File,
+                                                       TypeServer2Record &TS);
----------------
zturner wrote:
> It looks like you may have run clang-format against the entire source files, rather than against only the lines that you changed.  Would it be possible to revert these changes and only clang-format changed lines?  There's a script called git-clang-format in the clang source tree.  Usually what we do is add this location to `PATH`, and then run `git clang-format`  That will only format touched lines.
Yes, I ran clang-format against all touched files. I thought this was the intention to make everything use the same format, but can revert the changes of course.


================
Comment at: COFF/PDB.cpp:790
+    // without any padding to mimic the behaviour of link.exe.
+    SC.Size = SecChunk->getRawSize();
+
----------------
zturner wrote:
> That is quite odd.  Wouldn't this mean that the PDB has incorrect info?  I wonder how this could possibly be correct.  So either it's a bug in link.exe or there's something I don't understand about what PDB conumers use this information for.
I don't think this means that the PDB has incorrect info, but rather that the padding bytes are not accounted for anywhere.
E.g. take a function SimpleFunction at RVA 0x3190:

The public symbol stored will hold its address:
PublicSymbol: [00003190][0001:00002190] ?SimpleFunction@@YAXXZ(void __cdecl SimpleFunction(void))

The function stored inside the compiland will hold the following info:
Function       : static, [00003190][0001:00002190], len = 00000020, void __cdecl SimpleFunction(void)
FuncDebugStart :   static, [00003194][0001:00002194]
FuncDebugEnd   :   static, [000031A7][0001:000021A7]

This stores the address, the length, and end of the prologue as well as start of the epilogue. None of these are concerned about padding bytes. The contribution also shows 0x20 bytes being contributed, which does *not* include padding bytes:
00003190  0001:00002190  00000020  C:\SomePath\SimpleFunction.obj

The padding bytes between functions are 'unaccounted' for, so to speak.
I think this is by design, otherwise you'd get different info from the compiland/function stream and the contribution stream.

Note that padding bytes are inserted in front of functions (=between section chunks), but function entry points/RVAs still point at the first code byte, not any padding.



Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D49366