[llvm-objcopy] Support --{enable,disable}-deterministic-archives

[rupprecht]

Summary

ar and objcopy/strip all support configuring whether archives are written deterministically (timestamps/UIDs/GIDs/etc zero'd). This has been ported to llvm-ar (the U/D modifiers) but not yet to llvm-objcopy/strip.

[ruiu]

I feel it should be enabled by default, as deterministic build is a very nice property.

[rupprecht]

In D53913#1281601, @ruiu wrote:

I feel it should be enabled by default, as deterministic build is a very nice property.

It was before this patch and it still is. This is providing the option to make it not default (if anyone wants that), and also won't break if someone is explicitly running "objcopy --enable-deterministic-archives" and we drop llvm-objcopy in place of that.
I can add a test case for that, actually.

[rupprecht]

• Add test case that deterministic is the default

[ruiu]

It was before this patch and it still is. This is providing the option to make it not default (if anyone wants that), and also won't break if someone is explicitly running "objcopy --enable-deterministic-archives" and we drop llvm-objcopy in place of that.

Oh sorry I didn't notice that. What was the motivation for you to add a new flag so that you can choose deterministic and non-deterministic? (Just wondering.)

[rupprecht]

In D53913#1281610, @ruiu wrote:

It was before this patch and it still is. This is providing the option to make it not default (if anyone wants that), and also won't break if someone is explicitly running "objcopy --enable-deterministic-archives" and we drop llvm-objcopy in place of that.

Oh sorry I didn't notice that. What was the motivation for you to add a new flag so that you can choose deterministic and non-deterministic? (Just wondering.)

These flags exist in GNU strip/objcopy/ar (see [man strip]), so it isn't really "new".
If we convert users of strip to llvm-strip, they will break if they are specifying --enable-determinstic-archives and this flag is not defined.

[ruiu]

I do not see any problem of adding these flags for the sake of compatibility with GNU. What I wondered is whether or not you really have a need to make it nondeterministic. I thought that we might be able to just ignore these flags.

[jhenderson]

In D53913#1281632, @ruiu wrote:

I do not see any problem of adding these flags for the sake of compatibility with GNU. What I wondered is whether or not you really have a need to make it nondeterministic. I thought that we might be able to just ignore these flags.

Personally, I think that if the default is to make them deterministic, then I don't see any reason not to allow overriding this default, as the code does not become significantly more complex with this change. However, I could be persuaded that nobody has this use case.

test/tools/llvm-objcopy/deterministic-archive.test

23–26

Missing llvm-strip equivalents?

29–30

Ditto.

tools/llvm-objcopy/CopyConfig.cpp

348–349

Is this what GNU objcopy does? I'd actually expect this to be a "last one wins" scenario (similar to various --[no-]some-option in LLD etc.

425–426

Same comment as above.

[rupprecht]

• Implement last-one-wins for -D/-U

[rupprecht]

In D53913#1281963, @jhenderson wrote:

In D53913#1281632, @ruiu wrote:

I do not see any problem of adding these flags for the sake of compatibility with GNU. What I wondered is whether or not you really have a need to make it nondeterministic. I thought that we might be able to just ignore these flags.

Personally, I think that if the default is to make them deterministic, then I don't see any reason not to allow overriding this default, as the code does not become significantly more complex with this change. However, I could be persuaded that nobody has this use case.

Ditto -- sorry for not understanding the comment earlier.

I only have a use case for explicitly setting deterministic archives. I don't have a use case for non-deterministic archives, although I could imagine some build systems maybe want it (for preserving timestamps). At any rate, it's trivial to provide. I think the onus is on someone providing evidence that exposing this option is *harmful*.

tools/llvm-objcopy/CopyConfig.cpp

348–349

I thought it was simpler to just error instead of match objcopy, but it actually comes to fewer lines implementing last-one-wins. Done.

[jhenderson]

tools/llvm-objcopy/CopyConfig.cpp

348–349

You don't need to use the filtered and loop method at all, you can just use the hasFlag method, which takes both a positive and negative option:

I think this is right (various LLD options are implemented this way):

Config->DeterministicArchives(OOBJCOPY_enable_deterministic_archvies, OBJCOPY_disable_deterministic_archives, /*default*/true);

tools/llvm-objcopy/ObjcopyOpts.td

42

Somebody pointed out in an llvm-objdump review (see D53804) that Alias options are not in the help text by default (they are hidden), but they really should be. This might want to be a wider issue for us to fix at some point though, but in the meantime, perhaps you should add these particular ones? What do you think?

[rupprecht]

• Use better flag parsing, and include alias in --help

[rupprecht]

tools/llvm-objcopy/CopyConfig.cpp

348–349

Oh, I didn't see that method. Even shorter now, thanks! :)

tools/llvm-objcopy/ObjcopyOpts.td

42

Agreed that --help is terrible. It also bothers me that we print help for both "--foo=value" and "--foo value". Fixed for these ones.

[jhenderson]

LGTM.

Might want to wait to see if @ruiu or anybody else has any other comments though.

[ruiu]

You don't need to wait for my LGTM for this, but it is LGTM. :)

test/tools/llvm-objcopy/deterministic-archive.test

+# RUN: yaml2obj %s > %t.o
+
+# Create an archive, specifying U so that timestamps/etc. are preserved.
+# We only test timestamps as a proxy for full deterministic writing; i.e. we
+# assume UID/GIDs are preserved if timestamps are preserved.
+# RUN: touch -t 199505050555.55 %t.o
+# RUN: rm -f %t.a
+# RUN: llvm-ar crsU %t.a %t.o
+
+# Test short flags.
+# RUN: llvm-objcopy -D %t.a %t.2D.a
+# RUN: env TZ=GMT llvm-ar tv %t.2D.a | FileCheck %s --check-prefix=CHECK-DETERMINISTIC
+# RUN: llvm-objcopy -U %t.a %t.2U.a
+# RUN: env TZ=GMT llvm-ar tv %t.2U.a | FileCheck %s --check-prefix=CHECK-NONDETERMINISTIC
+
+# RUN: llvm-strip -D %t.a -o %t.3D.a
+# RUN: env TZ=GMT llvm-ar tv %t.3D.a | FileCheck %s --check-prefix=CHECK-DETERMINISTIC
+# RUN: llvm-strip -U %t.a -o %t.3U.a
+# RUN: env TZ=GMT llvm-ar tv %t.3U.a | FileCheck %s --check-prefix=CHECK-NONDETERMINISTIC
+
+# Test long flags.
+# RUN: llvm-objcopy --enable-deterministic-archives %t.a %t.4D.a
+# RUN: env TZ=GMT llvm-ar tv %t.4D.a | FileCheck %s --check-prefix=CHECK-DETERMINISTIC
+# RUN: llvm-objcopy --disable-deterministic-archives %t.a %t.4U.a
+# RUN: env TZ=GMT llvm-ar tv %t.4U.a | FileCheck %s --check-prefix=CHECK-NONDETERMINISTIC
+
+# Test various forms of invalid flag combintations.
+# RUN: not llvm-objcopy -U -D %t.a 2>&1 \
+# RUN:     | FileCheck %s --check-prefix=NO-MIXING
+# RUN: not llvm-objcopy --enable-deterministic-archives \
+# RUN:     --disable-deterministic-archives %t.a 2>&1 \
+# RUN:     | FileCheck %s --check-prefix=NO-MIXING
+# RUN: not llvm-objcopy --disable-deterministic-archives -D %t.a 2>&1 \
+# RUN:     | FileCheck %s --check-prefix=NO-MIXING
+
+# CHECK-DETERMINISTIC: {{[[:space:]]1970[[:space:]]}}
+# CHECK-NONDETERMINISTIC:  {{[[:space:]]1995[[:space:]]}}
+# NO-MIXING: Specifying both --enable-deterministic-archives and --disable-deterministic-archives is ambiguous.
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]

