[llvm-libtool-darwin] Add support for -D and -U option

[sameerarora101]

Summary

-D allows for using zero for timestamps and UIDs/GIDs.
-U allows for using actual timestamps and UIDs/GIDs.
Depends on D84206.

[jhenderson]

Documentation for this option? Also, you've changed the default behaviour of the tool from being deterministic to being non-deterministic. Are you sure that's what you want?

[sameerarora101]

In D84209#2163675, @jhenderson wrote:

Documentation for this option? Also, you've changed the default behaviour of the tool from being deterministic to being non-deterministic. Are you sure that's what you want?

Would changing the default behavior to being non-deterministic be a problem? This, non-deterministic behavior, is the default behavior of cctools' libtool and so I thought of replicating that here.
I have added the docs now, thanks!

[sameerarora101]

Adding docs

[sameerarora101]

Rebasing

[MaskRay]

In binutils, there is a configure-time option --enable-deterministic-archives. For llvm-objcopy and llvm-ar, we find that deterministic builds tend to make more sense so we simply default to determinism. There probably should be a survey whether cctool libtool users really want the default non-deterministic behavior.

[jhenderson]

In D84209#2164501, @sameerarora101 wrote:

In D84209#2163675, @jhenderson wrote:

Documentation for this option? Also, you've changed the default behaviour of the tool from being deterministic to being non-deterministic. Are you sure that's what you want?

Would changing the default behavior to being non-deterministic be a problem? This, non-deterministic behavior, is the default behavior of cctools' libtool and so I thought of replicating that here.
I have added the docs now, thanks!

I don't know whether it would be a problem or not, but it does make comparing archives harder. That being said, in our downstream llvm-ar, we've had to override the default to use non-deterministic timestamps, because our build system relies on the timestamps being accurate, so that might be evidence that changing it is good. I don't know if it's important to people outside our environment though - there aren't lots of people complaining about the current upstream llvm-ar behaviour, so matching that might make more sense. I don't really have an opinion either way. I'm happy for users of the tool to provide guidance.

[sameerarora101]

Thanks, I'll wait for others to respond too. Maybe @alexshap and @smeenai can comment on the right behavior here?

[sameerarora101]

It seems like cctools' libtool does not have an explicit flag for indicating non-deterministic behavior. Thus, if I set Deterministic to true by default, I would need to implement another flag (something like -U) to specify non-deterministic behavior for llvm-libtool. This would then be a slight deviation from libtool's behavior. What do you guys think?

[jhenderson]

In D84209#2167919, @sameerarora101 wrote:

It seems like cctools' libtool does not have an explicit flag for indicating non-deterministic behavior. Thus, if I set Deterministic to true by default, I would need to implement another flag (something like -U) to specify non-deterministic behavior for llvm-libtool. This would then be a slight deviation from libtool's behavior. What do you guys think?

I think if we have Deterministic set to a different default value to libtool, we should definitely provide a -U option to allow a fallback, in case there are people who need that behaviour.

[sameerarora101]

In D84209#2168725, @jhenderson wrote:

In D84209#2167919, @sameerarora101 wrote:

It seems like cctools' libtool does not have an explicit flag for indicating non-deterministic behavior. Thus, if I set Deterministic to true by default, I would need to implement another flag (something like -U) to specify non-deterministic behavior for llvm-libtool. This would then be a slight deviation from libtool's behavior. What do you guys think?

I think if we have Deterministic set to a different default value to libtool, we should definitely provide a -U option to allow a fallback, in case there are people who need that behaviour.

I see. Would we still need the -D option in that case?

[sameerarora101]

I have updated the diff below to add support for both -D and -U options. Although, the -D flag is not necessary, we perhaps need it to align the behavior with cctools' libtool? I can update it by removing the D flag if people prefer that? Thanks.

[sameerarora101]

Changing default behavior to Deterministic and adding -U flag

[sameerarora101]

Updating the docs for -U flag

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/deterministic-library.test

1–2

Change "timestamps/etc." to simply "timestamps". You're next paragraph explains the bit about other fields, so the "etc" is somewhat misleading/confusing.

Also, combine these into one sentence and add a couple of missing "the"s: "This test checks that timestamps are set to 0 by default or when the -D option is specified, and that they are preserved when the -U option is specified."

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

25

I'm not particularly a fan of adding a global variable for this. Are there going to be other configuration options? If so, I'd recommend creating a Config class/struct, that can be created and passed around instead.

(Also, missing trailing full stop in comment)

182

This function does more than just check the options - it also sets behaviour, based on options that are parsed. I think you need to rename the function to clarify. If you add a Config class, perhaps it could become: static Expected<Config> parseCommandline() instead, and also do the cl::ParseCommandLineOptions call. What do you think?

[sameerarora101]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

25

