gas: Add --force-compress-debug-sections

  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(-)
  

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
  

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.)
  

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
  

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.
  

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
  

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.
  

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
  

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
  

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
  

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
  

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
  

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.
  

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
  

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
  

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
  

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
>
  

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
  

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
  

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
  

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.
  

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.