tools/llvm-objcopy/CopyConfig.h

struct CopyConfig {
   std::vector<StringRef> ToRemove;
   std::vector<std::string> SymbolsToKeepGlobal;
 
   // Map options
   StringMap<SectionRename> SectionsToRename;
   StringMap<StringRef> SymbolsToRename;
 
   // Boolean options
+  bool DeterministicArchives = true;
   bool DiscardAll = false;
   bool ExtractDWO = false;
   bool KeepFileSymbols = false;
   bool LocalizeHidden = false;
   bool OnlyKeepDebug = false;
   bool PreserveDates = false;
   bool StripAll = false;
   bool StripAllGNU = false;

tools/llvm-objcopy/CopyConfig.cpp

for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbol))
     Config.SymbolsToGlobalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbol))
     Config.SymbolsToWeaken.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbol))
     Config.SymbolsToRemove.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     Config.SymbolsToKeep.push_back(Arg->getValue());
 
+  if (InputArgs.hasArg(OBJCOPY_enable_deterministic_archives) &&
+      InputArgs.hasArg(OBJCOPY_disable_deterministic_archives))
+    error("Specifying both --enable-deterministic-archives and "
+          "--disable-deterministic-archives is ambiguous");
+  if (InputArgs.hasArg(OBJCOPY_enable_deterministic_archives))
+    Config.DeterministicArchives = true;
+  else if (InputArgs.hasArg(OBJCOPY_disable_deterministic_archives))
+    Config.DeterministicArchives = false;
+
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);
 
   if (Config.DecompressDebugSections &&
       Config.CompressionType != DebugCompressionType::None) {
     error("Cannot specify --compress-debug-sections at the same time as "
           "--decompress-debug-sections at the same time");
   }
 
