[v2] ar: Add --thin for creating thin archives

Message ID 20220110223454.3608270-1-maskray@google.com
State New
Headers show
Series
  • [v2] ar: Add --thin for creating thin archives
Related show

Commit Message

H.J. Lu via Binutils Jan. 10, 2022, 10:34 p.m.
In many ar implementations (FreeBSD, elfutils, etc), ar -T has the
X/Open System Interface specified semantics. Therefore -T for thin
archives is not recommended for portability.

    PR binutils/28759
    * ar.c (long_options): Add --thin.
    (usage) Add --thin. Soft deprecate -T.
    * doc/binutils.texi: Add doc.
    * NEWS: Mention --thin.
    * binutils/testsuite/binutils-all/ar.exp: Add tests.
---
 binutils/NEWS                          |  4 ++++
 binutils/ar.c                          |  4 +++-
 binutils/doc/binutils.texi             | 15 ++++++++++-----
 binutils/testsuite/binutils-all/ar.exp | 17 +++++++++++++++++
 4 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.34.1.575.g55b058a8bb-goog

Comments

H.J. Lu via Binutils Jan. 11, 2022, 8:20 a.m. | #1
On 10.01.2022 23:34, Fangrui Song via Binutils wrote:
> In many ar implementations (FreeBSD, elfutils, etc), ar -T has the

> X/Open System Interface specified semantics. Therefore -T for thin

> archives is not recommended for portability.

> 

>     PR binutils/28759

>     * ar.c (long_options): Add --thin.

>     (usage) Add --thin. Soft deprecate -T.

>     * doc/binutils.texi: Add doc.

>     * NEWS: Mention --thin.

>     * binutils/testsuite/binutils-all/ar.exp: Add tests.

> ---

>  binutils/NEWS                          |  4 ++++

>  binutils/ar.c                          |  4 +++-

>  binutils/doc/binutils.texi             | 15 ++++++++++-----

>  binutils/testsuite/binutils-all/ar.exp | 17 +++++++++++++++++

>  4 files changed, 34 insertions(+), 6 deletions(-)

> 

> diff --git a/binutils/NEWS b/binutils/NEWS

> index 903f8233b99..d62f019bc82 100644

> --- a/binutils/NEWS

> +++ b/binutils/NEWS

> @@ -18,6 +18,10 @@

>  * Support for efi-app-aarch64, efi-rtdrv-aarch64 and efi-bsdrv-aarch64 has been

>    added to objcopy in order to enable UEFI development using binutils.

>  

> +* ar: Add --thin for creating thin archives. -T is deprecated (no diagnostic)

> +  for because in many ar implementations -T has a different meaning from X/Open

> +  System Interface.


I'm not a native speaker, so I may be entirely off, but this wording reads
to me as the opposite of what the description says: It feels as if you said
"has a meaning different from X/Open System Interface" (two words swapped).
Yet aiui you mean "has a meaning as in the X/Open System Interface". If I
was to suggest a wording change keeping your text largely intact, how about
"has a different meaning, as specified by the X/Open System Interface"?

I also wonder about "for because". Imo it should be just one of the two,
and if "for" was chosen, grammar would need adjusting later in the sentence.

Same (obviously) for the doc change.

Jan
H.J. Lu via Binutils Jan. 11, 2022, 4:26 p.m. | #2
On 2022-01-11, Jan Beulich wrote:
>On 10.01.2022 23:34, Fangrui Song via Binutils wrote:

>> In many ar implementations (FreeBSD, elfutils, etc), ar -T has the

>> X/Open System Interface specified semantics. Therefore -T for thin

>> archives is not recommended for portability.

>>

>>     PR binutils/28759

>>     * ar.c (long_options): Add --thin.

>>     (usage) Add --thin. Soft deprecate -T.

>>     * doc/binutils.texi: Add doc.

>>     * NEWS: Mention --thin.

>>     * binutils/testsuite/binutils-all/ar.exp: Add tests.

