[PATCH] D15943: Add support for headerpad_max_install_names cmdline option

[Pete Cooper via llvm-commits]

pete 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;
----------------
lhames wrote:
> We may want to add a command line option (with a default of 1024) for this eventually, but we can deal with that separately.
Sounds good.  Will do that in a follow up if we need it.

================
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)
----------------
lhames wrote:
> 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. ;)
> 
You're right.  Its not needed. 

I think I got this by copying the cmdsize from the macho writer which is aligning the string up to pointer align so that the load command is the correct length.  But thats not needed here when we are just padding them out.  I'll remove it and the other pointerAlign call


http://reviews.llvm.org/D15943