if (!Config.StripDebug && !Config.StripUnneeded && !Config.DiscardAll)
     Config.StripAll = true;
 
   for (auto Arg : InputArgs.filtered(STRIP_remove_section))
     Config.ToRemove.push_back(Arg->getValue());
 
   for (auto Arg : InputArgs.filtered(STRIP_keep_symbol))
     Config.SymbolsToKeep.push_back(Arg->getValue());
 
+  if (InputArgs.hasArg(STRIP_enable_deterministic_archives) &&
+      InputArgs.hasArg(STRIP_disable_deterministic_archives))
+    error("Specifying both --enable-deterministic-archives and "
+          "--disable-deterministic-archives is ambiguous");
+  if (InputArgs.hasArg(STRIP_enable_deterministic_archives))
+    Config.DeterministicArchives = true;
+  else if (InputArgs.hasArg(STRIP_disable_deterministic_archives))
+    Config.DeterministicArchives = false;
+
   Config.PreserveDates = InputArgs.hasArg(STRIP_preserve_dates);
 
   DriverConfig DC;
   if (Positional.size() == 1) {
     Config.InputFilename = Positional[0];
     Config.OutputFilename =
         InputArgs.getLastArgValue(STRIP_output, Positional[0]);
     DC.CopyConfigs.push_back(std::move(Config));

tools/llvm-objcopy/ObjcopyOpts.td

 def decompress_debug_sections : Flag<["-", "--"], "decompress-debug-sections">,
                                 HelpText<"Decompress DWARF debug sections.">;
 def O : JoinedOrSeparate<["-"], "O">,
         Alias<output_target>;
 defm split_dwo : Eq<"split-dwo">,
                  MetaVarName<"dwo-file">,
                  HelpText<"Equivalent to extract-dwo on the input file to <dwo-file>, then strip-dwo on the input file">;
 
+def enable_deterministic_archives
+    : Flag<["-", "--"], "enable-deterministic-archives">,
+      HelpText<"Enable deterministic mode when stripping archives (use zero "
+               "for UIDs, GIDs, and timestamps).">;
+def D : Flag<[ "-" ], "D">, Alias<enable_deterministic_archives>;
+
+def disable_deterministic_archives
+    : Flag<["-", "--"], "disable-deterministic-archives">,
+      HelpText<"Disable deterministic mode when stripping archives (use real "
+               "values for UIDs, GIDs, and timestamps).">;
+def U : Flag<[ "-" ], "U">, Alias<disable_deterministic_archives>;
+
 def preserve_dates : Flag<[ "-", "--" ], "preserve-dates">,
                      HelpText<"Preserve access and modification timestamps">;
 
 def p : Flag<[ "-" ], "p">, Alias<preserve_dates>;
 
 defm add_gnu_debuglink : Eq<"add-gnu-debuglink">,
                          MetaVarName<"debug-file">,
                          HelpText<"Add a .gnu_debuglink for <debug-file>">;

tools/llvm-objcopy/StripOpts.td

 include "llvm/Option/OptParser.td"
 
 multiclass Eq<string name> {
   def NAME: Separate<["--", "-"], name>;
   def NAME # _eq: Joined<["--", "-"], name # "=">, Alias<!cast<Separate>(NAME)>;
 }
 
 def help : Flag<["-", "--"], "help">;
 
+def enable_deterministic_archives
+    : Flag<["-", "--"], "enable-deterministic-archives">,
+      HelpText<"Enable deterministic mode when stripping archives (use zero "
+               "for UIDs, GIDs, and timestamps).">;
+def D : Flag<[ "-" ], "D">, Alias<enable_deterministic_archives>;
+
+def disable_deterministic_archives
+    : Flag<["-", "--"], "disable-deterministic-archives">,
+      HelpText<"Disable deterministic mode when stripping archives (use real "
+               "values for UIDs, GIDs, and timestamps).">;
+def U : Flag<[ "-" ], "U">, Alias<disable_deterministic_archives>;
+
 defm output : Eq<"o">,
               MetaVarName<"output">,
               HelpText<"Write output to <file>">;
 
 def preserve_dates : Flag<[ "-", "--" ], "preserve-dates">,
                      HelpText<"Preserve access and modification timestamps">;
 
 def p : Flag<[ "-" ], "p">, Alias<preserve_dates>;

tools/llvm-objcopy/llvm-objcopy.cpp

for (const Archive::Child &Child : Ar.children(Err)) {
     Expected<StringRef> ChildNameOrErr = Child.getName();
     if (!ChildNameOrErr)
       reportError(Ar.getFileName(), ChildNameOrErr.takeError());
 
     MemBuffer MB(ChildNameOrErr.get());
     executeObjcopyOnBinary(Config, *Bin, MB);
 
     Expected<NewArchiveMember> Member =
-        NewArchiveMember::getOldMember(Child, true);
+        NewArchiveMember::getOldMember(Child, Config.DeterministicArchives);
     if (!Member)
       reportError(Ar.getFileName(), Member.takeError());
     Member->Buf = MB.releaseMemoryBuffer();
     Member->MemberName = Member->Buf->getBufferIdentifier();
     NewArchiveMembers.push_back(std::move(*Member));
   }
 
   if (Err)
     reportError(Config.InputFilename, std::move(Err));
-  if (Error E =
-          deepWriteArchive(Config.OutputFilename, NewArchiveMembers,
-                           Ar.hasSymbolTable(), Ar.kind(), true, Ar.isThin()))
+  if (Error E = deepWriteArchive(Config.OutputFilename, NewArchiveMembers,
+                                 Ar.hasSymbolTable(), Ar.kind(),
+                                 Config.DeterministicArchives, Ar.isThin()))
     reportError(Config.OutputFilename, std::move(E));
 }
 
 static void restoreDateOnFile(StringRef Filename,
                               const sys::fs::file_status &Stat) {
   int FD;
 
   if (auto EC =