For now, there aren't any other configuration options that vary depending on the flags passed in on the command line. However, I can still wrap this in a Config class/struct just in case one is needed in the future? What do you think?

182

Yup, I agree on the name and cl::ParseCommandLineOptions call, I have changed it accordingly now. (I can change the signature too if we decide on adding the Config class - please take a look at my comment above for that).

[sameerarora101]

checkCommandLineOptions -> parseCommandLine

[sameerarora101]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

25

By "For now" I am referring to the period until we add full support for creating static libraries. ( I am not completely sure yet about the dynamic case)

[jhenderson]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

25

I'm talking about in the future. I'm guessing there will be other configuration options added in due course, but you have a better idea of this than me. If there are, I'd recommend my suggested approach.

[sameerarora101]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

25

Added the config, thanks.

[sameerarora101]

Add Config struct.

[jhenderson]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

61

How about Deterministic = true (or false, whichever is the preferred default)?

112

Config is likely to grow, so it probably is best to pass it as a const &, here and everywhere else.

189–190

Consider this instead, combined with the default initialiser:

else if (NonDeterminisitcOption)
  C.Deterministic = false;

[sameerarora101]

Config C -> const Config &C

[jhenderson]

LGTM, but as always, please get somebody else with relevant experience of the existing tool to sign off.

[smeenai]

LGTM

[sameerarora101]

Rebase.

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

 .. option:: -color
 
   Use colors in output.
 
 .. option:: -version
 
   Display the version of this program.
 
+.. option:: -D
+
+ Use zero for timestamps and UIDs/GIDs. This is set by default.
+
+.. option:: -U
+
+ Use actual timestamps and UIDs/GIDs.
+
 .. option:: -o <filename>
 
   Specify the output file name. Must be specified exactly once.
 
 .. option:: -static
 
  Produces a static library from the input files.
 

llvm/test/tools/llvm-libtool-darwin/deterministic-library.test

+## This test checks that timestamps/etc. are set to 0 by default or when -D
+## option is specified. Timestamps/etc. are preserved when -U option is
+## specified.
+## We only test timestamps as a proxy for full deterministic writing; i.e. we
+## assume UID/GIDs are preserved if timestamps are preserved.
+
+# RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
+# RUN: touch -t 199505050555.55 %t-input1.o
+
+## Test values are set to 0 (by default):
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o
+# RUN: env TZ=GMT llvm-ar tv %t.lib | FileCheck %s --check-prefix=CHECK-DETERMINISTIC
+
+## Test values are set to 0 (with -D):
+# RUN: llvm-libtool-darwin -static -o %t.lib -D %t-input1.o
+# RUN: env TZ=GMT llvm-ar tv %t.lib | FileCheck %s --check-prefix=CHECK-DETERMINISTIC
+
+# CHECK-DETERMINISTIC: {{[[:space:]]1970[[:space:]]}}
+
+## Test values are preserved (with -U):
+# RUN: llvm-libtool-darwin -static -o %t.lib -U %t-input1.o
+# RUN: env TZ=GMT llvm-ar tv %t.lib | FileCheck %s --check-prefix=CHECK-NONDETERMINISTIC
+
+# CHECK-NONDETERMINISTIC:  {{[[:space:]]1995[[:space:]]}}
+
+## D Flag specified more than once:
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t-input1.o -D -D 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=CHECK-ERROR-D
+
+# CHECK-ERROR-D: for the -D option: may only occur zero or one times!
+
+## U Flag specified more than once:
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t-input1.o -U -U 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=CHECK-ERROR-U
+
+# CHECK-ERROR-U: for the -U option: may only occur zero or one times!
+
+## Both D and U flags specified:
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t-input1.o -D -U 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=CHECK-ERROR-BOTH
+
+# CHECK-ERROR-BOTH: error: cannot specify both -D and -U flags

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/LineIterator.h"
 #include "llvm/Support/WithColor.h"
 
 using namespace llvm;
 using namespace llvm::object;
 
+static bool Deterministic; // Updated by 'D' and 'U' modifiers
+
 cl::OptionCategory LibtoolCategory("llvm-libtool-darwin Options");
 
 static cl::opt<std::string> OutputFile("o", cl::desc("Specify output filename"),
                                        cl::value_desc("filename"), cl::Required,
                                        cl::cat(LibtoolCategory));
 
 static cl::list<std::string> InputFiles(cl::Positional,
                                         cl::desc("<input files>"),
                                         cl::ZeroOrMore,
                                         cl::cat(LibtoolCategory));
 
 enum class Operation { Static };
 
 static cl::opt<Operation> LibraryOperation(
     cl::desc("Library Type: "),
     cl::values(
         clEnumValN(Operation::Static, "static",
                    "Produce a statically linked library from the input files")),
     cl::Required, cl::cat(LibtoolCategory));
 