>> ---

>>  binutils/NEWS                          |  4 ++++

>>  binutils/ar.c                          |  4 +++-

>>  binutils/doc/binutils.texi             | 15 ++++++++++-----

>>  binutils/testsuite/binutils-all/ar.exp | 17 +++++++++++++++++

>>  4 files changed, 34 insertions(+), 6 deletions(-)

>>

>> diff --git a/binutils/NEWS b/binutils/NEWS

>> index 903f8233b99..d62f019bc82 100644

>> --- a/binutils/NEWS

>> +++ b/binutils/NEWS

>> @@ -18,6 +18,10 @@

>>  * Support for efi-app-aarch64, efi-rtdrv-aarch64 and efi-bsdrv-aarch64 has been

>>    added to objcopy in order to enable UEFI development using binutils.

>>

>> +* ar: Add --thin for creating thin archives. -T is deprecated (no diagnostic)

>> +  for because in many ar implementations -T has a different meaning from X/Open

>> +  System Interface.

>

>I'm not a native speaker, so I may be entirely off, but this wording reads

>to me as the opposite of what the description says: It feels as if you said

>"has a meaning different from X/Open System Interface" (two words swapped).

>Yet aiui you mean "has a meaning as in the X/Open System Interface". If I

>was to suggest a wording change keeping your text largely intact, how about

>"has a different meaning, as specified by the X/Open System Interface"?

>

>I also wonder about "for because". Imo it should be just one of the two,

>and if "for" was chosen, grammar would need adjusting later in the sentence.

>

>Same (obviously) for the doc change.

>

>Jan

>

Me neither:) Thank you for the suggestion! I'll adopt it.
"... many ar implementations @option{T} has a different meaning, as specified
by X/Open System Interface."

Patch

diff --git a/binutils/NEWS b/binutils/NEWS
index 903f8233b99..d62f019bc82 100644
--- a/binutils/NEWS
+++ b/binutils/NEWS
@@ -18,6 +18,10 @@ 
 * Support for efi-app-aarch64, efi-rtdrv-aarch64 and efi-bsdrv-aarch64 has been
   added to objcopy in order to enable UEFI development using binutils.
 
+* ar: Add --thin for creating thin archives. -T is deprecated (no diagnostic)
+  for because in many ar implementations -T has a different meaning from X/Open
+  System Interface.
+
 Changes in 2.37:
 
 * The readelf tool has a new command line option which can be used to specify
diff --git a/binutils/ar.c b/binutils/ar.c
index d7d2fc21dd9..0d4c7cf16a6 100644
--- a/binutils/ar.c
+++ b/binutils/ar.c
@@ -172,6 +172,7 @@  static struct option long_options[] =
   {"version", no_argument, &show_version, 1},
   {"output", required_argument, NULL, OPTION_OUTPUT},
   {"record-libdeps", required_argument, NULL, 'l'},
+  {"thin", no_argument, NULL, 'T'},
   {NULL, no_argument, NULL, 0}
 };
 
@@ -337,13 +338,14 @@  usage (int help)
   fprintf (s, _("  [s]          - create an archive index (cf. ranlib)\n"));
   fprintf (s, _("  [l <text> ]  - specify the dependencies of this library\n"));
   fprintf (s, _("  [S]          - do not build a symbol table\n"));
-  fprintf (s, _("  [T]          - make a thin archive\n"));
+  fprintf (s, _("  [T]          - deprecated, use --thin instead\n"));
   fprintf (s, _("  [v]          - be verbose\n"));
   fprintf (s, _("  [V]          - display the version number\n"));
   fprintf (s, _("  @<file>      - read options from <file>\n"));
   fprintf (s, _("  --target=BFDNAME - specify the target object format as BFDNAME\n"));
   fprintf (s, _("  --output=DIRNAME - specify the output directory for extraction operations\n"));
   fprintf (s, _("  --record-libdeps=<text> - specify the dependencies of this library\n"));
