* [PATCH] gas: Add --force-compress-debug-sections
@ 2023-02-23 12:45 Tom de Vries
2023-02-23 13:08 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: Tom de Vries @ 2023-02-23 12:45 UTC (permalink / raw)
To: binutils
Gas has an option --compress-debug-sections that allows it to generate
compressed debug sections.
That does not guarantee that the debug sections are in fact compressed:
...
$ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
[11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
[12] .debug_info PROGBITS 000120 000039 00 0 0 1
[14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
[15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
[17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
...
Sensibly so, they're only compressed if that provides a size benefit.
However, for the purposes of testing components consuming dwarf
we may want the sections to be compressed regardless.
Add a new option --force-compress-debug-sections that ignores the size
heuristic, such that we have instead:
...
$ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd \
-Wa,--force-compress-debug-sections
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
[11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
[12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
[14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
[15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
[17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
...
Advertised as:
...
$ as --help 2>&1 | grep compress
--compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]
compress DWARF debug sections
--nocompress-debug-sections
don't compress DWARF debug sections
--force-compress-debug-sections
force compression of DWARF debug sections
...
Tested on x86_64-linux.
---
gas/as.c | 13 ++++++++++++-
gas/as.h | 4 ++++
gas/doc/as.texi | 10 ++++++++--
gas/write.c | 4 ++--
4 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/gas/as.c b/gas/as.c
index 598bfd56cf5..835c51c609b 100644
--- a/gas/as.c
+++ b/gas/as.c
@@ -230,6 +230,8 @@ enum compressed_debug_section_type flag_compress_debug
= DEFAULT_COMPRESSED_DEBUG_ALGORITHM;
#endif
+bool flag_force_compress_debug = false;
+
static void
show_usage (FILE * stream)
{
@@ -262,6 +264,9 @@ Options:\n\
--nocompress-debug-sections\n\
don't compress DWARF debug sections\n"));
fprintf (stream, _("\
+ --force-compress-debug-sections\n\
+ force compression of DWARF debug sections\n")),
+ fprintf (stream, _("\
-D produce assembler debugging messages\n"));
fprintf (stream, _("\
--dump-config display how the assembler is configured and then exit\n"));
@@ -508,7 +513,8 @@ parse_args (int * pargc, char *** pargv)
OPTION_NOCOMPRESS_DEBUG,
OPTION_NO_PAD_SECTIONS,
OPTION_MULTIBYTE_HANDLING, /* = STD_BASE + 40 */
- OPTION_SFRAME
+ OPTION_SFRAME,
+ OPTION_FORCE_COMPRESS_DEBUG
/* When you add options here, check that they do
not collide with OPTION_MD_BASE. See as.h. */
};
@@ -528,6 +534,7 @@ parse_args (int * pargc, char *** pargv)
,{"al", optional_argument, NULL, OPTION_AL}
,{"compress-debug-sections", optional_argument, NULL, OPTION_COMPRESS_DEBUG}
,{"nocompress-debug-sections", no_argument, NULL, OPTION_NOCOMPRESS_DEBUG}
+ ,{"force-compress-debug-sections", no_argument, NULL, OPTION_FORCE_COMPRESS_DEBUG}
,{"debug-prefix-map", required_argument, NULL, OPTION_DEBUG_PREFIX_MAP}
,{"defsym", required_argument, NULL, OPTION_DEFSYM}
,{"dump-config", no_argument, NULL, OPTION_DUMPCONFIG}
@@ -771,6 +778,10 @@ This program has absolutely no warranty.\n"));
flag_compress_debug = COMPRESS_DEBUG_NONE;
break;
+ case OPTION_FORCE_COMPRESS_DEBUG:
+ flag_force_compress_debug = true;
+ break;
+
case OPTION_DEBUG_PREFIX_MAP:
add_debug_prefix_map (optarg);
break;
diff --git a/gas/as.h b/gas/as.h
index 4c5fa9ecf7d..3f8935d2827 100644
--- a/gas/as.h
+++ b/gas/as.h
@@ -331,6 +331,10 @@ COMMON int flag_traditional_format;
/* Type of compressed debug sections we should generate. */
COMMON enum compressed_debug_section_type flag_compress_debug;
+/* True if we want to generate compressed debug sections, even if it
+ doesn't make them smaller. */
+COMMON bool flag_force_compress_debug;
+
/* TRUE if .note.GNU-stack section with SEC_CODE should be created */
COMMON int flag_execstack;
diff --git a/gas/doc/as.texi b/gas/doc/as.texi
index bbdfa4bfdca..c853cafbbe5 100644
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -229,6 +229,7 @@ gcc(1), ld(1), and the Info entries for @file{binutils} and @file{ld}.
@value{AS} [@b{-a}[@b{cdghlns}][=@var{file}]]
[@b{--alternate}]
[@b{--compress-debug-sections}] [@b{--nocompress-debug-sections}]
+ [@b{--force-compress-debug-sections}]
[@b{-D}]
[@b{--dump-config}]
[@b{--debug-prefix-map} @var{old}=@var{new}]
@@ -718,7 +719,8 @@ Begin in alternate macro mode.
Compress DWARF debug sections using zlib with SHF_COMPRESSED from the
ELF ABI. The resulting object file may not be compatible with older
linkers and object file utilities. Note if compression would make a
-given section @emph{larger} then it is not compressed.
+given section @emph{larger} then it is not compressed, unless
+@option{--force-compress-debug-section} is used.
@ifset ELF
@cindex @samp{--compress-debug-sections=} option
@@ -738,7 +740,11 @@ using the obsoleted zlib-gnu format. The debug sections are renamed to begin
with @samp{.zdebug}.
@option{--compress-debug-sections=zstd} compresses DWARF debug
sections using zstd. Note - if compression would actually make a section
-@emph{larger}, then it is not compressed nor renamed.
+@emph{larger}, then it is not compressed nor renamed, unless
+@option{--force-compress-debug-section} is used.
+
+@item --force-compress-debug-sections
+Compress DWARF debug sections, even if this does not reduce size.
@end ifset
diff --git a/gas/write.c b/gas/write.c
index 8273b7a42f1..8fcbc326e3f 100644
--- a/gas/write.c
+++ b/gas/write.c
@@ -1465,7 +1465,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
flagword flags = bfd_section_flags (sec);
if (seginfo == NULL
- || uncompressed_size < 32
+ || (!flag_force_compress_debug && uncompressed_size < 32)
|| (flags & SEC_HAS_CONTENTS) == 0)
return;
@@ -1582,7 +1582,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
/* PR binutils/18087: If compression didn't make the section smaller,
just keep it uncompressed. */
- if (compressed_size >= uncompressed_size)
+ if (!flag_force_compress_debug && compressed_size >= uncompressed_size)
return;
/* Replace the uncompressed frag list with the compressed frag list. */
--
2.35.3
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-23 12:45 [PATCH] gas: Add --force-compress-debug-sections Tom de Vries
@ 2023-02-23 13:08 ` Jan Beulich
2023-02-23 13:27 ` Tom de Vries
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2023-02-23 13:08 UTC (permalink / raw)
To: Tom de Vries; +Cc: binutils
On 23.02.2023 13:45, Tom de Vries via Binutils wrote:
> Gas has an option --compress-debug-sections that allows it to generate
> compressed debug sections.
>
> That does not guarantee that the debug sections are in fact compressed:
> ...
> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
> $ readelf -S -W hello.o | grep " .debug"
> [ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
> [11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
> [12] .debug_info PROGBITS 000120 000039 00 0 0 1
> [14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
> [15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
> [17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
> ...
>
> Sensibly so, they're only compressed if that provides a size benefit.
>
> However, for the purposes of testing components consuming dwarf
> we may want the sections to be compressed regardless.
>
> Add a new option --force-compress-debug-sections that ignores the size
> heuristic, such that we have instead:
> ...
> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd \
> -Wa,--force-compress-debug-sections
> $ readelf -S -W hello.o | grep " .debug"
> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
> ...
>
> Advertised as:
> ...
> $ as --help 2>&1 | grep compress
> --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]
> compress DWARF debug sections
> --nocompress-debug-sections
> don't compress DWARF debug sections
> --force-compress-debug-sections
> force compression of DWARF debug sections
No objection in principle, but have you considered making this a new
sub-option to --compress-debug-sections, i.e. compress-debug-sections=force?
(I've actually been puzzled by --nocompress-debug-sections, which looks to
be no different than --compress-debug-sections=none.)
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-23 13:08 ` Jan Beulich
@ 2023-02-23 13:27 ` Tom de Vries
2023-02-23 13:44 ` Jan Beulich
2023-02-23 15:23 ` Michael Matz
0 siblings, 2 replies; 24+ messages in thread
From: Tom de Vries @ 2023-02-23 13:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils
On 2/23/23 14:08, Jan Beulich wrote:
> On 23.02.2023 13:45, Tom de Vries via Binutils wrote:
>> Gas has an option --compress-debug-sections that allows it to generate
>> compressed debug sections.
>>
>> That does not guarantee that the debug sections are in fact compressed:
>> ...
>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
>> $ readelf -S -W hello.o | grep " .debug"
>> [ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
>> [11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
>> [12] .debug_info PROGBITS 000120 000039 00 0 0 1
>> [14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
>> [15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
>> [17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
>> ...
>>
>> Sensibly so, they're only compressed if that provides a size benefit.
>>
>> However, for the purposes of testing components consuming dwarf
>> we may want the sections to be compressed regardless.
>>
>> Add a new option --force-compress-debug-sections that ignores the size
>> heuristic, such that we have instead:
>> ...
>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd \
>> -Wa,--force-compress-debug-sections
>> $ readelf -S -W hello.o | grep " .debug"
>> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
>> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
>> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
>> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
>> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
>> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>> ...
>>
>> Advertised as:
>> ...
>> $ as --help 2>&1 | grep compress
>> --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]
>> compress DWARF debug sections
>> --nocompress-debug-sections
>> don't compress DWARF debug sections
>> --force-compress-debug-sections
>> force compression of DWARF debug sections
>
> No objection in principle, but have you considered making this a new
> sub-option to --compress-debug-sections, i.e. compress-debug-sections=force?
I did consider adding a "force-" prefix variant for all the non-none
sub-options, but decided to go with the simplest solution first.
Your suggestion, --compress-debug-sections=force is more orthogonal,
though it breaks the pattern that all the sub-options are mutually
exclusive.
We could have it be standalone, so you'd do:
--compress-debug-sections=zstd --compress-debug-sections=force.
Or instead combined: --compress-debug-sections=force,zstd. Harder to
parse though, I suppose.
I guess this last one would be my preference, because it makes it clear
force is in a different category than zlib/zstd.
Thanks,
- Tom
> (I've actually been puzzled by --nocompress-debug-sections, which looks to
> be no different than --compress-debug-sections=none.)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-23 13:27 ` Tom de Vries
@ 2023-02-23 13:44 ` Jan Beulich
2023-02-24 10:52 ` Tom de Vries
2023-02-23 15:23 ` Michael Matz
1 sibling, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2023-02-23 13:44 UTC (permalink / raw)
To: Tom de Vries; +Cc: binutils
On 23.02.2023 14:27, Tom de Vries wrote:
> On 2/23/23 14:08, Jan Beulich wrote:
>> On 23.02.2023 13:45, Tom de Vries via Binutils wrote:
>>> Gas has an option --compress-debug-sections that allows it to generate
>>> compressed debug sections.
>>>
>>> That does not guarantee that the debug sections are in fact compressed:
>>> ...
>>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
>>> $ readelf -S -W hello.o | grep " .debug"
>>> [ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
>>> [11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
>>> [12] .debug_info PROGBITS 000120 000039 00 0 0 1
>>> [14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
>>> [15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
>>> [17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
>>> ...
>>>
>>> Sensibly so, they're only compressed if that provides a size benefit.
>>>
>>> However, for the purposes of testing components consuming dwarf
>>> we may want the sections to be compressed regardless.
>>>
>>> Add a new option --force-compress-debug-sections that ignores the size
>>> heuristic, such that we have instead:
>>> ...
>>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd \
>>> -Wa,--force-compress-debug-sections
>>> $ readelf -S -W hello.o | grep " .debug"
>>> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
>>> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
>>> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
>>> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
>>> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
>>> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>>> ...
>>>
>>> Advertised as:
>>> ...
>>> $ as --help 2>&1 | grep compress
>>> --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]
>>> compress DWARF debug sections
>>> --nocompress-debug-sections
>>> don't compress DWARF debug sections
>>> --force-compress-debug-sections
>>> force compression of DWARF debug sections
>>
>> No objection in principle, but have you considered making this a new
>> sub-option to --compress-debug-sections, i.e. compress-debug-sections=force?
>
> I did consider adding a "force-" prefix variant for all the non-none
> sub-options, but decided to go with the simplest solution first.
>
> Your suggestion, --compress-debug-sections=force is more orthogonal,
> though it breaks the pattern that all the sub-options are mutually
> exclusive.
>
> We could have it be standalone, so you'd do:
> --compress-debug-sections=zstd --compress-debug-sections=force.
>
> Or instead combined: --compress-debug-sections=force,zstd. Harder to
> parse though, I suppose.
I think both should be allowed. In a complex build system it may be
different entities setting "how" and "whether". (To me "none" falls in
the "whether" category together with "force", and it also can be seen
as falling in the "how" category together with "zlib" etc. In Linux
Kconfig, for example, I'd see this being expressed as first a "whether"
choice [yes/maybe/forced] and then a "how" choice dependent upon
"whether != none".)
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-23 13:27 ` Tom de Vries
2023-02-23 13:44 ` Jan Beulich
@ 2023-02-23 15:23 ` Michael Matz
2023-02-23 15:28 ` Tom de Vries
1 sibling, 1 reply; 24+ messages in thread
From: Michael Matz @ 2023-02-23 15:23 UTC (permalink / raw)
To: Tom de Vries; +Cc: Jan Beulich, binutils
Hello,
On Thu, 23 Feb 2023, Tom de Vries via Binutils wrote:
> Or instead combined: --compress-debug-sections=force,zstd. Harder to parse
> though, I suppose.
>
> I guess this last one would be my preference, because it makes it clear force
> is in a different category than zlib/zstd.
Regardless of the outcome how to spell this on the cmdline, can the
force-mode please be explicitely documented as doing stuff "even if it
increases section size", at least in the .info docu (not necessarily in
--help output). Because, knowing users trying random options, they might
be tempted to use the force-mode, because "it surely will be better" and
the remark would hopefully deflect this.
Ciao,
Michael.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-23 15:23 ` Michael Matz
@ 2023-02-23 15:28 ` Tom de Vries
2023-02-23 15:44 ` Michael Matz
0 siblings, 1 reply; 24+ messages in thread
From: Tom de Vries @ 2023-02-23 15:28 UTC (permalink / raw)
To: Michael Matz; +Cc: Jan Beulich, binutils
On 2/23/23 16:23, Michael Matz wrote:
> Hello,
>
> On Thu, 23 Feb 2023, Tom de Vries via Binutils wrote:
>
>> Or instead combined: --compress-debug-sections=force,zstd. Harder to parse
>> though, I suppose.
>>
>> I guess this last one would be my preference, because it makes it clear force
>> is in a different category than zlib/zstd.
>
> Regardless of the outcome how to spell this on the cmdline, can the
> force-mode please be explicitely documented as doing stuff "even if it
> increases section size", at least in the .info docu (not necessarily in
> --help output). Because, knowing users trying random options, they might
> be tempted to use the force-mode, because "it surely will be better" and
> the remark would hopefully deflect this.
>
The submitted version (
https://sourceware.org/pipermail/binutils/2023-February/126284.html )
contains:
...
@@ -718,7 +719,8 @@ Begin in alternate macro mode.
Compress DWARF debug sections using zlib with SHF_COMPRESSED from the
ELF ABI. The resulting object file may not be compatible with older
linkers and object file utilities. Note if compression would make a
-given section @emph{larger} then it is not compressed.
+given section @emph{larger} then it is not compressed, unless
+@option{--force-compress-debug-section} is used.
@ifset ELF
@cindex @samp{--compress-debug-sections=} option
@@ -738,7 +740,11 @@ using the obsoleted zlib-gnu format. The debug
sections are renamed to begin
with @samp{.zdebug}.
@option{--compress-debug-sections=zstd} compresses DWARF debug
sections using zstd. Note - if compression would actually make a section
-@emph{larger}, then it is not compressed nor renamed.
+@emph{larger}, then it is not compressed nor renamed, unless
+@option{--force-compress-debug-section} is used.
+
+@item --force-compress-debug-sections
+Compress DWARF debug sections, even if this does not reduce size.
@end ifset
...
So, do you consider this insufficient?
Thanks,
- Tom
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-23 15:28 ` Tom de Vries
@ 2023-02-23 15:44 ` Michael Matz
2023-02-23 15:46 ` Tom de Vries
0 siblings, 1 reply; 24+ messages in thread
From: Michael Matz @ 2023-02-23 15:44 UTC (permalink / raw)
To: Tom de Vries; +Cc: Jan Beulich, binutils
Hey Tom,
On Thu, 23 Feb 2023, Tom de Vries wrote:
> The submitted version (
> https://sourceware.org/pipermail/binutils/2023-February/126284.html )
> contains:
> ...
> @@ -718,7 +719,8 @@ Begin in alternate macro mode.
> Compress DWARF debug sections using zlib with SHF_COMPRESSED from the
> ELF ABI. The resulting object file may not be compatible with older
> linkers and object file utilities. Note if compression would make a
> -given section @emph{larger} then it is not compressed.
> +given section @emph{larger} then it is not compressed, unless
> +@option{--force-compress-debug-section} is used.
Gah! I overlooked this snippet and the one below that.
> So, do you consider this insufficient?
No, it's completely fine IMHO :)
Ciao,
Michael.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-23 15:44 ` Michael Matz
@ 2023-02-23 15:46 ` Tom de Vries
0 siblings, 0 replies; 24+ messages in thread
From: Tom de Vries @ 2023-02-23 15:46 UTC (permalink / raw)
To: Michael Matz; +Cc: Jan Beulich, binutils
On 2/23/23 16:44, Michael Matz wrote:
> Hey Tom,
>
> On Thu, 23 Feb 2023, Tom de Vries wrote:
>
>> The submitted version (
>> https://sourceware.org/pipermail/binutils/2023-February/126284.html )
>> contains:
>> ...
>> @@ -718,7 +719,8 @@ Begin in alternate macro mode.
>> Compress DWARF debug sections using zlib with SHF_COMPRESSED from the
>> ELF ABI. The resulting object file may not be compatible with older
>> linkers and object file utilities. Note if compression would make a
>> -given section @emph{larger} then it is not compressed.
>> +given section @emph{larger} then it is not compressed, unless
>> +@option{--force-compress-debug-section} is used.
>
> Gah! I overlooked this snippet and the one below that.
>
>> So, do you consider this insufficient?
>
> No, it's completely fine IMHO :)
Cool, thanks :)
- Tom
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-23 13:44 ` Jan Beulich
@ 2023-02-24 10:52 ` Tom de Vries
2023-02-24 11:28 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: Tom de Vries @ 2023-02-24 10:52 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils, Michael Matz
[-- Attachment #1: Type: text/plain, Size: 3597 bytes --]
On 2/23/23 14:44, Jan Beulich wrote:
> On 23.02.2023 14:27, Tom de Vries wrote:
>> On 2/23/23 14:08, Jan Beulich wrote:
>>> On 23.02.2023 13:45, Tom de Vries via Binutils wrote:
>>>> Gas has an option --compress-debug-sections that allows it to generate
>>>> compressed debug sections.
>>>>
>>>> That does not guarantee that the debug sections are in fact compressed:
>>>> ...
>>>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
>>>> $ readelf -S -W hello.o | grep " .debug"
>>>> [ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
>>>> [11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
>>>> [12] .debug_info PROGBITS 000120 000039 00 0 0 1
>>>> [14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
>>>> [15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
>>>> [17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
>>>> ...
>>>>
>>>> Sensibly so, they're only compressed if that provides a size benefit.
>>>>
>>>> However, for the purposes of testing components consuming dwarf
>>>> we may want the sections to be compressed regardless.
>>>>
>>>> Add a new option --force-compress-debug-sections that ignores the size
>>>> heuristic, such that we have instead:
>>>> ...
>>>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd \
>>>> -Wa,--force-compress-debug-sections
>>>> $ readelf -S -W hello.o | grep " .debug"
>>>> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
>>>> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
>>>> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
>>>> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
>>>> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
>>>> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>>>> ...
>>>>
>>>> Advertised as:
>>>> ...
>>>> $ as --help 2>&1 | grep compress
>>>> --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]
>>>> compress DWARF debug sections
>>>> --nocompress-debug-sections
>>>> don't compress DWARF debug sections
>>>> --force-compress-debug-sections
>>>> force compression of DWARF debug sections
>>>
>>> No objection in principle, but have you considered making this a new
>>> sub-option to --compress-debug-sections, i.e. compress-debug-sections=force?
>>
>> I did consider adding a "force-" prefix variant for all the non-none
>> sub-options, but decided to go with the simplest solution first.
>>
>> Your suggestion, --compress-debug-sections=force is more orthogonal,
>> though it breaks the pattern that all the sub-options are mutually
>> exclusive.
>>
>> We could have it be standalone, so you'd do:
>> --compress-debug-sections=zstd --compress-debug-sections=force.
>>
>> Or instead combined: --compress-debug-sections=force,zstd. Harder to
>> parse though, I suppose.
>
> I think both should be allowed. In a complex build system it may be
> different entities setting "how" and "whether". (To me "none" falls in
> the "whether" category together with "force", and it also can be seen
> as falling in the "how" category together with "zlib" etc. In Linux
> Kconfig, for example, I'd see this being expressed as first a "whether"
> choice [yes/maybe/forced] and then a "how" choice dependent upon
> "whether != none".)
>
I gave this approach a try.
Thanks,
- Tom
[-- Attachment #2: 0001-gas-Add-compress-debug-sections-force.patch --]
[-- Type: text/x-patch, Size: 11188 bytes --]
From 792f364b885126b83ad50619085426aca1bc308b Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Thu, 23 Feb 2023 12:53:40 +0100
Subject: [PATCH] gas: Add --compress-debug-sections=force
Gas has an option --compress-debug-sections that allows it to generate
compressed debug sections.
That does not guarantee that the debug sections are in fact compressed:
...
$ gcc hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
[11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
[12] .debug_info PROGBITS 000120 000039 00 0 0 1
[14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
[15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
[17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
...
Sensibly so, they're only compressed if that provides a size benefit.
However, for the purpose of testing components consuming dwarf
we may want the sections to be compressed regardless.
Add a new suboption --compress-debug-sections=force that ignores the size
heuristic, such that we have instead:
...
$ gcc hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd+force
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
[11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
[12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
[14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
[15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
[17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
...
Advertised as:
...
$ as --help
...
--compress-debug-sections[={none|<format>|force|force+<format>}]
where <format> is {zlib|zlib-gnu|zlib-gabi|zstd}
compress DWARF debug sections
Default: zstd
...
Tested on x86_64-linux.
---
gas/as.c | 158 ++++++++++++++++++++++++++++++++++++++++++------
gas/as.h | 4 ++
gas/doc/as.texi | 10 ++-
gas/write.c | 4 +-
4 files changed, 153 insertions(+), 23 deletions(-)
diff --git a/gas/as.c b/gas/as.c
index 598bfd56cf5..3c38b7a45e9 100644
--- a/gas/as.c
+++ b/gas/as.c
@@ -230,6 +230,18 @@ enum compressed_debug_section_type flag_compress_debug
= DEFAULT_COMPRESSED_DEBUG_ALGORITHM;
#endif
+enum compress_debug_action
+{
+ cda_default,
+ cda_none,
+ cda_force,
+ cda_yes,
+};
+static enum compress_debug_action compress_debug_action
+ = cda_default;
+
+bool force_compress_debug = false;
+
static void
show_usage (FILE * stream)
{
@@ -252,7 +264,8 @@ Options:\n\
fprintf (stream, _("\
--alternate initially turn on alternate macro syntax\n"));
fprintf (stream, _("\
- --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]\n\
+ --compress-debug-sections[={none|<format>|force|force+<format>}]\n\
+ where <format> is {zlib|zlib-gnu|zlib-gabi|zstd}\n\
compress DWARF debug sections\n")),
fprintf (stream, _("\
Default: %s\n"),
@@ -418,6 +431,126 @@ Options:\n\
fprintf (stream, _("Report bugs to %s\n"), REPORT_BUGS_TO);
}
+static void
+parse_compress_debug_optarg_1 (const char *optarg, unsigned int *none_cnt,
+ unsigned int *force_cnt,
+ unsigned int *format_cnt,
+ enum compressed_debug_section_type *format)
+{
+ gas_assert (optarg != NULL);
+
+ if (strcmp (optarg, "force") == 0)
+ {
+ ++*force_cnt;
+ return;
+ }
+
+ enum compressed_debug_section_type tmp
+ = bfd_get_compression_algorithm (optarg);
+
+#ifndef HAVE_ZSTD
+ if (tmp == COMPRESS_DEBUG_ZSTD)
+ as_fatal (_ ("--compress-debug-sections=zstd: gas is not "
+ "built with zstd support"));
+#endif
+
+ if (tmp == COMPRESS_UNKNOWN)
+ as_fatal (_("Invalid --compress-debug-sections option: `%s'"),
+ optarg);
+
+ if (tmp == COMPRESS_DEBUG_NONE)
+ {
+ ++*none_cnt;
+ return;
+ }
+
+ *format = tmp;
+ ++*format_cnt;
+}
+
+static void
+parse_compress_debug_optarg (const char *optarg)
+{
+ const char *orig_optarg = optarg;
+
+#if !defined OBJ_ELF && !defined OBJ_MAYBE_ELF
+ as_fatal (_("--compress-debug-sections=%s is unsupported"),
+ optarg);
+#endif
+
+ /* Tokenize subopts seperated by '+' and pass to
+ parse_compress_debug_optarg_1. */
+ unsigned int none_cnt = 0;
+ unsigned int force_cnt = 0;
+ unsigned int format_cnt = 0;
+ enum compressed_debug_section_type format = COMPRESS_UNKNOWN;
+ while (true)
+ {
+ const char *idx = optarg;
+ while (*idx != '\0' && *idx != '+')
+ idx++;
+
+ size_t len = idx - optarg;
+ if (len == 0)
+ {
+ /* Generate error. */
+ parse_compress_debug_optarg_1 ("", NULL, NULL, NULL, NULL);
+ break;
+ }
+
+ char *tmp = xstrndup (optarg, len);
+ parse_compress_debug_optarg_1 (tmp, &none_cnt, &force_cnt, &format_cnt,
+ &format);
+ free (tmp);
+
+ if (*idx == '\0')
+ break;
+
+ /* Step over '+' and continue tokenizing. */
+ gas_assert (*idx == '+');
+ optarg = idx + 1;
+ }
+
+ bool valid_p = true;
+ if (force_cnt >= 2 || none_cnt >= 2 || format_cnt >= 2)
+ /* Not allowed: force+force, none+none, zstd+zstd, zstd+zlib. */
+ valid_p = false;
+ else if (none_cnt == 1 && (force_cnt > 0 || format_cnt > 0))
+ /* Not allowed: none+force, none+zstd. */
+ valid_p = false;
+
+ if (!valid_p)
+ as_fatal (_("Invalid --compress-debug-sections option combination: `%s'"),
+ orig_optarg);
+
+ if (none_cnt == 1)
+ /* Let --compress-debug-section=none override
+ --compress-debug-section and --compress-debug-section=force. */
+ compress_debug_action = cda_none;
+ else if (force_cnt == 1)
+ /* Let --compress-debug-section=force override
+ --compress-debug-section and --compress-debug-section=none. */
+ compress_debug_action = cda_force;
+ else
+ /* Let --compress-debug-section override
+ --compress-debug-section=none and --compress-debug-section=force. */
+ compress_debug_action = cda_yes;
+
+ if (format_cnt == 1)
+ /* Let --compress-debug-section=zlib override
+ --compress-debug-section=zstd. */
+ flag_compress_debug = format;
+}
+
+static void
+finalize_parse_compress_debug_optarg (void)
+{
+ if (compress_debug_action == cda_none)
+ flag_compress_debug = COMPRESS_DEBUG_NONE;
+ else if (compress_debug_action == cda_force)
+ force_compress_debug = true;
+}
+
/* Since it is easy to do here we interpret the special arg "-"
to mean "use stdin" and we set that argv[] pointing to "".
After we have munged argv[], the only things left are source file
@@ -747,28 +880,13 @@ This program has absolutely no warranty.\n"));
case OPTION_COMPRESS_DEBUG:
if (optarg)
- {
-#if defined OBJ_ELF || defined OBJ_MAYBE_ELF
- flag_compress_debug = bfd_get_compression_algorithm (optarg);
-#ifndef HAVE_ZSTD
- if (flag_compress_debug == COMPRESS_DEBUG_ZSTD)
- as_fatal (_ ("--compress-debug-sections=zstd: gas is not "
- "built with zstd support"));
-#endif
- if (flag_compress_debug == COMPRESS_UNKNOWN)
- as_fatal (_("Invalid --compress-debug-sections option: `%s'"),
- optarg);
-#else
- as_fatal (_("--compress-debug-sections=%s is unsupported"),
- optarg);
-#endif
- }
+ parse_compress_debug_optarg (optarg);
else
- flag_compress_debug = COMPRESS_DEBUG_GABI_ZLIB;
+ parse_compress_debug_optarg ("zlib-gabi");
break;
case OPTION_NOCOMPRESS_DEBUG:
- flag_compress_debug = COMPRESS_DEBUG_NONE;
+ parse_compress_debug_optarg ("none");
break;
case OPTION_DEBUG_PREFIX_MAP:
@@ -1136,6 +1254,8 @@ This program has absolutely no warranty.\n"));
*pargc = new_argc;
*pargv = new_argv;
+ finalize_parse_compress_debug_optarg ();
+
#ifdef md_after_parse_args
md_after_parse_args ();
#endif
diff --git a/gas/as.h b/gas/as.h
index 4c5fa9ecf7d..115af019815 100644
--- a/gas/as.h
+++ b/gas/as.h
@@ -331,6 +331,10 @@ COMMON int flag_traditional_format;
/* Type of compressed debug sections we should generate. */
COMMON enum compressed_debug_section_type flag_compress_debug;
+/* True if we want to generate compressed debug sections, even if it
+ doesn't make them smaller. */
+COMMON bool force_compress_debug;
+
/* TRUE if .note.GNU-stack section with SEC_CODE should be created */
COMMON int flag_execstack;
diff --git a/gas/doc/as.texi b/gas/doc/as.texi
index bbdfa4bfdca..d25559141c7 100644
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -718,7 +718,8 @@ Begin in alternate macro mode.
Compress DWARF debug sections using zlib with SHF_COMPRESSED from the
ELF ABI. The resulting object file may not be compatible with older
linkers and object file utilities. Note if compression would make a
-given section @emph{larger} then it is not compressed.
+given section @emph{larger} then it is not compressed, unless
+@option{--compress-debug-section=force} is used.
@ifset ELF
@cindex @samp{--compress-debug-sections=} option
@@ -727,6 +728,7 @@ given section @emph{larger} then it is not compressed.
@itemx --compress-debug-sections=zlib-gnu
@itemx --compress-debug-sections=zlib-gabi
@itemx --compress-debug-sections=zstd
+@itemx --compress-debug-sections=force
These options control how DWARF debug sections are compressed.
@option{--compress-debug-sections=none} is equivalent to
@option{--nocompress-debug-sections}.
@@ -738,7 +740,11 @@ using the obsoleted zlib-gnu format. The debug sections are renamed to begin
with @samp{.zdebug}.
@option{--compress-debug-sections=zstd} compresses DWARF debug
sections using zstd. Note - if compression would actually make a section
-@emph{larger}, then it is not compressed nor renamed.
+@emph{larger}, then it is not compressed nor renamed, unless
+@option{--compress-debug-section=force} is used.
+@option{--compress-debug-sections=force} compresses DWARF debug sections,
+even if this does not reduce size. It can be used in conjunction with a format
+selection, for instance @option{--compress-debug-section=zstd+force}.
@end ifset
diff --git a/gas/write.c b/gas/write.c
index 8273b7a42f1..39bcea23fac 100644
--- a/gas/write.c
+++ b/gas/write.c
@@ -1465,7 +1465,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
flagword flags = bfd_section_flags (sec);
if (seginfo == NULL
- || uncompressed_size < 32
+ || (!force_compress_debug && uncompressed_size < 32)
|| (flags & SEC_HAS_CONTENTS) == 0)
return;
@@ -1582,7 +1582,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
/* PR binutils/18087: If compression didn't make the section smaller,
just keep it uncompressed. */
- if (compressed_size >= uncompressed_size)
+ if (!force_compress_debug && compressed_size >= uncompressed_size)
return;
/* Replace the uncompressed frag list with the compressed frag list. */
--
2.35.3
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-24 10:52 ` Tom de Vries
@ 2023-02-24 11:28 ` Jan Beulich
2023-02-24 12:21 ` Tom de Vries
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2023-02-24 11:28 UTC (permalink / raw)
To: Tom de Vries; +Cc: binutils, Michael Matz
On 24.02.2023 11:52, Tom de Vries wrote:
> On 2/23/23 14:44, Jan Beulich wrote:
>> On 23.02.2023 14:27, Tom de Vries wrote:
>>> On 2/23/23 14:08, Jan Beulich wrote:
>>>> On 23.02.2023 13:45, Tom de Vries via Binutils wrote:
>>>>> Gas has an option --compress-debug-sections that allows it to generate
>>>>> compressed debug sections.
>>>>>
>>>>> That does not guarantee that the debug sections are in fact compressed:
>>>>> ...
>>>>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
>>>>> $ readelf -S -W hello.o | grep " .debug"
>>>>> [ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
>>>>> [11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
>>>>> [12] .debug_info PROGBITS 000120 000039 00 0 0 1
>>>>> [14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
>>>>> [15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
>>>>> [17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
>>>>> ...
>>>>>
>>>>> Sensibly so, they're only compressed if that provides a size benefit.
>>>>>
>>>>> However, for the purposes of testing components consuming dwarf
>>>>> we may want the sections to be compressed regardless.
>>>>>
>>>>> Add a new option --force-compress-debug-sections that ignores the size
>>>>> heuristic, such that we have instead:
>>>>> ...
>>>>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd \
>>>>> -Wa,--force-compress-debug-sections
>>>>> $ readelf -S -W hello.o | grep " .debug"
>>>>> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
>>>>> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
>>>>> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
>>>>> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
>>>>> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
>>>>> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>>>>> ...
>>>>>
>>>>> Advertised as:
>>>>> ...
>>>>> $ as --help 2>&1 | grep compress
>>>>> --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]
>>>>> compress DWARF debug sections
>>>>> --nocompress-debug-sections
>>>>> don't compress DWARF debug sections
>>>>> --force-compress-debug-sections
>>>>> force compression of DWARF debug sections
>>>>
>>>> No objection in principle, but have you considered making this a new
>>>> sub-option to --compress-debug-sections, i.e. compress-debug-sections=force?
>>>
>>> I did consider adding a "force-" prefix variant for all the non-none
>>> sub-options, but decided to go with the simplest solution first.
>>>
>>> Your suggestion, --compress-debug-sections=force is more orthogonal,
>>> though it breaks the pattern that all the sub-options are mutually
>>> exclusive.
>>>
>>> We could have it be standalone, so you'd do:
>>> --compress-debug-sections=zstd --compress-debug-sections=force.
>>>
>>> Or instead combined: --compress-debug-sections=force,zstd. Harder to
>>> parse though, I suppose.
>>
>> I think both should be allowed. In a complex build system it may be
>> different entities setting "how" and "whether". (To me "none" falls in
>> the "whether" category together with "force", and it also can be seen
>> as falling in the "how" category together with "zlib" etc. In Linux
>> Kconfig, for example, I'd see this being expressed as first a "whether"
>> choice [yes/maybe/forced] and then a "how" choice dependent upon
>> "whether != none".)
>>
>
> I gave this approach a try.
Any specific reason you chose + as the separator instead of the more
conventional , ? I also wouldn't see anything wrong with something
like "...=force,zstd,none" - the last one(s) win. That's no different
from specifying a second instance of the option. And without that it
looks as if the parsing would end up simpler.
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-24 11:28 ` Jan Beulich
@ 2023-02-24 12:21 ` Tom de Vries
2023-02-24 13:23 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: Tom de Vries @ 2023-02-24 12:21 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils, Michael Matz
[-- Attachment #1: Type: text/plain, Size: 4531 bytes --]
On 2/24/23 12:28, Jan Beulich wrote:
> On 24.02.2023 11:52, Tom de Vries wrote:
>> On 2/23/23 14:44, Jan Beulich wrote:
>>> On 23.02.2023 14:27, Tom de Vries wrote:
>>>> On 2/23/23 14:08, Jan Beulich wrote:
>>>>> On 23.02.2023 13:45, Tom de Vries via Binutils wrote:
>>>>>> Gas has an option --compress-debug-sections that allows it to generate
>>>>>> compressed debug sections.
>>>>>>
>>>>>> That does not guarantee that the debug sections are in fact compressed:
>>>>>> ...
>>>>>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
>>>>>> $ readelf -S -W hello.o | grep " .debug"
>>>>>> [ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
>>>>>> [11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
>>>>>> [12] .debug_info PROGBITS 000120 000039 00 0 0 1
>>>>>> [14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
>>>>>> [15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
>>>>>> [17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
>>>>>> ...
>>>>>>
>>>>>> Sensibly so, they're only compressed if that provides a size benefit.
>>>>>>
>>>>>> However, for the purposes of testing components consuming dwarf
>>>>>> we may want the sections to be compressed regardless.
>>>>>>
>>>>>> Add a new option --force-compress-debug-sections that ignores the size
>>>>>> heuristic, such that we have instead:
>>>>>> ...
>>>>>> $ gcc ~/hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd \
>>>>>> -Wa,--force-compress-debug-sections
>>>>>> $ readelf -S -W hello.o | grep " .debug"
>>>>>> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
>>>>>> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
>>>>>> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
>>>>>> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
>>>>>> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
>>>>>> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>>>>>> ...
>>>>>>
>>>>>> Advertised as:
>>>>>> ...
>>>>>> $ as --help 2>&1 | grep compress
>>>>>> --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]
>>>>>> compress DWARF debug sections
>>>>>> --nocompress-debug-sections
>>>>>> don't compress DWARF debug sections
>>>>>> --force-compress-debug-sections
>>>>>> force compression of DWARF debug sections
>>>>>
>>>>> No objection in principle, but have you considered making this a new
>>>>> sub-option to --compress-debug-sections, i.e. compress-debug-sections=force?
>>>>
>>>> I did consider adding a "force-" prefix variant for all the non-none
>>>> sub-options, but decided to go with the simplest solution first.
>>>>
>>>> Your suggestion, --compress-debug-sections=force is more orthogonal,
>>>> though it breaks the pattern that all the sub-options are mutually
>>>> exclusive.
>>>>
>>>> We could have it be standalone, so you'd do:
>>>> --compress-debug-sections=zstd --compress-debug-sections=force.
>>>>
>>>> Or instead combined: --compress-debug-sections=force,zstd. Harder to
>>>> parse though, I suppose.
>>>
>>> I think both should be allowed. In a complex build system it may be
>>> different entities setting "how" and "whether". (To me "none" falls in
>>> the "whether" category together with "force", and it also can be seen
>>> as falling in the "how" category together with "zlib" etc. In Linux
>>> Kconfig, for example, I'd see this being expressed as first a "whether"
>>> choice [yes/maybe/forced] and then a "how" choice dependent upon
>>> "whether != none".)
>>>
>>
>> I gave this approach a try.
>
> Any specific reason you chose + as the separator instead of the more
> conventional , ?
Yes, I initially went for ',', but ran into:
...
$ gcc ~/hello.c -Wa,-gdwarf-5 \
-Wa,--compress-debug-sections=zstd,force -c -v
...
as -v --64 -gdwarf-5 --compress-debug-sections=zstd force -o hello.o \
/tmp/ccOUMqHL.s
...
Assembler messages:
Error: can't open force for reading: No such file or directory
...
> I also wouldn't see anything wrong with something
> like "...=force,zstd,none" - the last one(s) win. That's no different
> from specifying a second instance of the option. And without that it
> looks as if the parsing would end up simpler.
OK, gave that a try.
Thanks,
- Tom
[-- Attachment #2: 0001-gas-Add-compress-debug-sections-force.patch --]
[-- Type: text/x-patch, Size: 10115 bytes --]
From f23c41372cb7d48116ce51f99b9f265248b2d7d4 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Thu, 23 Feb 2023 12:53:40 +0100
Subject: [PATCH] gas: Add --compress-debug-sections=force
Gas has an option --compress-debug-sections that allows it to generate
compressed debug sections.
That does not guarantee that the debug sections are in fact compressed:
...
$ gcc hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
[11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
[12] .debug_info PROGBITS 000120 000039 00 0 0 1
[14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
[15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
[17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
...
Sensibly so, they're only compressed if that provides a size benefit.
However, for the purpose of testing components consuming dwarf
we may want the sections to be compressed regardless.
Add a new suboption --compress-debug-sections=force that ignores the size
heuristic, such that we have instead:
...
$ gcc hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd+force
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
[11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
[12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
[14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
[15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
[17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
...
Advertised as:
...
$ as --help
...
--compress-debug-sections[={none|<format>|force|force+<format>}]
where <format> is {zlib|zlib-gnu|zlib-gabi|zstd}
compress DWARF debug sections
Default: zstd
...
Tested on x86_64-linux.
---
gas/as.c | 133 +++++++++++++++++++++++++++++++++++++++++-------
gas/as.h | 4 ++
gas/doc/as.texi | 10 +++-
gas/write.c | 4 +-
4 files changed, 128 insertions(+), 23 deletions(-)
diff --git a/gas/as.c b/gas/as.c
index 598bfd56cf5..0f4d43f256d 100644
--- a/gas/as.c
+++ b/gas/as.c
@@ -230,6 +230,18 @@ enum compressed_debug_section_type flag_compress_debug
= DEFAULT_COMPRESSED_DEBUG_ALGORITHM;
#endif
+enum compress_debug_action
+{
+ cda_default,
+ cda_none,
+ cda_force,
+ cda_yes,
+};
+static enum compress_debug_action compress_debug_action
+ = cda_default;
+
+bool force_compress_debug = false;
+
static void
show_usage (FILE * stream)
{
@@ -252,7 +264,8 @@ Options:\n\
fprintf (stream, _("\
--alternate initially turn on alternate macro syntax\n"));
fprintf (stream, _("\
- --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]\n\
+ --compress-debug-sections[={none|<format>|force|force+<format>}]\n\
+ where <format> is {zlib|zlib-gnu|zlib-gabi|zstd}\n\
compress DWARF debug sections\n")),
fprintf (stream, _("\
Default: %s\n"),
@@ -418,6 +431,101 @@ Options:\n\
fprintf (stream, _("Report bugs to %s\n"), REPORT_BUGS_TO);
}
+static void
+parse_compress_debug_optarg_1 (const char *optarg, bool *none, bool *force,
+ enum compressed_debug_section_type *format)
+{
+ gas_assert (optarg != NULL);
+
+ if (strcmp (optarg, "force") == 0)
+ {
+ *force = true;
+ *none = false;
+ return;
+ }
+
+ enum compressed_debug_section_type tmp
+ = bfd_get_compression_algorithm (optarg);
+
+#ifndef HAVE_ZSTD
+ if (tmp == COMPRESS_DEBUG_ZSTD)
+ as_fatal (_ ("--compress-debug-sections=zstd: gas is not "
+ "built with zstd support"));
+#endif
+
+ if (tmp == COMPRESS_UNKNOWN)
+ as_fatal (_("Invalid --compress-debug-sections option: `%s'"),
+ optarg);
+
+ if (tmp == COMPRESS_DEBUG_NONE)
+ {
+ *none = true;
+ *force = false;
+ return;
+ }
+
+ *format = tmp;
+}
+
+static void
+parse_compress_debug_optarg (const char *optarg)
+{
+#if !defined OBJ_ELF && !defined OBJ_MAYBE_ELF
+ as_fatal (_("--compress-debug-sections=%s is unsupported"),
+ optarg);
+#endif
+
+ /* Tokenize subopts seperated by '+' and pass to
+ parse_compress_debug_optarg_1. */
+ bool none = false;
+ bool force = false;
+ enum compressed_debug_section_type format = COMPRESS_UNKNOWN;
+ while (true)
+ {
+ const char *idx = optarg;
+ while (*idx != '\0' && *idx != '+')
+ idx++;
+
+ size_t len = idx - optarg;
+ if (len == 0)
+ {
+ /* Generate error. */
+ parse_compress_debug_optarg_1 ("", NULL, NULL, NULL);
+ break;
+ }
+
+ char *tmp = xstrndup (optarg, len);
+ parse_compress_debug_optarg_1 (tmp, &none, &force, &format);
+ free (tmp);
+
+ if (*idx == '\0')
+ break;
+
+ /* Step over '+' and continue tokenizing. */
+ gas_assert (*idx == '+');
+ optarg = idx + 1;
+ }
+
+ if (none)
+ compress_debug_action = cda_none;
+ else if (force)
+ compress_debug_action = cda_force;
+ else
+ compress_debug_action = cda_yes;
+
+ if (format != COMPRESS_UNKNOWN)
+ flag_compress_debug = format;
+}
+
+static void
+finalize_parse_compress_debug_optarg (void)
+{
+ if (compress_debug_action == cda_none)
+ flag_compress_debug = COMPRESS_DEBUG_NONE;
+ else if (compress_debug_action == cda_force)
+ force_compress_debug = true;
+}
+
/* Since it is easy to do here we interpret the special arg "-"
to mean "use stdin" and we set that argv[] pointing to "".
After we have munged argv[], the only things left are source file
@@ -747,28 +855,13 @@ This program has absolutely no warranty.\n"));
case OPTION_COMPRESS_DEBUG:
if (optarg)
- {
-#if defined OBJ_ELF || defined OBJ_MAYBE_ELF
- flag_compress_debug = bfd_get_compression_algorithm (optarg);
-#ifndef HAVE_ZSTD
- if (flag_compress_debug == COMPRESS_DEBUG_ZSTD)
- as_fatal (_ ("--compress-debug-sections=zstd: gas is not "
- "built with zstd support"));
-#endif
- if (flag_compress_debug == COMPRESS_UNKNOWN)
- as_fatal (_("Invalid --compress-debug-sections option: `%s'"),
- optarg);
-#else
- as_fatal (_("--compress-debug-sections=%s is unsupported"),
- optarg);
-#endif
- }
+ parse_compress_debug_optarg (optarg);
else
- flag_compress_debug = COMPRESS_DEBUG_GABI_ZLIB;
+ parse_compress_debug_optarg ("zlib-gabi");
break;
case OPTION_NOCOMPRESS_DEBUG:
- flag_compress_debug = COMPRESS_DEBUG_NONE;
+ parse_compress_debug_optarg ("none");
break;
case OPTION_DEBUG_PREFIX_MAP:
@@ -1136,6 +1229,8 @@ This program has absolutely no warranty.\n"));
*pargc = new_argc;
*pargv = new_argv;
+ finalize_parse_compress_debug_optarg ();
+
#ifdef md_after_parse_args
md_after_parse_args ();
#endif
diff --git a/gas/as.h b/gas/as.h
index 4c5fa9ecf7d..115af019815 100644
--- a/gas/as.h
+++ b/gas/as.h
@@ -331,6 +331,10 @@ COMMON int flag_traditional_format;
/* Type of compressed debug sections we should generate. */
COMMON enum compressed_debug_section_type flag_compress_debug;
+/* True if we want to generate compressed debug sections, even if it
+ doesn't make them smaller. */
+COMMON bool force_compress_debug;
+
/* TRUE if .note.GNU-stack section with SEC_CODE should be created */
COMMON int flag_execstack;
diff --git a/gas/doc/as.texi b/gas/doc/as.texi
index bbdfa4bfdca..d25559141c7 100644
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -718,7 +718,8 @@ Begin in alternate macro mode.
Compress DWARF debug sections using zlib with SHF_COMPRESSED from the
ELF ABI. The resulting object file may not be compatible with older
linkers and object file utilities. Note if compression would make a
-given section @emph{larger} then it is not compressed.
+given section @emph{larger} then it is not compressed, unless
+@option{--compress-debug-section=force} is used.
@ifset ELF
@cindex @samp{--compress-debug-sections=} option
@@ -727,6 +728,7 @@ given section @emph{larger} then it is not compressed.
@itemx --compress-debug-sections=zlib-gnu
@itemx --compress-debug-sections=zlib-gabi
@itemx --compress-debug-sections=zstd
+@itemx --compress-debug-sections=force
These options control how DWARF debug sections are compressed.
@option{--compress-debug-sections=none} is equivalent to
@option{--nocompress-debug-sections}.
@@ -738,7 +740,11 @@ using the obsoleted zlib-gnu format. The debug sections are renamed to begin
with @samp{.zdebug}.
@option{--compress-debug-sections=zstd} compresses DWARF debug
sections using zstd. Note - if compression would actually make a section
-@emph{larger}, then it is not compressed nor renamed.
+@emph{larger}, then it is not compressed nor renamed, unless
+@option{--compress-debug-section=force} is used.
+@option{--compress-debug-sections=force} compresses DWARF debug sections,
+even if this does not reduce size. It can be used in conjunction with a format
+selection, for instance @option{--compress-debug-section=zstd+force}.
@end ifset
diff --git a/gas/write.c b/gas/write.c
index 8273b7a42f1..39bcea23fac 100644
--- a/gas/write.c
+++ b/gas/write.c
@@ -1465,7 +1465,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
flagword flags = bfd_section_flags (sec);
if (seginfo == NULL
- || uncompressed_size < 32
+ || (!force_compress_debug && uncompressed_size < 32)
|| (flags & SEC_HAS_CONTENTS) == 0)
return;
@@ -1582,7 +1582,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
/* PR binutils/18087: If compression didn't make the section smaller,
just keep it uncompressed. */
- if (compressed_size >= uncompressed_size)
+ if (!force_compress_debug && compressed_size >= uncompressed_size)
return;
/* Replace the uncompressed frag list with the compressed frag list. */
--
2.35.3
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-24 12:21 ` Tom de Vries
@ 2023-02-24 13:23 ` Jan Beulich
2023-02-24 14:11 ` Tom de Vries
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2023-02-24 13:23 UTC (permalink / raw)
To: Tom de Vries; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 24.02.2023 13:21, Tom de Vries wrote:
> On 2/24/23 12:28, Jan Beulich wrote:
>> On 24.02.2023 11:52, Tom de Vries wrote:
>>> On 2/23/23 14:44, Jan Beulich wrote:
>>>> I think both should be allowed. In a complex build system it may be
>>>> different entities setting "how" and "whether". (To me "none" falls in
>>>> the "whether" category together with "force", and it also can be seen
>>>> as falling in the "how" category together with "zlib" etc. In Linux
>>>> Kconfig, for example, I'd see this being expressed as first a "whether"
>>>> choice [yes/maybe/forced] and then a "how" choice dependent upon
>>>> "whether != none".)
>>>>
>>>
>>> I gave this approach a try.
>>
>> Any specific reason you chose + as the separator instead of the more
>> conventional , ?
>
> Yes, I initially went for ',', but ran into:
> ...
> $ gcc ~/hello.c -Wa,-gdwarf-5 \
> -Wa,--compress-debug-sections=zstd,force -c -v
> ...
> as -v --64 -gdwarf-5 --compress-debug-sections=zstd force -o hello.o \
> /tmp/ccOUMqHL.s
> ...
> Assembler messages:
> Error: can't open force for reading: No such file or directory
> ...
Hmm. I have to admit that I'm not happy with +, irrespective of this
issue. I wonder what other maintainers think - Nick, Alan?
>> I also wouldn't see anything wrong with something
>> like "...=force,zstd,none" - the last one(s) win. That's no different
>> from specifying a second instance of the option. And without that it
>> looks as if the parsing would end up simpler.
>
> OK, gave that a try.
That's still accumulating none and force across the entire sequence
(and then giving none priority over force, no matter that force may
have been specified last), rather than handling things the same as
when multiple options are specified. With accumulation partially
removed parsing became less involved, but it can be yet more simple
when that accumulation is dropped.
In case of contention maybe best to not allow a sequence and hence
require (in certain cases) two instances of the option to be passed?
At the very least that's then easier to parse.
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-24 13:23 ` Jan Beulich
@ 2023-02-24 14:11 ` Tom de Vries
2023-02-24 14:26 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: Tom de Vries @ 2023-02-24 14:11 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 2/24/23 14:23, Jan Beulich wrote:
> On 24.02.2023 13:21, Tom de Vries wrote:
>> On 2/24/23 12:28, Jan Beulich wrote:
>>> On 24.02.2023 11:52, Tom de Vries wrote:
>>>> On 2/23/23 14:44, Jan Beulich wrote:
>>>>> I think both should be allowed. In a complex build system it may be
>>>>> different entities setting "how" and "whether". (To me "none" falls in
>>>>> the "whether" category together with "force", and it also can be seen
>>>>> as falling in the "how" category together with "zlib" etc. In Linux
>>>>> Kconfig, for example, I'd see this being expressed as first a "whether"
>>>>> choice [yes/maybe/forced] and then a "how" choice dependent upon
>>>>> "whether != none".)
>>>>>
>>>>
>>>> I gave this approach a try.
>>>
>>> Any specific reason you chose + as the separator instead of the more
>>> conventional , ?
>>
>> Yes, I initially went for ',', but ran into:
>> ...
>> $ gcc ~/hello.c -Wa,-gdwarf-5 \
>> -Wa,--compress-debug-sections=zstd,force -c -v
>> ...
>> as -v --64 -gdwarf-5 --compress-debug-sections=zstd force -o hello.o \
>> /tmp/ccOUMqHL.s
>> ...
>> Assembler messages:
>> Error: can't open force for reading: No such file or directory
>> ...
>
> Hmm. I have to admit that I'm not happy with +, irrespective of this
> issue. I wonder what other maintainers think - Nick, Alan?
>
AFAIU you're proposing to use "-Xassembler
--compress-debug-sections=zstd,force" in this case instead of -Wa.
>>> I also wouldn't see anything wrong with something
>>> like "...=force,zstd,none" - the last one(s) win. That's no different
>>> from specifying a second instance of the option. And without that it
>>> looks as if the parsing would end up simpler.
>>
>> OK, gave that a try.
>
> That's still accumulating none and force across the entire sequence
> (and then giving none priority over force, no matter that force may
> have been specified last),
Um, so you're saying that none+zstd+force is currently interpreted as none?
Lets try:
...
$ gcc ~/hello.c -c -Wa,-gdwarf-5 -Xassembler
--compress-debug-sections=none+zstd+force
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
[11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
[12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
[14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
[15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
[17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
...
So, that doesn't seem to be the case, compression is done, as expected.
Thanks,
- Tom
> rather than handling things the same as
> when multiple options are specified. With accumulation partially
> removed parsing became less involved, but it can be yet more simple
> when that accumulation is dropped.
>
> In case of contention maybe best to not allow a sequence and hence
> require (in certain cases) two instances of the option to be passed?
> At the very least that's then easier to parse.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-24 14:11 ` Tom de Vries
@ 2023-02-24 14:26 ` Jan Beulich
2023-02-24 14:57 ` Tom de Vries
0 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2023-02-24 14:26 UTC (permalink / raw)
To: Tom de Vries; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 24.02.2023 15:11, Tom de Vries wrote:
> On 2/24/23 14:23, Jan Beulich wrote:
>> On 24.02.2023 13:21, Tom de Vries wrote:
>>> On 2/24/23 12:28, Jan Beulich wrote:
>>>> I also wouldn't see anything wrong with something
>>>> like "...=force,zstd,none" - the last one(s) win. That's no different
>>>> from specifying a second instance of the option. And without that it
>>>> looks as if the parsing would end up simpler.
>>>
>>> OK, gave that a try.
>>
>> That's still accumulating none and force across the entire sequence
>> (and then giving none priority over force, no matter that force may
>> have been specified last),
>
> Um, so you're saying that none+zstd+force is currently interpreted as none?
>
> Lets try:
> ...
> $ gcc ~/hello.c -c -Wa,-gdwarf-5 -Xassembler
> --compress-debug-sections=none+zstd+force
> $ readelf -S -W hello.o | grep " .debug"
> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>
> ...
>
> So, that doesn't seem to be the case, compression is done, as expected.
Oh, I've overlooked that you explicitly clear *none when you set *force
(my attention was mainly on the bottom of parse_compress_debug_optarg()).
I think that's more involved than necessary (possibly merely a result of
you having worked incrementally from your earlier version), and less
obviously doing the same as would happen when multiple separate options
were parsed.
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-24 14:26 ` Jan Beulich
@ 2023-02-24 14:57 ` Tom de Vries
2023-02-27 9:03 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: Tom de Vries @ 2023-02-24 14:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]
On 2/24/23 15:26, Jan Beulich wrote:
> On 24.02.2023 15:11, Tom de Vries wrote:
>> On 2/24/23 14:23, Jan Beulich wrote:
>>> On 24.02.2023 13:21, Tom de Vries wrote:
>>>> On 2/24/23 12:28, Jan Beulich wrote:
>>>>> I also wouldn't see anything wrong with something
>>>>> like "...=force,zstd,none" - the last one(s) win. That's no different
>>>>> from specifying a second instance of the option. And without that it
>>>>> looks as if the parsing would end up simpler.
>>>>
>>>> OK, gave that a try.
>>>
>>> That's still accumulating none and force across the entire sequence
>>> (and then giving none priority over force, no matter that force may
>>> have been specified last),
>>
>> Um, so you're saying that none+zstd+force is currently interpreted as none?
>>
>> Lets try:
>> ...
>> $ gcc ~/hello.c -c -Wa,-gdwarf-5 -Xassembler
>> --compress-debug-sections=none+zstd+force
>> $ readelf -S -W hello.o | grep " .debug"
>> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
>> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
>> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
>> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
>> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
>> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>>
>> ...
>>
>> So, that doesn't seem to be the case, compression is done, as expected.
>
> Oh, I've overlooked that you explicitly clear *none when you set *force
> (my attention was mainly on the bottom of parse_compress_debug_optarg()).
> I think that's more involved than necessary (possibly merely a result of
> you having worked incrementally from your earlier version), and less
> obviously doing the same as would happen when multiple separate options
> were parsed.
I've tried to simplify further.
Is this more how you want it?
Thanks,
- Tom
[-- Attachment #2: 0001-gas-Add-compress-debug-sections-force.patch --]
[-- Type: text/x-patch, Size: 9694 bytes --]
From ae0cdfec9c854d89cc87f88c496a3b6e60539115 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Thu, 23 Feb 2023 12:53:40 +0100
Subject: [PATCH] gas: Add --compress-debug-sections=force
Gas has an option --compress-debug-sections that allows it to generate
compressed debug sections.
That does not guarantee that the debug sections are in fact compressed:
...
$ gcc hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
[11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
[12] .debug_info PROGBITS 000120 000039 00 0 0 1
[14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
[15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
[17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
...
Sensibly so, they're only compressed if that provides a size benefit.
However, for the purpose of testing components consuming dwarf
we may want the sections to be compressed regardless.
Add a new suboption --compress-debug-sections=force that ignores the size
heuristic, such that we have instead:
...
$ gcc hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd+force
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
[11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
[12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
[14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
[15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
[17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
...
Advertised as:
...
$ as --help
...
--compress-debug-sections[={none|<format>|force|force+<format>}]
where <format> is {zlib|zlib-gnu|zlib-gabi|zstd}
compress DWARF debug sections
Default: zstd
...
Tested on x86_64-linux.
---
gas/as.c | 118 ++++++++++++++++++++++++++++++++++++++++--------
gas/as.h | 4 ++
gas/doc/as.texi | 10 +++-
gas/write.c | 4 +-
4 files changed, 113 insertions(+), 23 deletions(-)
diff --git a/gas/as.c b/gas/as.c
index 598bfd56cf5..3b2d286befd 100644
--- a/gas/as.c
+++ b/gas/as.c
@@ -230,6 +230,18 @@ enum compressed_debug_section_type flag_compress_debug
= DEFAULT_COMPRESSED_DEBUG_ALGORITHM;
#endif
+enum compress_debug_action
+{
+ cda_default,
+ cda_none,
+ cda_force,
+ cda_yes,
+};
+static enum compress_debug_action compress_debug_action
+ = cda_default;
+
+bool force_compress_debug = false;
+
static void
show_usage (FILE * stream)
{
@@ -252,7 +264,8 @@ Options:\n\
fprintf (stream, _("\
--alternate initially turn on alternate macro syntax\n"));
fprintf (stream, _("\
- --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]\n\
+ --compress-debug-sections[={none|<format>|force|force+<format>}]\n\
+ where <format> is {zlib|zlib-gnu|zlib-gabi|zstd}\n\
compress DWARF debug sections\n")),
fprintf (stream, _("\
Default: %s\n"),
@@ -418,6 +431,86 @@ Options:\n\
fprintf (stream, _("Report bugs to %s\n"), REPORT_BUGS_TO);
}
+static void
+parse_compress_debug_optarg_1 (const char *optarg)
+{
+ gas_assert (optarg != NULL);
+
+ if (strcmp (optarg, "force") == 0)
+ {
+ compress_debug_action = cda_force;
+ return;
+ }
+
+ enum compressed_debug_section_type tmp
+ = bfd_get_compression_algorithm (optarg);
+
+#ifndef HAVE_ZSTD
+ if (tmp == COMPRESS_DEBUG_ZSTD)
+ as_fatal (_ ("--compress-debug-sections=zstd: gas is not "
+ "built with zstd support"));
+#endif
+
+ if (tmp == COMPRESS_UNKNOWN)
+ as_fatal (_("Invalid --compress-debug-sections option: `%s'"),
+ optarg);
+
+ if (tmp == COMPRESS_DEBUG_NONE)
+ {
+ compress_debug_action = cda_none;
+ return;
+ }
+
+ compress_debug_action = cda_yes;
+ flag_compress_debug = tmp;
+}
+
+static void
+parse_compress_debug_optarg (const char *optarg)
+{
+#if !defined OBJ_ELF && !defined OBJ_MAYBE_ELF
+ as_fatal (_("--compress-debug-sections=%s is unsupported"),
+ optarg);
+#endif
+
+ /* Tokenize subopts seperated by '+' and pass to
+ parse_compress_debug_optarg_1. */
+ while (true)
+ {
+ const char *idx = optarg;
+ while (*idx != '\0' && *idx != '+')
+ idx++;
+
+ size_t len = idx - optarg;
+ if (len == 0)
+ {
+ /* Generate error. */
+ parse_compress_debug_optarg_1 ("");
+ break;
+ }
+
+ char *tmp = xstrndup (optarg, len);
+ parse_compress_debug_optarg_1 (tmp);
+ free (tmp);
+
+ if (*idx == '\0')
+ break;
+
+ /* Step over '+' and continue tokenizing. */
+ gas_assert (*idx == '+');
+ optarg = idx + 1;
+ }
+}
+
+static void
+finalize_parse_compress_debug_optarg (void)
+{
+ if (compress_debug_action == cda_none)
+ flag_compress_debug = COMPRESS_DEBUG_NONE;
+ else if (compress_debug_action == cda_force)
+ force_compress_debug = true;
+}
+
/* Since it is easy to do here we interpret the special arg "-"
to mean "use stdin" and we set that argv[] pointing to "".
After we have munged argv[], the only things left are source file
@@ -747,28 +840,13 @@ This program has absolutely no warranty.\n"));
case OPTION_COMPRESS_DEBUG:
if (optarg)
- {
-#if defined OBJ_ELF || defined OBJ_MAYBE_ELF
- flag_compress_debug = bfd_get_compression_algorithm (optarg);
-#ifndef HAVE_ZSTD
- if (flag_compress_debug == COMPRESS_DEBUG_ZSTD)
- as_fatal (_ ("--compress-debug-sections=zstd: gas is not "
- "built with zstd support"));
-#endif
- if (flag_compress_debug == COMPRESS_UNKNOWN)
- as_fatal (_("Invalid --compress-debug-sections option: `%s'"),
- optarg);
-#else
- as_fatal (_("--compress-debug-sections=%s is unsupported"),
- optarg);
-#endif
- }
+ parse_compress_debug_optarg (optarg);
else
- flag_compress_debug = COMPRESS_DEBUG_GABI_ZLIB;
+ parse_compress_debug_optarg ("zlib-gabi");
break;
case OPTION_NOCOMPRESS_DEBUG:
- flag_compress_debug = COMPRESS_DEBUG_NONE;
+ parse_compress_debug_optarg ("none");
break;
case OPTION_DEBUG_PREFIX_MAP:
@@ -1136,6 +1214,8 @@ This program has absolutely no warranty.\n"));
*pargc = new_argc;
*pargv = new_argv;
+ finalize_parse_compress_debug_optarg ();
+
#ifdef md_after_parse_args
md_after_parse_args ();
#endif
diff --git a/gas/as.h b/gas/as.h
index 4c5fa9ecf7d..115af019815 100644
--- a/gas/as.h
+++ b/gas/as.h
@@ -331,6 +331,10 @@ COMMON int flag_traditional_format;
/* Type of compressed debug sections we should generate. */
COMMON enum compressed_debug_section_type flag_compress_debug;
+/* True if we want to generate compressed debug sections, even if it
+ doesn't make them smaller. */
+COMMON bool force_compress_debug;
+
/* TRUE if .note.GNU-stack section with SEC_CODE should be created */
COMMON int flag_execstack;
diff --git a/gas/doc/as.texi b/gas/doc/as.texi
index bbdfa4bfdca..d25559141c7 100644
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -718,7 +718,8 @@ Begin in alternate macro mode.
Compress DWARF debug sections using zlib with SHF_COMPRESSED from the
ELF ABI. The resulting object file may not be compatible with older
linkers and object file utilities. Note if compression would make a
-given section @emph{larger} then it is not compressed.
+given section @emph{larger} then it is not compressed, unless
+@option{--compress-debug-section=force} is used.
@ifset ELF
@cindex @samp{--compress-debug-sections=} option
@@ -727,6 +728,7 @@ given section @emph{larger} then it is not compressed.
@itemx --compress-debug-sections=zlib-gnu
@itemx --compress-debug-sections=zlib-gabi
@itemx --compress-debug-sections=zstd
+@itemx --compress-debug-sections=force
These options control how DWARF debug sections are compressed.
@option{--compress-debug-sections=none} is equivalent to
@option{--nocompress-debug-sections}.
@@ -738,7 +740,11 @@ using the obsoleted zlib-gnu format. The debug sections are renamed to begin
with @samp{.zdebug}.
@option{--compress-debug-sections=zstd} compresses DWARF debug
sections using zstd. Note - if compression would actually make a section
-@emph{larger}, then it is not compressed nor renamed.
+@emph{larger}, then it is not compressed nor renamed, unless
+@option{--compress-debug-section=force} is used.
+@option{--compress-debug-sections=force} compresses DWARF debug sections,
+even if this does not reduce size. It can be used in conjunction with a format
+selection, for instance @option{--compress-debug-section=zstd+force}.
@end ifset
diff --git a/gas/write.c b/gas/write.c
index 8273b7a42f1..39bcea23fac 100644
--- a/gas/write.c
+++ b/gas/write.c
@@ -1465,7 +1465,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
flagword flags = bfd_section_flags (sec);
if (seginfo == NULL
- || uncompressed_size < 32
+ || (!force_compress_debug && uncompressed_size < 32)
|| (flags & SEC_HAS_CONTENTS) == 0)
return;
@@ -1582,7 +1582,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
/* PR binutils/18087: If compression didn't make the section smaller,
just keep it uncompressed. */
- if (compressed_size >= uncompressed_size)
+ if (!force_compress_debug && compressed_size >= uncompressed_size)
return;
/* Replace the uncompressed frag list with the compressed frag list. */
--
2.35.3
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-24 14:57 ` Tom de Vries
@ 2023-02-27 9:03 ` Jan Beulich
2023-02-27 11:43 ` [PATCH] gas: Add --compress-debug-sections=force Tom de Vries
2023-02-27 13:44 ` [PATCH] gas: Add --force-compress-debug-sections Pedro Alves
0 siblings, 2 replies; 24+ messages in thread
From: Jan Beulich @ 2023-02-27 9:03 UTC (permalink / raw)
To: Tom de Vries; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 24.02.2023 15:57, Tom de Vries wrote:
> On 2/24/23 15:26, Jan Beulich wrote:
>> On 24.02.2023 15:11, Tom de Vries wrote:
>>> On 2/24/23 14:23, Jan Beulich wrote:
>>>> On 24.02.2023 13:21, Tom de Vries wrote:
>>>>> On 2/24/23 12:28, Jan Beulich wrote:
>>>>>> I also wouldn't see anything wrong with something
>>>>>> like "...=force,zstd,none" - the last one(s) win. That's no different
>>>>>> from specifying a second instance of the option. And without that it
>>>>>> looks as if the parsing would end up simpler.
>>>>>
>>>>> OK, gave that a try.
>>>>
>>>> That's still accumulating none and force across the entire sequence
>>>> (and then giving none priority over force, no matter that force may
>>>> have been specified last),
>>>
>>> Um, so you're saying that none+zstd+force is currently interpreted as none?
>>>
>>> Lets try:
>>> ...
>>> $ gcc ~/hello.c -c -Wa,-gdwarf-5 -Xassembler
>>> --compress-debug-sections=none+zstd+force
>>> $ readelf -S -W hello.o | grep " .debug"
>>> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
>>> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
>>> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
>>> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
>>> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
>>> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>>>
>>> ...
>>>
>>> So, that doesn't seem to be the case, compression is done, as expected.
>>
>> Oh, I've overlooked that you explicitly clear *none when you set *force
>> (my attention was mainly on the bottom of parse_compress_debug_optarg()).
>> I think that's more involved than necessary (possibly merely a result of
>> you having worked incrementally from your earlier version), and less
>> obviously doing the same as would happen when multiple separate options
>> were parsed.
>
> I've tried to simplify further.
>
> Is this more how you want it?
I have to admit that I'm still puzzled by the presence of
finalize_parse_compress_debug_optarg() as well as you needing both a new
static variable and a new global one. But I guess whether that's really
needed first of all depends on the semantics we want e.g.
--nocompress-debug-sections --compress-debug-sections=force
to have (which, with how you have it presently, could also be expressed
as
--compress-debug-sections=none+force
or
--compress-debug-sections=none --compress-debug-sections=force
afaict). I view the present meaning as one sensible one, but I could
also see "none" (or equivalent) simply zapping the compression type
(and hence rendering "force" meaningless) as another sensible one. A
change in meaning may then also result in the three option combinations
above possibly not all doing the same.
As an aside: As you update the patch, please try to keep the title in
line with what the patch actually does.
Also, ftaod, I don't mean to stand in the way of another maintainer
approving any of the forms proposed so far. This specifically also
includes the use of '+' as a separator, which I personally don't
(currently) intend to approve.
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] gas: Add --compress-debug-sections=force
2023-02-27 9:03 ` Jan Beulich
@ 2023-02-27 11:43 ` Tom de Vries
2023-02-27 11:51 ` Jan Beulich
2023-02-27 13:44 ` [PATCH] gas: Add --force-compress-debug-sections Pedro Alves
1 sibling, 1 reply; 24+ messages in thread
From: Tom de Vries @ 2023-02-27 11:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
[-- Attachment #1: Type: text/plain, Size: 4169 bytes --]
[ was: Re: [PATCH] gas: Add --force-compress-debug-sections ]
On 2/27/23 10:03, Jan Beulich wrote:
> On 24.02.2023 15:57, Tom de Vries wrote:
>> On 2/24/23 15:26, Jan Beulich wrote:
>>> On 24.02.2023 15:11, Tom de Vries wrote:
>>>> On 2/24/23 14:23, Jan Beulich wrote:
>>>>> On 24.02.2023 13:21, Tom de Vries wrote:
>>>>>> On 2/24/23 12:28, Jan Beulich wrote:
>>>>>>> I also wouldn't see anything wrong with something
>>>>>>> like "...=force,zstd,none" - the last one(s) win. That's no different
>>>>>>> from specifying a second instance of the option. And without that it
>>>>>>> looks as if the parsing would end up simpler.
>>>>>>
>>>>>> OK, gave that a try.
>>>>>
>>>>> That's still accumulating none and force across the entire sequence
>>>>> (and then giving none priority over force, no matter that force may
>>>>> have been specified last),
>>>>
>>>> Um, so you're saying that none+zstd+force is currently interpreted as none?
>>>>
>>>> Lets try:
>>>> ...
>>>> $ gcc ~/hello.c -c -Wa,-gdwarf-5 -Xassembler
>>>> --compress-debug-sections=none+zstd+force
>>>> $ readelf -S -W hello.o | grep " .debug"
>>>> [ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
>>>> [11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
>>>> [12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
>>>> [14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
>>>> [15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
>>>> [17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
>>>>
>>>> ...
>>>>
>>>> So, that doesn't seem to be the case, compression is done, as expected.
>>>
>>> Oh, I've overlooked that you explicitly clear *none when you set *force
>>> (my attention was mainly on the bottom of parse_compress_debug_optarg()).
>>> I think that's more involved than necessary (possibly merely a result of
>>> you having worked incrementally from your earlier version), and less
>>> obviously doing the same as would happen when multiple separate options
>>> were parsed.
>>
>> I've tried to simplify further.
>>
>> Is this more how you want it?
>
> I have to admit that I'm still puzzled by the presence of
> finalize_parse_compress_debug_optarg() as well as you needing both a new
> static variable and a new global one.
The latter I've fixed, by exporting the static variable.
That eliminates one half of finalize_parse_compress_debug_optarg().
The remaining half is necessary because we accumulate "none" into
compress_debug_action instead of into flag_compress_debug, so after
parsing is over we need to assign the accumulation result, if it is
indeed cda_none, to flag_compress_debug.
And the reason we have that complicated setup is related to preventing
none from zapping compression type.
> But I guess whether that's really
> needed first of all depends on the semantics we want e.g.
>
> --nocompress-debug-sections --compress-debug-sections=force
>
> to have (which, with how you have it presently, could also be expressed
> as
>
> --compress-debug-sections=none+force
>
> or
>
> --compress-debug-sections=none --compress-debug-sections=force
>
> afaict). I view the present meaning as one sensible one, but I could
> also see "none" (or equivalent) simply zapping the compression type
> (and hence rendering "force" meaningless) as another sensible one.
Sure, that's another possible semantics.
Should I implement that instead?
> A
> change in meaning may then also result in the three option combinations
> above possibly not all doing the same.
>
> As an aside: As you update the patch, please try to keep the title in
> line with what the patch actually does.
>
I'm assuming you mean the email title (the patch title looks ok to me),
and I've updated it.
> Also, ftaod, I don't mean to stand in the way of another maintainer
> approving any of the forms proposed so far. This specifically also
> includes the use of '+' as a separator, which I personally don't
> (currently) intend to approve.
Well, in case that's blocking you from giving approval, I've updated the
patch to use the ',' separator.
Thanks,
- Tom
[-- Attachment #2: 0001-gas-Add-compress-debug-sections-force.patch --]
[-- Type: text/x-patch, Size: 9714 bytes --]
From 0fb4ed6d648a69799cdbf6b19fd36c6125c5fc41 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Thu, 23 Feb 2023 12:53:40 +0100
Subject: [PATCH] gas: Add --compress-debug-sections=force
Gas has an option --compress-debug-sections that allows it to generate
compressed debug sections.
That does not guarantee that the debug sections are in fact compressed:
...
$ gcc hello.c -Wa,-gdwarf-5 -c -Wa,--compress-debug-sections=zstd
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000053 00 0 0 1
[11] .debug_line_str PROGBITS 0000fb 000025 01 MS 0 0 1
[12] .debug_info PROGBITS 000120 000039 00 0 0 1
[14] .debug_abbrev PROGBITS 000159 000028 00 0 0 1
[15] .debug_aranges PROGBITS 000190 000030 00 0 0 16
[17] .debug_str PROGBITS 0001c0 000039 01 MS 0 0 1
...
Sensibly so, they're only compressed if that provides a size benefit.
However, for the purpose of testing components consuming dwarf
we may want the sections to be compressed regardless.
Add a new suboption --compress-debug-sections=force that ignores the size
heuristic, such that we have instead:
...
$ gcc hello.c -Wa,-gdwarf-5 -c -Xassembler \
--compress-debug-sections=zstd,force
$ readelf -S -W hello.o | grep " .debug"
[ 9] .debug_line PROGBITS 0000a8 000064 00 C 0 0 8
[11] .debug_line_str PROGBITS 000110 000046 01 MSC 0 0 8
[12] .debug_info PROGBITS 000158 000046 00 C 0 0 8
[14] .debug_abbrev PROGBITS 0001a0 000049 00 C 0 0 8
[15] .debug_aranges PROGBITS 0001f0 000034 00 C 0 0 8
[17] .debug_str PROGBITS 000228 00005a 01 MSC 0 0 8
...
Advertised as:
...
$ as --help
...
--compress-debug-sections[={none|<format>|force|force,<format>}]
where <format> is {zlib|zlib-gnu|zlib-gabi|zstd}
compress DWARF debug sections
Default: zstd
...
Tested on x86_64-linux.
---
gas/as.c | 106 +++++++++++++++++++++++++++++++++++++++---------
gas/as.h | 10 +++++
gas/doc/as.texi | 10 ++++-
gas/write.c | 5 ++-
4 files changed, 108 insertions(+), 23 deletions(-)
diff --git a/gas/as.c b/gas/as.c
index 598bfd56cf5..acbccfb5dce 100644
--- a/gas/as.c
+++ b/gas/as.c
@@ -230,6 +230,8 @@ enum compressed_debug_section_type flag_compress_debug
= DEFAULT_COMPRESSED_DEBUG_ALGORITHM;
#endif
+enum compress_debug_action compress_debug_action = cda_default;
+
static void
show_usage (FILE * stream)
{
@@ -252,7 +254,8 @@ Options:\n\
fprintf (stream, _("\
--alternate initially turn on alternate macro syntax\n"));
fprintf (stream, _("\
- --compress-debug-sections[={none|zlib|zlib-gnu|zlib-gabi|zstd}]\n\
+ --compress-debug-sections[={none|<format>|force|force,<format>}]\n\
+ where <format> is {zlib|zlib-gnu|zlib-gabi|zstd}\n\
compress DWARF debug sections\n")),
fprintf (stream, _("\
Default: %s\n"),
@@ -418,6 +421,84 @@ Options:\n\
fprintf (stream, _("Report bugs to %s\n"), REPORT_BUGS_TO);
}
+static void
+parse_compress_debug_optarg_1 (const char *optarg)
+{
+ gas_assert (optarg != NULL);
+
+ if (strcmp (optarg, "force") == 0)
+ {
+ compress_debug_action = cda_force;
+ return;
+ }
+
+ enum compressed_debug_section_type tmp
+ = bfd_get_compression_algorithm (optarg);
+
+#ifndef HAVE_ZSTD
+ if (tmp == COMPRESS_DEBUG_ZSTD)
+ as_fatal (_ ("--compress-debug-sections=zstd: gas is not "
+ "built with zstd support"));
+#endif
+
+ if (tmp == COMPRESS_UNKNOWN)
+ as_fatal (_("Invalid --compress-debug-sections option: `%s'"),
+ optarg);
+
+ if (tmp == COMPRESS_DEBUG_NONE)
+ {
+ compress_debug_action = cda_none;
+ return;
+ }
+
+ compress_debug_action = cda_yes;
+ flag_compress_debug = tmp;
+}
+
+static void
+parse_compress_debug_optarg (const char *optarg)
+{
+#if !defined OBJ_ELF && !defined OBJ_MAYBE_ELF
+ as_fatal (_("--compress-debug-sections=%s is unsupported"),
+ optarg);
+#endif
+
+ /* Tokenize subopts pass to parse_compress_debug_optarg_1. */
+ char sep = ',';
+ while (true)
+ {
+ const char *idx = optarg;
+ while (*idx != '\0' && *idx != sep)
+ idx++;
+
+ size_t len = idx - optarg;
+ if (len == 0)
+ {
+ /* Generate error. */
+ parse_compress_debug_optarg_1 ("");
+ break;
+ }
+
+ char *tmp = xstrndup (optarg, len);
+ parse_compress_debug_optarg_1 (tmp);
+ free (tmp);
+
+ if (*idx == '\0')
+ break;
+
+ /* Step over separator and continue tokenizing. */
+ gas_assert (*idx == sep);
+ optarg = idx + 1;
+ }
+}
+
+static void
+finalize_parse_compress_debug_optarg (void)
+{
+ if (compress_debug_action == cda_none)
+ flag_compress_debug = COMPRESS_DEBUG_NONE;
+}
+
/* Since it is easy to do here we interpret the special arg "-"
to mean "use stdin" and we set that argv[] pointing to "".
After we have munged argv[], the only things left are source file
@@ -747,28 +828,13 @@ This program has absolutely no warranty.\n"));
case OPTION_COMPRESS_DEBUG:
if (optarg)
- {
-#if defined OBJ_ELF || defined OBJ_MAYBE_ELF
- flag_compress_debug = bfd_get_compression_algorithm (optarg);
-#ifndef HAVE_ZSTD
- if (flag_compress_debug == COMPRESS_DEBUG_ZSTD)
- as_fatal (_ ("--compress-debug-sections=zstd: gas is not "
- "built with zstd support"));
-#endif
- if (flag_compress_debug == COMPRESS_UNKNOWN)
- as_fatal (_("Invalid --compress-debug-sections option: `%s'"),
- optarg);
-#else
- as_fatal (_("--compress-debug-sections=%s is unsupported"),
- optarg);
-#endif
- }
+ parse_compress_debug_optarg (optarg);
else
- flag_compress_debug = COMPRESS_DEBUG_GABI_ZLIB;
+ parse_compress_debug_optarg ("zlib-gabi");
break;
case OPTION_NOCOMPRESS_DEBUG:
- flag_compress_debug = COMPRESS_DEBUG_NONE;
+ parse_compress_debug_optarg ("none");
break;
case OPTION_DEBUG_PREFIX_MAP:
@@ -1136,6 +1202,8 @@ This program has absolutely no warranty.\n"));
*pargc = new_argc;
*pargv = new_argv;
+ finalize_parse_compress_debug_optarg ();
+
#ifdef md_after_parse_args
md_after_parse_args ();
#endif
diff --git a/gas/as.h b/gas/as.h
index 4c5fa9ecf7d..bc0fe7f5cd9 100644
--- a/gas/as.h
+++ b/gas/as.h
@@ -331,6 +331,16 @@ COMMON int flag_traditional_format;
/* Type of compressed debug sections we should generate. */
COMMON enum compressed_debug_section_type flag_compress_debug;
+/* Whether we should compress debug sections. */
+enum compress_debug_action
+{
+ cda_default,
+ cda_none,
+ cda_force,
+ cda_yes,
+};
+COMMON enum compress_debug_action compress_debug_action;
+
/* TRUE if .note.GNU-stack section with SEC_CODE should be created */
COMMON int flag_execstack;
diff --git a/gas/doc/as.texi b/gas/doc/as.texi
index bbdfa4bfdca..f0140c09779 100644
--- a/gas/doc/as.texi
+++ b/gas/doc/as.texi
@@ -718,7 +718,8 @@ Begin in alternate macro mode.
Compress DWARF debug sections using zlib with SHF_COMPRESSED from the
ELF ABI. The resulting object file may not be compatible with older
linkers and object file utilities. Note if compression would make a
-given section @emph{larger} then it is not compressed.
+given section @emph{larger} then it is not compressed, unless
+@option{--compress-debug-section=force} is used.
@ifset ELF
@cindex @samp{--compress-debug-sections=} option
@@ -727,6 +728,7 @@ given section @emph{larger} then it is not compressed.
@itemx --compress-debug-sections=zlib-gnu
@itemx --compress-debug-sections=zlib-gabi
@itemx --compress-debug-sections=zstd
+@itemx --compress-debug-sections=force
These options control how DWARF debug sections are compressed.
@option{--compress-debug-sections=none} is equivalent to
@option{--nocompress-debug-sections}.
@@ -738,7 +740,11 @@ using the obsoleted zlib-gnu format. The debug sections are renamed to begin
with @samp{.zdebug}.
@option{--compress-debug-sections=zstd} compresses DWARF debug
sections using zstd. Note - if compression would actually make a section
-@emph{larger}, then it is not compressed nor renamed.
+@emph{larger}, then it is not compressed nor renamed, unless
+@option{--compress-debug-section=force} is used.
+@option{--compress-debug-sections=force} compresses DWARF debug sections,
+even if this does not reduce size. It can be used in conjunction with a format
+selection, for instance @option{--compress-debug-section=zstd,force}.
@end ifset
diff --git a/gas/write.c b/gas/write.c
index 8273b7a42f1..1fa3b54a03b 100644
--- a/gas/write.c
+++ b/gas/write.c
@@ -1463,9 +1463,10 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
segment_info_type *seginfo = seg_info (sec);
bfd_size_type uncompressed_size = sec->size;
flagword flags = bfd_section_flags (sec);
+ bool force_compress_debug = compress_debug_action == cda_force;
if (seginfo == NULL
- || uncompressed_size < 32
+ || (!force_compress_debug && uncompressed_size < 32)
|| (flags & SEC_HAS_CONTENTS) == 0)
return;
@@ -1582,7 +1583,7 @@ compress_debug (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
/* PR binutils/18087: If compression didn't make the section smaller,
just keep it uncompressed. */
- if (compressed_size >= uncompressed_size)
+ if (!force_compress_debug && compressed_size >= uncompressed_size)
return;
/* Replace the uncompressed frag list with the compressed frag list. */
--
2.35.3
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --compress-debug-sections=force
2023-02-27 11:43 ` [PATCH] gas: Add --compress-debug-sections=force Tom de Vries
@ 2023-02-27 11:51 ` Jan Beulich
0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2023-02-27 11:51 UTC (permalink / raw)
To: Tom de Vries; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 27.02.2023 12:43, Tom de Vries wrote:
> On 2/27/23 10:03, Jan Beulich wrote:
>> But I guess whether that's really
>> needed first of all depends on the semantics we want e.g.
>>
>> --nocompress-debug-sections --compress-debug-sections=force
>>
>> to have (which, with how you have it presently, could also be expressed
>> as
>>
>> --compress-debug-sections=none+force
>>
>> or
>>
>> --compress-debug-sections=none --compress-debug-sections=force
>>
>> afaict). I view the present meaning as one sensible one, but I could
>> also see "none" (or equivalent) simply zapping the compression type
>> (and hence rendering "force" meaningless) as another sensible one.
>
> Sure, that's another possible semantics.
>
> Should I implement that instead?
Not sure - as said, depends on what exactly is wanted. I find both
variants leaving too much room for ambiguity, so I can't really decide
(for myself).
>> A
>> change in meaning may then also result in the three option combinations
>> above possibly not all doing the same.
>>
>
>> As an aside: As you update the patch, please try to keep the title in
>> line with what the patch actually does.
>>
>
> I'm assuming you mean the email title (the patch title looks ok to me),
> and I've updated it.
Hmm, yes, sorry - I'm normally implying new patch versions to be at
the root of new threads (and with increased version number), not in
reply to earlier versions. Hence I didn't pay attention to the Re: ...
still being there from the initial patch.
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-27 9:03 ` Jan Beulich
2023-02-27 11:43 ` [PATCH] gas: Add --compress-debug-sections=force Tom de Vries
@ 2023-02-27 13:44 ` Pedro Alves
2023-02-27 14:07 ` Jan Beulich
1 sibling, 1 reply; 24+ messages in thread
From: Pedro Alves @ 2023-02-27 13:44 UTC (permalink / raw)
To: Jan Beulich, Tom de Vries
Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 2023-02-27 9:03 a.m., Jan Beulich via Binutils wrote:
> On 24.02.2023 15:57, Tom de Vries wrote:
>>
>> Is this more how you want it?
>
> I have to admit that I'm still puzzled by the presence of
> finalize_parse_compress_debug_optarg() as well as you needing both a new
> static variable and a new global one. But I guess whether that's really
> needed first of all depends on the semantics we want e.g.
>
> --nocompress-debug-sections --compress-debug-sections=force
>
> to have (which, with how you have it presently, could also be expressed
> as
>
> --compress-debug-sections=none+force
>
> or
>
> --compress-debug-sections=none --compress-debug-sections=force
>
> afaict). I view the present meaning as one sensible one, but I could
> also see "none" (or equivalent) simply zapping the compression type
> (and hence rendering "force" meaningless) as another sensible one. A
> change in meaning may then also result in the three option combinations
> above possibly not all doing the same.
ISTM that these confusions (which no doubt users will have too) would go away if
we did not try to put orthogonal settings into one option.
Witness how the implementation uses two different enums:
This new one:
+enum compress_debug_action
+{
+ cda_default,
+ cda_none,
+ cda_force,
+ cda_yes,
+};
And this preexisting one:
/* Types of compressed DWARF debug sections. */
enum compressed_debug_section_type
{
COMPRESS_DEBUG_NONE = 0,
COMPRESS_DEBUG_GNU_ZLIB = 1 << 1,
COMPRESS_DEBUG_GABI_ZLIB = 1 << 2,
COMPRESS_DEBUG_ZSTD = 1 << 3,
COMPRESS_UNKNOWN = 1 << 4
};
Imagine we started over from scratch, and had these two orthogonal options,
matching internal enums (enum compress_debug_action would be slightly different):
--compressed-debug-sections-format=zlib|zstd|...|none
# Iff we're compressing, what format shall we use?
--compress-debug-sections=no|yes|sizewin
# Are we compressing debug sections? When?
#
# - "no" - never, we're not compressing.
#
# - "yes" - always, we're compressing, using the format specified
# by --compressed-debug-sections-format (and if that is "none", well,
# you still get what you asked for).
#
# - "sizewin" - compress if there's a size win. Like "yes", if the format is
# "none", well, this becomes a nop.
... then the semantics of mixing these options, or what happens when you repeat them
would be much more obvious:
- You can specify '--compressed-debug-sections-format' multiple times. The last wins.
- You can specify '--compress-debug-sections' multiple times. The last wins.
- Changing '--compressed-debug-sections-format' does not affect the value of '--compress-debug-sections'.
- Changing '--compress-debug-sections' does not affect the value of '--compressed-debug-sections-format'
Now, we can't use those option names with that meaning, though, because
'--compress-debug-sections' today already has the meaning of selecting
the compression format. But that just means we would need to pick different
names, like for example:
--compress-debug-sections=zlib|zstd|none
# Iff we're compressing, what format shall we use?
--nocompress-debug-sections
# Shorthand for --compress-debug-sections=none
--compress-debug-sections-when=never|always|sizewin
# Are we compressing debug sections? When?
#
# - "never" - never, we're not compressing.
#
# - "always" - always compress, using the format specified by
# by --compress-debug-sections (and if that is "none", well,
# you still get what you asked for).
#
# - "sizewin", compress if there's a size win. Like "always", respects
# the format specified by --compress-debug-sections.
The semantics of mixing these options, or what happens when you repeat them
would be obvious in the same way in the other options naming earlier:
- You can specify '--compress-debug-sections' multiple times. The last wins.
- You can specify '--compress-debug-sections-when' multiple times. The last wins.
- Changing '--compress-debug-sections' does not affect the value of '--compress-debug-sections-when'.
- Changing '--compress-debug-sections-when' does not affect the value of '--compressed-debug-sections'
You end up with two different ways to disable compressing debug sections,
but that seems OK to me.
All that would be left would be bikeshed on the new option name.
Pedro Alves
>
> As an aside: As you update the patch, please try to keep the title in
> line with what the patch actually does.
>
> Also, ftaod, I don't mean to stand in the way of another maintainer
> approving any of the forms proposed so far. This specifically also
> includes the use of '+' as a separator, which I personally don't
> (currently) intend to approve.
>
> Jan
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-27 13:44 ` [PATCH] gas: Add --force-compress-debug-sections Pedro Alves
@ 2023-02-27 14:07 ` Jan Beulich
2023-02-27 23:24 ` Tom de Vries
2023-02-28 12:49 ` Pedro Alves
0 siblings, 2 replies; 24+ messages in thread
From: Jan Beulich @ 2023-02-27 14:07 UTC (permalink / raw)
To: Pedro Alves
Cc: binutils, Michael Matz, Nick Clifton, Alan Modra, Tom de Vries
On 27.02.2023 14:44, Pedro Alves wrote:
> On 2023-02-27 9:03 a.m., Jan Beulich via Binutils wrote:
>> On 24.02.2023 15:57, Tom de Vries wrote:
>>>
>>> Is this more how you want it?
>>
>> I have to admit that I'm still puzzled by the presence of
>> finalize_parse_compress_debug_optarg() as well as you needing both a new
>> static variable and a new global one. But I guess whether that's really
>> needed first of all depends on the semantics we want e.g.
>>
>> --nocompress-debug-sections --compress-debug-sections=force
>>
>> to have (which, with how you have it presently, could also be expressed
>> as
>>
>> --compress-debug-sections=none+force
>>
>> or
>>
>> --compress-debug-sections=none --compress-debug-sections=force
>>
>> afaict). I view the present meaning as one sensible one, but I could
>> also see "none" (or equivalent) simply zapping the compression type
>> (and hence rendering "force" meaningless) as another sensible one. A
>> change in meaning may then also result in the three option combinations
>> above possibly not all doing the same.
>
>
> ISTM that these confusions (which no doubt users will have too) would go away if
> we did not try to put orthogonal settings into one option.
>
> Witness how the implementation uses two different enums:
>
> This new one:
>
> +enum compress_debug_action
> +{
> + cda_default,
> + cda_none,
> + cda_force,
> + cda_yes,
> +};
>
> And this preexisting one:
>
> /* Types of compressed DWARF debug sections. */
> enum compressed_debug_section_type
> {
> COMPRESS_DEBUG_NONE = 0,
> COMPRESS_DEBUG_GNU_ZLIB = 1 << 1,
> COMPRESS_DEBUG_GABI_ZLIB = 1 << 2,
> COMPRESS_DEBUG_ZSTD = 1 << 3,
> COMPRESS_UNKNOWN = 1 << 4
> };
>
> Imagine we started over from scratch, and had these two orthogonal options,
> matching internal enums (enum compress_debug_action would be slightly different):
>
> --compressed-debug-sections-format=zlib|zstd|...|none
>
> # Iff we're compressing, what format shall we use?
>
> --compress-debug-sections=no|yes|sizewin
>
> # Are we compressing debug sections? When?
> #
> # - "no" - never, we're not compressing.
> #
> # - "yes" - always, we're compressing, using the format specified
> # by --compressed-debug-sections-format (and if that is "none", well,
> # you still get what you asked for).
> #
> # - "sizewin" - compress if there's a size win. Like "yes", if the format is
> # "none", well, this becomes a nop.
>
> ... then the semantics of mixing these options, or what happens when you repeat them
> would be much more obvious:
>
> - You can specify '--compressed-debug-sections-format' multiple times. The last wins.
>
> - You can specify '--compress-debug-sections' multiple times. The last wins.
>
> - Changing '--compressed-debug-sections-format' does not affect the value of '--compress-debug-sections'.
>
> - Changing '--compress-debug-sections' does not affect the value of '--compressed-debug-sections-format'
>
>
> Now, we can't use those option names with that meaning, though, because
> '--compress-debug-sections' today already has the meaning of selecting
> the compression format. But that just means we would need to pick different
> names, like for example:
>
> --compress-debug-sections=zlib|zstd|none
>
> # Iff we're compressing, what format shall we use?
>
> --nocompress-debug-sections
>
> # Shorthand for --compress-debug-sections=none
>
> --compress-debug-sections-when=never|always|sizewin
>
> # Are we compressing debug sections? When?
> #
> # - "never" - never, we're not compressing.
> #
> # - "always" - always compress, using the format specified by
> # by --compress-debug-sections (and if that is "none", well,
> # you still get what you asked for).
> #
> # - "sizewin", compress if there's a size win. Like "always", respects
> # the format specified by --compress-debug-sections.
>
>
> The semantics of mixing these options, or what happens when you repeat them
> would be obvious in the same way in the other options naming earlier:
>
> - You can specify '--compress-debug-sections' multiple times. The last wins.
>
> - You can specify '--compress-debug-sections-when' multiple times. The last wins.
>
> - Changing '--compress-debug-sections' does not affect the value of '--compress-debug-sections-when'.
>
> - Changing '--compress-debug-sections-when' does not affect the value of '--compressed-debug-sections'
>
> You end up with two different ways to disable compressing debug sections,
> but that seems OK to me.
That would be okay with me as well, but (in part to avoid ...
> All that would be left would be bikeshed on the new option name.
... this aspect) I fail to see why a single option won't do:
--compress-debug-sections=zlib|zstd|no|yes|sizewin
Since the sub-options are distinct sets (alongside "no" one could add "none"
if wanted, to separate "method: no compression" from "don't compress", which
I think would then also eliminate the ambiguity referred to earlier on
[still visible in context above]), within each set your "last wins" would
apply equally. And in fact that was the main reason for my original response
back to Tom: I view the proliferation of command line options (not just
here, but in general) as a scalability issue. Therefore my rule of thumb is
that things which belong together are best grouped under a single top-level
option.
Jan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-27 14:07 ` Jan Beulich
@ 2023-02-27 23:24 ` Tom de Vries
2023-02-28 0:19 ` Tom de Vries
2023-02-28 13:21 ` Pedro Alves
2023-02-28 12:49 ` Pedro Alves
1 sibling, 2 replies; 24+ messages in thread
From: Tom de Vries @ 2023-02-27 23:24 UTC (permalink / raw)
To: Jan Beulich, Pedro Alves; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 2/27/23 15:07, Jan Beulich wrote:
> On 27.02.2023 14:44, Pedro Alves wrote:
>> On 2023-02-27 9:03 a.m., Jan Beulich via Binutils wrote:
>>> On 24.02.2023 15:57, Tom de Vries wrote:
>>>>
>>>> Is this more how you want it?
>>>
>>> I have to admit that I'm still puzzled by the presence of
>>> finalize_parse_compress_debug_optarg() as well as you needing both a new
>>> static variable and a new global one. But I guess whether that's really
>>> needed first of all depends on the semantics we want e.g.
>>>
>>> --nocompress-debug-sections --compress-debug-sections=force
>>>
>>> to have (which, with how you have it presently, could also be expressed
>>> as
>>>
>>> --compress-debug-sections=none+force
>>>
>>> or
>>>
>>> --compress-debug-sections=none --compress-debug-sections=force
>>>
>>> afaict). I view the present meaning as one sensible one, but I could
>>> also see "none" (or equivalent) simply zapping the compression type
>>> (and hence rendering "force" meaningless) as another sensible one. A
>>> change in meaning may then also result in the three option combinations
>>> above possibly not all doing the same.
>>
>>
>> ISTM that these confusions (which no doubt users will have too) would go away if
>> we did not try to put orthogonal settings into one option.
>>
>> Witness how the implementation uses two different enums:
>>
>> This new one:
>>
>> +enum compress_debug_action
>> +{
>> + cda_default,
>> + cda_none,
>> + cda_force,
>> + cda_yes,
>> +};
>>
>> And this preexisting one:
>>
>> /* Types of compressed DWARF debug sections. */
>> enum compressed_debug_section_type
>> {
>> COMPRESS_DEBUG_NONE = 0,
>> COMPRESS_DEBUG_GNU_ZLIB = 1 << 1,
>> COMPRESS_DEBUG_GABI_ZLIB = 1 << 2,
>> COMPRESS_DEBUG_ZSTD = 1 << 3,
>> COMPRESS_UNKNOWN = 1 << 4
>> };
>>
>> Imagine we started over from scratch, and had these two orthogonal options,
>> matching internal enums (enum compress_debug_action would be slightly different):
>>
>> --compressed-debug-sections-format=zlib|zstd|...|none
>>
>> # Iff we're compressing, what format shall we use?
>>
>> --compress-debug-sections=no|yes|sizewin
>>
>> # Are we compressing debug sections? When?
>> #
>> # - "no" - never, we're not compressing.
>> #
>> # - "yes" - always, we're compressing, using the format specified
>> # by --compressed-debug-sections-format (and if that is "none", well,
>> # you still get what you asked for).
>> #
>> # - "sizewin" - compress if there's a size win. Like "yes", if the format is
>> # "none", well, this becomes a nop.
>>
>> ... then the semantics of mixing these options, or what happens when you repeat them
>> would be much more obvious:
>>
>> - You can specify '--compressed-debug-sections-format' multiple times. The last wins.
>>
>> - You can specify '--compress-debug-sections' multiple times. The last wins.
>>
>> - Changing '--compressed-debug-sections-format' does not affect the value of '--compress-debug-sections'.
>>
>> - Changing '--compress-debug-sections' does not affect the value of '--compressed-debug-sections-format'
>>
>>
>> Now, we can't use those option names with that meaning, though, because
>> '--compress-debug-sections' today already has the meaning of selecting
>> the compression format. But that just means we would need to pick different
>> names, like for example:
>>
>> --compress-debug-sections=zlib|zstd|none
>>
>> # Iff we're compressing, what format shall we use?
>>
>> --nocompress-debug-sections
>>
>> # Shorthand for --compress-debug-sections=none
>>
>> --compress-debug-sections-when=never|always|sizewin
>>
>> # Are we compressing debug sections? When?
>> #
>> # - "never" - never, we're not compressing.
>> #
>> # - "always" - always compress, using the format specified by
>> # by --compress-debug-sections (and if that is "none", well,
>> # you still get what you asked for).
>> #
>> # - "sizewin", compress if there's a size win. Like "always", respects
>> # the format specified by --compress-debug-sections.
>>
>>
>> The semantics of mixing these options, or what happens when you repeat them
>> would be obvious in the same way in the other options naming earlier:
>>
>> - You can specify '--compress-debug-sections' multiple times. The last wins.
>>
>> - You can specify '--compress-debug-sections-when' multiple times. The last wins.
>>
>> - Changing '--compress-debug-sections' does not affect the value of '--compress-debug-sections-when'.
>>
>> - Changing '--compress-debug-sections-when' does not affect the value of '--compressed-debug-sections'
>>
>> You end up with two different ways to disable compressing debug sections,
>> but that seems OK to me.
>
> That would be okay with me as well, but (in part to avoid ...
>
>> All that would be left would be bikeshed on the new option name.
>
> ... this aspect) I fail to see why a single option won't do:
>
> --compress-debug-sections=zlib|zstd|no|yes|sizewin
>
> Since the sub-options are distinct sets (alongside "no" one could add "none"
> if wanted, to separate "method: no compression" from "don't compress", which
> I think would then also eliminate the ambiguity referred to earlier on
> [still visible in context above]), within each set your "last wins" would
> apply equally. And in fact that was the main reason for my original response
> back to Tom: I view the proliferation of command line options (not just
> here, but in general) as a scalability issue. Therefore my rule of thumb is
> that things which belong together are best grouped under a single top-level
> option.
OK, so say we have --compress-debug-sections=none|zlib|zstd|no|yes|sizewin.
We have the two existing aliases, --compress-debug-sections and
--nocompress-debug-sections, how do we translate those?
Currently --nocompress-debug-sections maps to
--compress-debug-sections=none, but I suppose that would have to become
--compress-debug-sections=no, otherwise things will get extremely confusing.
Then --compress-debug-sections currently maps to
--compress-debug-sections=zlib. So, should that become
--compress-debug-sections=zlib,sizewin? Or just stay
--compress-debug-sections=zlib? I'm not sure if we have made things
clearer or more orthogonal this way.
I'm starting to wonder if not a simple
--compress-debug-sections=heuristic-sizewin|heuristic-always is the
solution, in the sense that these two would only influence the
heuristics and nothing else, which would work well with the existing
options.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-27 23:24 ` Tom de Vries
@ 2023-02-28 0:19 ` Tom de Vries
2023-02-28 13:21 ` Pedro Alves
1 sibling, 0 replies; 24+ messages in thread
From: Tom de Vries @ 2023-02-28 0:19 UTC (permalink / raw)
To: Jan Beulich, Pedro Alves; +Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 2/28/23 00:24, Tom de Vries wrote:
> I'm starting to wonder if not a simple
> --compress-debug-sections=heuristic-sizewin|heuristic-always is the
> solution, in the sense that these two would only influence the
> heuristics and nothing else, which would work well with the existing
> options.
Submitted a v2 patch series implementing this approach here (
https://sourceware.org/pipermail/binutils/2023-February/126335.html ).
Thanks,
- Tom
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-27 14:07 ` Jan Beulich
2023-02-27 23:24 ` Tom de Vries
@ 2023-02-28 12:49 ` Pedro Alves
1 sibling, 0 replies; 24+ messages in thread
From: Pedro Alves @ 2023-02-28 12:49 UTC (permalink / raw)
To: Jan Beulich
Cc: binutils, Michael Matz, Nick Clifton, Alan Modra, Tom de Vries
On 2023-02-27 2:07 p.m., Jan Beulich wrote:
> On 27.02.2023 14:44, Pedro Alves wrote:
>> On 2023-02-27 9:03 a.m., Jan Beulich via Binutils wrote:
>>> On 24.02.2023 15:57, Tom de Vries wrote:
>>>>
>>>> Is this more how you want it?
>>>
>>> I have to admit that I'm still puzzled by the presence of
>>> finalize_parse_compress_debug_optarg() as well as you needing both a new
>>> static variable and a new global one. But I guess whether that's really
>>> needed first of all depends on the semantics we want e.g.
>>>
>>> --nocompress-debug-sections --compress-debug-sections=force
>>>
>>> to have (which, with how you have it presently, could also be expressed
>>> as
>>>
>>> --compress-debug-sections=none+force
>>>
>>> or
>>>
>>> --compress-debug-sections=none --compress-debug-sections=force
>>>
>>> afaict). I view the present meaning as one sensible one, but I could
>>> also see "none" (or equivalent) simply zapping the compression type
>>> (and hence rendering "force" meaningless) as another sensible one. A
>>> change in meaning may then also result in the three option combinations
>>> above possibly not all doing the same.
>>
>>
>> ISTM that these confusions (which no doubt users will have too) would go away if
>> we did not try to put orthogonal settings into one option.
>>
>> Witness how the implementation uses two different enums:
>>
>> This new one:
>>
>> +enum compress_debug_action
>> +{
>> + cda_default,
>> + cda_none,
>> + cda_force,
>> + cda_yes,
>> +};
>>
>> And this preexisting one:
>>
>> /* Types of compressed DWARF debug sections. */
>> enum compressed_debug_section_type
>> {
>> COMPRESS_DEBUG_NONE = 0,
>> COMPRESS_DEBUG_GNU_ZLIB = 1 << 1,
>> COMPRESS_DEBUG_GABI_ZLIB = 1 << 2,
>> COMPRESS_DEBUG_ZSTD = 1 << 3,
>> COMPRESS_UNKNOWN = 1 << 4
>> };
>>
>> Imagine we started over from scratch, and had these two orthogonal options,
>> matching internal enums (enum compress_debug_action would be slightly different):
>>
>> --compressed-debug-sections-format=zlib|zstd|...|none
>>
>> # Iff we're compressing, what format shall we use?
>>
>> --compress-debug-sections=no|yes|sizewin
>>
>> # Are we compressing debug sections? When?
>> #
>> # - "no" - never, we're not compressing.
>> #
>> # - "yes" - always, we're compressing, using the format specified
>> # by --compressed-debug-sections-format (and if that is "none", well,
>> # you still get what you asked for).
>> #
>> # - "sizewin" - compress if there's a size win. Like "yes", if the format is
>> # "none", well, this becomes a nop.
>>
>> ... then the semantics of mixing these options, or what happens when you repeat them
>> would be much more obvious:
>>
>> - You can specify '--compressed-debug-sections-format' multiple times. The last wins.
>>
>> - You can specify '--compress-debug-sections' multiple times. The last wins.
>>
>> - Changing '--compressed-debug-sections-format' does not affect the value of '--compress-debug-sections'.
>>
>> - Changing '--compress-debug-sections' does not affect the value of '--compressed-debug-sections-format'
>>
>>
>> Now, we can't use those option names with that meaning, though, because
>> '--compress-debug-sections' today already has the meaning of selecting
>> the compression format. But that just means we would need to pick different
>> names, like for example:
>>
>> --compress-debug-sections=zlib|zstd|none
>>
>> # Iff we're compressing, what format shall we use?
>>
>> --nocompress-debug-sections
>>
>> # Shorthand for --compress-debug-sections=none
>>
>> --compress-debug-sections-when=never|always|sizewin
>>
>> # Are we compressing debug sections? When?
>> #
>> # - "never" - never, we're not compressing.
>> #
>> # - "always" - always compress, using the format specified by
>> # by --compress-debug-sections (and if that is "none", well,
>> # you still get what you asked for).
>> #
>> # - "sizewin", compress if there's a size win. Like "always", respects
>> # the format specified by --compress-debug-sections.
>>
>>
>> The semantics of mixing these options, or what happens when you repeat them
>> would be obvious in the same way in the other options naming earlier:
>>
>> - You can specify '--compress-debug-sections' multiple times. The last wins.
>>
>> - You can specify '--compress-debug-sections-when' multiple times. The last wins.
>>
>> - Changing '--compress-debug-sections' does not affect the value of '--compress-debug-sections-when'.
>>
>> - Changing '--compress-debug-sections-when' does not affect the value of '--compressed-debug-sections'
>>
>> You end up with two different ways to disable compressing debug sections,
>> but that seems OK to me.
Actually, thinking back, we could just drop --compress-debug-sections-when=never,
it's not needed.
>
> That would be okay with me as well, but (in part to avoid ...
>
>> All that would be left would be bikeshed on the new option name.
>
> ... this aspect) I fail to see why a single option won't do:
>
> --compress-debug-sections=zlib|zstd|no|yes|sizewin
>
> Since the sub-options are distinct sets (alongside "no" one could add "none"
> if wanted, to separate "method: no compression" from "don't compress", which
> I think would then also eliminate the ambiguity referred to earlier on
> [still visible in context above]), within each set your "last wins" would
> apply equally.
Yes. As long as the sets are treated distinct, the same logic can be applied.
> And in fact that was the main reason for my original response
> back to Tom: I view the proliferation of command line options (not just
> here, but in general) as a scalability issue. Therefore my rule of thumb is
> that things which belong together are best grouped under a single top-level
> option.
I don't understand what you mean by scalability issue. Certainly adding more
options to a tool increases the support burden as it adds more combinations
to a support&test matrix. However, in this case, we're adding a new column to the
support&test matrix, regardless of whether we add it as a new option, or as
a distinct set within the preexisting option. The number of supported
combinations is the same regardless.
The separate options approach has the advantage of being clearer and easier to
explain, and doesn't have the "+" vs "," parsing issue.
But the single option with distinct sets also works for me. This seems to
be what Tom ended up with in his latest revision, though I haven't read his
patches.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] gas: Add --force-compress-debug-sections
2023-02-27 23:24 ` Tom de Vries
2023-02-28 0:19 ` Tom de Vries
@ 2023-02-28 13:21 ` Pedro Alves
1 sibling, 0 replies; 24+ messages in thread
From: Pedro Alves @ 2023-02-28 13:21 UTC (permalink / raw)
To: Tom de Vries, Jan Beulich
Cc: binutils, Michael Matz, Nick Clifton, Alan Modra
On 2023-02-27 11:24 p.m., Tom de Vries wrote:
> On 2/27/23 15:07, Jan Beulich wrote:
> OK, so say we have --compress-debug-sections=none|zlib|zstd|no|yes|sizewin.
>
> We have the two existing aliases, --compress-debug-sections and --nocompress-debug-sections, how do we translate those?
>
> Currently --nocompress-debug-sections maps to --compress-debug-sections=none, but I suppose that would have to become --compress-debug-sections=no, otherwise things will get extremely confusing.
Let's drop the new (when) "no" option, as it's not really needed, and rename "yes" -> when-always, and "sizewin" -> "when-always".
So we end up with:
--compress-debug-sections=none|zlib|zstd|when-always|when-sizewin
^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^
group1 group2
And then the "no" in nocompress-debug-sections is no longer ambiguous.
>
> Then --compress-debug-sections currently maps to --compress-debug-sections=zlib. So, should that become --compress-debug-sections=zlib,sizewin? Or just stay --compress-debug-sections=zlib? I'm not sure if we have made things clearer or more orthogonal this way.
I think it's clearer to make it map to whatever are the defaults in all subgroups,
so no subgroup is special cased.
The default compression format is zlib, and the default "when" is "sizewin", so:
"--compress-debug-sections" => "--compress-debug-sections=zlib,sizewin"
And then:
"--compress-debug-sections=always --compress-debug-sections" => "--compress-debug-sections=zlib,sizewin"
>
> I'm starting to wonder if not a simple --compress-debug-sections=heuristic-sizewin|heuristic-always is the solution, in the sense that these two would only influence the heuristics and nothing else, which would work well with the existing options.
I prefer "when" (or no leading prefix) to "heuristic", as "always" is not an heuristic, there's no trial
and error nor is it loosely defined what it does.
But that's a nitpick, it's actually fine with me. FWIW.
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-02-28 13:21 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-23 12:45 [PATCH] gas: Add --force-compress-debug-sections Tom de Vries
2023-02-23 13:08 ` Jan Beulich
2023-02-23 13:27 ` Tom de Vries
2023-02-23 13:44 ` Jan Beulich
2023-02-24 10:52 ` Tom de Vries
2023-02-24 11:28 ` Jan Beulich
2023-02-24 12:21 ` Tom de Vries
2023-02-24 13:23 ` Jan Beulich
2023-02-24 14:11 ` Tom de Vries
2023-02-24 14:26 ` Jan Beulich
2023-02-24 14:57 ` Tom de Vries
2023-02-27 9:03 ` Jan Beulich
2023-02-27 11:43 ` [PATCH] gas: Add --compress-debug-sections=force Tom de Vries
2023-02-27 11:51 ` Jan Beulich
2023-02-27 13:44 ` [PATCH] gas: Add --force-compress-debug-sections Pedro Alves
2023-02-27 14:07 ` Jan Beulich
2023-02-27 23:24 ` Tom de Vries
2023-02-28 0:19 ` Tom de Vries
2023-02-28 13:21 ` Pedro Alves
2023-02-28 12:49 ` Pedro Alves
2023-02-23 15:23 ` Michael Matz
2023-02-23 15:28 ` Tom de Vries
2023-02-23 15:44 ` Michael Matz
2023-02-23 15:46 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).