+static cl::opt<bool> DeterministicOption(
+    "D", cl::desc("Use zero for timestamps and UIDs/GIDs (Default)"),
+    cl::init(false), cl::cat(LibtoolCategory));
+
+static cl::opt<bool>
+    NonDeterministicOption("U", cl::desc("Use actual timestamps and UIDs/GIDs"),
+                           cl::init(false), cl::cat(LibtoolCategory));
+
 static cl::opt<std::string>
     FileList("filelist",
              cl::desc("Pass in file containing a list of filenames"),
              cl::value_desc("listfile[,dirname]"), cl::cat(LibtoolCategory));
 
 static Error processFileList() {
   StringRef FileName, DirName;
   std::tie(FileName, DirName) = StringRef(FileList).rsplit(",");
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> FileOrErr =
       MemoryBuffer::getFileOrSTDIN(FileName, /*FileSize=*/-1,
                                    /*RequiresNullTerminator=*/false);
   if (std::error_code EC = FileOrErr.getError())
     return createFileError(FileName, errorCodeToError(EC));
   const MemoryBuffer &Ref = *FileOrErr.get();
if (!isa<object::MachOObjectFile>(**ObjOrErr))
     return createStringError(std::errc::invalid_argument,
                              "'%s': format not supported",
                              Member.MemberName.data());
 
   return Error::success();
 }
 
 static Error addChildMember(std::vector<NewArchiveMember> &Members,
                             const object::Archive::Child &M) {
   Expected<NewArchiveMember> NMOrErr =
-      NewArchiveMember::getOldMember(M, /*Deterministic=*/true);
+      NewArchiveMember::getOldMember(M, Deterministic);
   if (!NMOrErr)
     return NMOrErr.takeError();
 
   // Verify that Member is a Mach-O object file.
   if (Error E = verifyMachOObject(*NMOrErr))
     return E;
 
   Members.push_back(std::move(*NMOrErr));
   return Error::success();
 }
 
 static Error
 addMember(std::vector<NewArchiveMember> &Members, StringRef FileName,
           std::vector<std::unique_ptr<MemoryBuffer>> &ArchiveBuffers) {
   Expected<NewArchiveMember> NMOrErr =
-      NewArchiveMember::getFile(FileName, /*Deterministic=*/true);
+      NewArchiveMember::getFile(FileName, Deterministic);
   if (!NMOrErr)
     return createFileError(FileName, NMOrErr.takeError());
 
   // For regular archives, use the basename of the object path for the member
   // name.
   NMOrErr->MemberName = sys::path::filename(NMOrErr->MemberName);
 
   // Flatten archives.
static Error createStaticLibrary() {
   std::vector<NewArchiveMember> NewMembers;
   std::vector<std::unique_ptr<MemoryBuffer>> ArchiveBuffers;
   for (StringRef Member : InputFiles)
     if (Error E = addMember(NewMembers, Member, ArchiveBuffers))
       return E;
 
   if (Error E = writeArchive(OutputFile, NewMembers,
                              /*WriteSymtab=*/true,
-                             /*Kind=*/object::Archive::K_DARWIN,
-                             /*Deterministic=*/true,
+                             /*Kind=*/object::Archive::K_DARWIN, Deterministic,
                              /*Thin=*/false))
     return E;
   return Error::success();
 }
 
+static Error checkCommandLineOptions() {
+  if (DeterministicOption && NonDeterministicOption)
+    return createStringError(std::errc::invalid_argument,
+                             "cannot specify both -D and -U flags");
+  else
+    Deterministic = NonDeterministicOption ? false : true;
+
+  if (!FileList.empty())
+    if (Error E = processFileList())
+      return E;
+
+  if (InputFiles.empty())
+    return createStringError(std::errc::invalid_argument,
+                             "no input files specified");
+
+  return Error::success();
+}
+
 int main(int Argc, char **Argv) {
   InitLLVM X(Argc, Argv);
   cl::HideUnrelatedOptions({&LibtoolCategory, &ColorCategory});
   cl::ParseCommandLineOptions(Argc, Argv, "llvm-libtool-darwin\n");
-  if (!FileList.empty()) {
-    if (Error E = processFileList()) {
-      WithColor::defaultErrorHandler(std::move(E));
-      return EXIT_FAILURE;
-    }
-  }
-
-  if (InputFiles.empty()) {
-    Error E = createStringError(std::errc::invalid_argument,
-                                "no input files specified");
+  if (Error E = checkCommandLineOptions()) {
     WithColor::defaultErrorHandler(std::move(E));
     return EXIT_FAILURE;
   }
 
   switch (LibraryOperation) {
   case Operation::Static:
     if (Error E = createStaticLibrary()) {
       WithColor::defaultErrorHandler(std::move(E));
       return EXIT_FAILURE;
     }
     break;
   }
 }