[PATCH] D15943: Add support for headerpad_max_install_names cmdline option

[Lang Hames via llvm-commits]

lhames added inline comments.

================
Comment at: lib/ReaderWriter/MachO/MachONormalizedFileBinaryWriter.cpp:462
@@ +461,3 @@
+  // pad the header with enough space for each path to grow to MAXPATHLEN.
+  // FIXME!: We need to get the max path in a reliable way.
+  const unsigned maxpathlen = 1024;
----------------
We may want to add a command line option (with a default of 1024) for this eventually, but we can deal with that separately.

================
Comment at: lib/ReaderWriter/MachO/MachONormalizedFileBinaryWriter.cpp:466
@@ +465,3 @@
+  for (const DependentDylib &dep : _file.dependentDylibs) {
+    size_t size = pointerAlign(dep.path.size()+1);
+    if (size < maxpathlen)
----------------
Is this pointerAlign necessary?

I don't think it matters much either way, but consider a 1 character path name on a 64-bit platform: With this pointerAlign that original path length will be "aligned" out to 8, then padded with 1016 bytes. Then, at write-out time the 1017 characters (1 char path plus padding) will be aligned back up to 1024.

Alternatively, I think we can ditch this and just add 1023 bytes of padding up front, for 1024 bytes total, at which point the second pointerAlign call is a no-op.

That said I haven't actually *tried* this, and you might have. If you did and it exploded, feel free to disregard this suggestion. ;)


================
Comment at: lib/ReaderWriter/MachO/MachONormalizedFileBinaryWriter.cpp:473
@@ +472,3 @@
+  if (_file.fileType == llvm::MachO::MH_DYLIB) {
+    size_t size = pointerAlign(_file.installName.size() + 1);
+    if (size < maxpathlen)
----------------
Likewise for this pointerAlign.


http://reviews.llvm.org/D15943