Re: [PATCH] gas: Add --force-compress-debug-sections

[Pedro Alves <pedro@palves.net>]

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.