[llvm-ar] Add --thin for creating a thin archive

[MaskRay]

Summary

In GNU ar (since 2008), the modifier 'T' means creating a thin archive.
In many other ar implementations (FreeBSD, macOS, elfutils, etc), -T
means "allow filename truncation of extracted files", as specified by
X/Open System Interface.

For portability, 'T' with thin archive semantics should be avoided.

See https://sourceware.org/bugzilla/show_bug.cgi?id=28759 binutils 2.38
will deprecate 'T' (without diagnostic) and add --thin.

[compnerd]

llvm/tools/llvm-ar/llvm-ar.cpp

122

[MaskRay]

fix typo

[MaskRay]

update doc

[jhenderson]

llvm/docs/CommandGuide/llvm-ar.rst

204–205 ↗

(On Diff #398766)

"from" isn't the right word here, and because of that, I'm struggling to understand the meaning of this sentence. I'd suggest rewording.

I'm also somewhat opposed to deprecating the existing form. We have many users who are using thin archives, and will be using the T form to generate their archives. Deprecation implies that the option will be removed in a few releases time, but that would break all those users. Yes, it's a small change for them to make, but I don't really see the benefit. Most people probably don't care about the portability aspects of their build scripts.

Another related issue is that T is a modifier that is grouped with other operations and modifiers like 'r' and 'c', to form Trc or similar, whereas --thin is a long-form option. I suspect most llvm-ar users are more familiar with the short-form versions, and don't even know that llvm-ar has a long-form.

284–287 ↗

(On Diff #398766)

Text isn't wrapped properly.

[MaskRay]

Reworking documentation

llvm/docs/CommandGuide/llvm-ar.rst

204–205 ↗

(On Diff #398766)

Reworded.

The deprecation semantic here is soft - no diagnostics. I think it will keep this way for a while. The Linux kernel uses it (kbuild: allow architectures to use thin archives instead of ld -r) since 2016.

elfutils has had the XSI -T since 2007. FreeBSD's and macOS's may be earlier. elfutils is used a lot on Linux, too, though its usage, compared with binutils, is fairly small. The thin/XSI conflict is unfortunate, and llvm-ar taking 'T' has some issues outside Linux (https://github.com/llvm/llvm-project/issues/25899#issuecomment-1009388242). If you find packages using T for thin archives, I'd still suggest that you try migrating away, if the available version (binutils>=2.38, llvm-ar>=14) is not a problem.

If you still think "Deprecated alias for --thin" is a problem, how about "Alias for --thin for compatibility"?

llvm/tools/llvm-ar/llvm-ar.cpp

122

Thanks!

[gbreynoo]

I understand your hesitation @jhenderson regarding the loss of the short option, maybe a replacement short option could be found?

[jhenderson]

llvm/docs/CommandGuide/llvm-ar.rst

204–205 ↗

(On Diff #398766)

Rather than deprecating the llvm-ar meaning of -T, perhaps the elfutils version should be deprecated? Truncating the filename hardly seems like useful functionality to me... I noticed that @emaste has turned off the -T functionality in FreeBSD ar, which means there's only macOS and elfutils remaining as other archivers (and including our proprietary archiver, I know of three current archivers that use T for thin archives - our version, llvm-ar and GNU ar).

llvm-ar is designed primarily to be a drop-in replacement for GNU ar. I guess I'm okay if we deprecate the option, if the GNU binutils community wish to deprecate it on their side (though I would question if that community has really considered this heavily), and we can then decide whether to remove it in the future, if they choose to. If they don't there's no point in deprecating it in llvm-ar.

[MaskRay]

llvm/docs/CommandGuide/llvm-ar.rst

204–205 ↗

(On Diff #398766)

The 2004 edition of https://pubs.opengroup.org/onlinepubs/009696899/utilities/ar.html mentioned -T, so this was indeed a binutils created portability issue. I think it is unfair to say that elfutils T is the one to be deprecated....

As of binutils 2.38, T has been deprecated (just without diagnostic). I hope folks can find users of T for thin archives can help migrate them to --thin. After a while, binutils T can introduce a warning and perhaps after few years T can be removed.

[MaskRay]

Ping:)

[jhenderson]

I'm still not particularly happy with either the interface (double dash options in archivers are pretty unusual) or wording (I'm not convinced we should be deprecating a useful option due to it clashing with one that has no particular benefit, although I acknowledge the Open Group publication), but I don't think it's enough to block this change. LGTM.

[gbreynoo]

Hi @MaskRay, apologies for the wait in this review. I think James brought up a good point regarding gnu ar still using T. I realise that when moving towards consistency someone has to go first, but it maybe worth us waiting until they make the change so we maintain compatibility. Do we know if they have plans to move over to --thin?

[MaskRay]

@jhenderson Thanks!

In D116979#3286974, @gbreynoo wrote:

Hi @MaskRay, apologies for the wait in this review. I think James brought up a good point regarding gnu ar still using T. I realise that when moving towards consistency someone has to go first, but it maybe worth us waiting until they make the change so we maintain compatibility. Do we know if they have plans to move over to --thin?

GNU ar's -T has been deprecated. The problem is whether any project is using T. These projects cannot straightforwardly migrate if llvm-ar does not have T.
The T problem may also be the reason that other implementations cannot change their T.

I think GNU ar T for thin may be removed, but that may take several years...

For double-dash options, GNU ar now has more such options:

 --target=BFDNAME - specify the target object format as BFDNAME
 --output=DIRNAME - specify the output directory for extraction operations
 --record-libdeps=<text> - specify the dependencies of this library
optional:
 --plugin <p> - load the specified plugin

It's indeed rare in other tools. (POSIX doesn't specify --; -- may be a GNU thing)

[MaskRay]

Don't say 'T' is deprecated

[gbreynoo]

@MaskRay Thanks for clarifying.

llvm/test/tools/llvm-ar/thin-archive.test

 RUN: rm -rf %t && mkdir -p %t/foo/bar/
 
 RUN: yaml2obj %S/Inputs/elf.yaml -o %t/foo/elf.o
 RUN: cp %t/foo/elf.o %t/foo/bar/elf.o
 RUN: cp %t/foo/bar/elf.o %t/delete.o
 
 Test that modules can be added with absolute paths when the archive is created using an absolute path
 
-RUN: llvm-ar rTc %t/absolute-1.ar %t/foo/elf.o %t/delete.o %t/foo/bar/elf.o
-RUN: llvm-ar dT %t/absolute-1.ar delete.o
+RUN: llvm-ar rc --thin %t/absolute-1a.ar %t/foo/elf.o %t/delete.o %t/foo/bar/elf.o
+RUN: llvm-ar --thin d %t/absolute-1a.ar delete.o
 
-RUN: FileCheck -input-file=%t/absolute-1.ar --check-prefixes=THIN,CHECK %s -DPATH=%/t/
-RUN: llvm-ar t %t/absolute-1.ar | FileCheck %s -DPATH=%/t/
+RUN: FileCheck --input-file=%t/absolute-1a.ar --check-prefixes=THIN,CHECK %s -DPATH=%/t/
+RUN: llvm-ar t %t/absolute-1a.ar | FileCheck %s -DPATH=%/t/
+
+RUN: llvm-ar rTc %t/absolute-1b.ar %t/foo/elf.o %t/delete.o %t/foo/bar/elf.o
+RUN: llvm-ar dT %t/absolute-1b.ar delete.o
+RUN: cmp %t/absolute-1a.ar %t/absolute-1b.ar
 
 These tests must be run in %t/foo. cd %t is included on each line to make debugging this test case easier.
 
 Test that modules can be added with absolute paths when the archive is created using a relative path
 
 RUN: cd %t/foo && llvm-ar rTc bar/absolute-2.ar %t/foo/elf.o %t/delete.o %t/foo/bar/elf.o
 RUN: cd %t/foo && llvm-ar dT bar/absolute-2.ar %t/delete.o
 

llvm/tools/llvm-ar/llvm-ar.cpp

--format              - archive format to create
     =gnu                -   gnu
     =darwin             -   darwin
     =bsd                -   bsd
   --plugin=<string>     - ignored for compatibility
   -h --help             - display this help and exit
   --rsp-quoting         - quoting style for response files
     =posix              -   posix
     =windows            -   windows
+  --thin                - create a thin archive
   --version             - print the version and exit
   @<file>               - read options from <file>
 
 OPERATIONS:
   d - delete [files] from the archive
   m - move [files] in the archive
   p - print contents of [files] found in the archive
   q - quick append [files] to the archive
MODIFIERS:
   [l] - ignored for compatibility
   [L] - add archive's contents
   [N] - use instance [count] of name
   [o] - preserve original dates
   [O] - display member offsets
   [P] - use full names when matching (implied for thin archives)
   [s] - create an archive index (cf. ranlib)
   [S] - do not build a symbol table
-  [T] - create a thin archive
+  [T] - depreacted, use --thin instead
   [u] - update only [files] newer than archive contents
   [U] - use actual timestamps and uids/gids
   [v] - be verbose about actions taken
   [V] - display the version and exit
 )";
 
 static void printHelpMessage() {
   if (Stem.contains_insensitive("ranlib"))
for (unsigned i = 0; i < Options.size(); ++i) {
     case 'U':
       Deterministic = false;
       break;
     case 'N':
       getCountParam();
       break;
     case 'T':
       Thin = true;
-      // Thin archives store path names, so P should be forced.
-      CompareFullPath = true;
       break;
     case 'L':
       AddLibrary = true;
       break;
     case 'V':
       cl::PrintVersionMessage();
       exit(0);
     case 'h':
       printHelpMessage();
       exit(0);
     default:
       badUsage(std::string("unknown option ") + Options[i]);
     }
   }
 
+  // Thin archives store path names, so P should be forced.
+  if (Thin)
+    CompareFullPath = true;
+
   // At this point, the next thing on the command line must be
   // the archive name.
   getArchive();
 
   // Everything on the command line at this point is a member.
   Members.assign(PositionalArgs.begin(), PositionalArgs.end());
 
   if (NumOperations == 0 && MaybeJustCreateSymTab) {
if (*ArgIt[0] != '-') {
       continue;
     }
 
     if (strcmp(*ArgIt, "-M") == 0) {
       MRI = true;
       continue;
     }
 
+    if (strcmp(*ArgIt, "--thin") == 0) {
+      Thin = true;
+      continue;
+    }
+
     Match = matchFlagWithArg("format", ArgIt, Argv);
     if (Match) {
       FormatType = StringSwitch<Format>(Match)
                        .Case("default", Default)
                        .Case("gnu", GNU)
                        .Case("darwin", DARWIN)
                        .Case("bsd", BSD)
                        .Default(Unknown);