+  fprintf (s, _("  --thin       - make a thin archive\n"));
 #if BFD_SUPPORTS_PLUGINS
   fprintf (s, _(" optional:\n"));
   fprintf (s, _("  --plugin <p> - load the specified plugin\n"));
diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 446c275a5db..de1a58e73ad 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -256,7 +256,7 @@  program.
 
 @smallexample
 @c man begin SYNOPSIS ar
-ar [@option{-X32_64}] [@option{-}]@var{p}[@var{mod}] [@option{--plugin} @var{name}] [@option{--target} @var{bfdname}] [@option{--output} @var{dirname}] [@option{--record-libdeps} @var{libdeps}] [@var{relpos}] [@var{count}] @var{archive} [@var{member}@dots{}]
+ar [@option{-X32_64}] [@option{-}]@var{p}[@var{mod}] [@option{--plugin} @var{name}] [@option{--target} @var{bfdname}] [@option{--output} @var{dirname}] [@option{--record-libdeps} @var{libdeps}] [@option{--thin}] [@var{relpos}] [@var{count}] @var{archive} [@var{member}@dots{}]
 @c man end
 @end smallexample
 
@@ -507,10 +507,9 @@  with the linker.  In order to build a symbol table, you must omit the
 @samp{ranlib} on the archive.
 
 @item T
-@cindex creating thin archive
-Make the specified @var{archive} a @emph{thin} archive.  If it already
-exists and is a regular archive, the existing members must be present
-in the same directory as @var{archive}.
+Deprecated alias for @option{--thin}.  @option{T} is not recommended because in
+many ar implementations @option{T} has a different meaning from X/Open System
+Interface.
 
 @item u
 @cindex updating an archive
@@ -603,6 +602,12 @@  line.
 The @option{--record-libdeps} option is identical to the @option{l} modifier,
 just handled in long form.
 
+@item --thin
+@cindex creating thin archive
+Make the specified @var{archive} a @emph{thin} archive.  If it already
+exists and is a regular archive, the existing members must be present
+in the same directory as @var{archive}.
+
 @end table
 @c man end
 
diff --git a/binutils/testsuite/binutils-all/ar.exp b/binutils/testsuite/binutils-all/ar.exp
index 12aa079865b..3b841825f6f 100644
--- a/binutils/testsuite/binutils-all/ar.exp
+++ b/binutils/testsuite/binutils-all/ar.exp
@@ -309,11 +309,13 @@  proc thin_archive_with_nested { bfdtests } {
     if [is_remote host] {
 	set archive artest.a
 	set archive2 artest2.a
+	set archive3 artest3.a
 	set objfile [remote_download host tmpdir/bintest.${obj}]
 	remote_file host delete $archive
     } else {
 	set archive tmpdir/artest.a
 	set archive2 tmpdir/artest2.a
+	set archive3 tmpdir/artest3.a
 	set objfile tmpdir/bintest.${obj}
     }
 
@@ -333,6 +335,14 @@  proc thin_archive_with_nested { bfdtests } {
 	return
     }
 
+    remote_file build delete tmpdir/artest3.a
+
+    set got [binutils_run $AR "rc --thin $archive3 ${archive}"]
+    if ![string match "" $got] {
+	fail $testname
+	return
+    }
+
     foreach bfdtest $bfdtests {
 	set exec_output [binutils_run "$base_dir/$bfdtest" "$archive"]
 	if ![string match "" $exec_output] {
@@ -347,6 +357,13 @@  proc thin_archive_with_nested { bfdtests } {
 	    fail "$testname ($bfdtest)"
 	    return
 	}
+
+	set exec_output [binutils_run "$base_dir/$bfdtest" "$archive3"]
+	if ![string match "" $exec_output] {
+	    verbose -log $exec_output
+	    fail "$testname ($bfdtest)"
+	    return
+	}
     }
 
     set got [binutils_run $NM "--print-armap $archive"]