[PATCH v2 5/5] ld: clarify comments on /DISCARD/ output section behavior

[Matthieu Longo]

On 2025-10-16 07:42, Jan Beulich wrote:
> On 15.10.2025 17:53, Matthieu Longo wrote:
>> On 2025-10-15 14:13, Jan Beulich wrote:
>>> On 14.10.2025 12:46, Matthieu Longo wrote:
>>>> --- a/ld/ldlang.c
>>>> +++ b/ld/ldlang.c
>>>> @@ -2709,17 +2709,25 @@ wont_add_section_p (asection *section,
>>>>    
>>>>      if (discard)
>>>>        {
>>>> +      /* /DISCARD/ is seen first and the top-most clause has precedence on the
>>>> +	 next ones, thus the section will be dropped.  No need to warn about
>>>> +	 potential change in behavior with non-contiguous regions when the
>>>> +	 section is already dropped.  */
>>>>          if (section->output_section == NULL)
>>>>    	{
>>>>    	  /* This prevents future calls from assigning this section or
>>>>    	     warning about it again.  */
>>>>    	  section->output_section = bfd_abs_section_ptr;
>>>>    	}
>>>> -      else if (bfd_is_abs_section (section->output_section))
>>>> -	;
>>>> -      else if (link_info.non_contiguous_regions_warnings)
>>>> +      /* The /DISCARD/ clause appears after previous ones which assigned the
>>>> +	 input section to an output section.  /DISCARD/ does not have the
>>>> +	 precedence, so the section will be kept.  */
>>>> +      else if (! bfd_is_abs_section (section->output_section)
>>>> +	       && link_info.non_contiguous_regions_warnings)
>>>>    	einfo (_("%P:%pS: warning: --enable-non-contiguous-regions makes "
>>>> -		 "section `%pA' from `%pB' match /DISCARD/ clause.\n"),
>>>> +		 "section `%pA' from `%pB' match /DISCARD/ clause.  If the "
>>>> +		 "section can be assigned to an output section, it won't be "
>>>> +		 "discarded.\n"),
>>>
>>> Besides this getting excessively long for a single line, to me comment and
>>> message text also read partially contradictory: The comment says "will",
>>> while the diagnostic effectively says "may". Which one is it?
>>
>> If I understood you well, you say that the diagnostic message
>> contradicts the following explanation from the commit message:
>>   > - If /DISCARD/ follows clauses that assign the input section to an output
>>   >   section, /DISCARD/ is ignored, and the section is kept. In this case,
>>   >   the linker must warn that the section *may* not be discarded.
> 
> No, my remark was about the code comment (see context above). Still ...
> 

What about this ? Is it clearer ?

+      /* The /DISCARD/ clause follows clauses that assign the input 
section to
+        an output section. Since /DISCARD/ does not have the precedence,
+        /DISCARD/ is ignored.
+        1. If the input section can be assigned to an output section,
+           the link will succeed. The warning below is emitted with
+           --enable-non-contiguous-regions-warnings so that the user can
+           notice that /DISCARD/ did not do what he might have expected,
+           i.e. discarding the input section.
+        2. If the input section cannot be assigned, the link will fail
+           with an error. The warning below is emitted with
+           --enable-non-contiguous-regions-warnings so that the user can
+           notice that /DISCARD/ was ignored for this input section, then
+           leading to a link failure caused by not enough space in the 
output
+           section for the input section.  */
+      else if (! bfd_is_abs_section (section->output_section)
+              && link_info.non_contiguous_regions_warnings)

>> What I meant is the same thing. The phrasing likely needs improvement.
>> I would like to keep the comment in the code as it is, it seems clear
>> enough to me.
>> What about the below for the commit message ?
>>
>> If /DISCARD/ follows clauses that assign the input section to an output
>> section, /DISCARD/ is ignored. If the input section can't be assigned to
>> the output section for a later reason, an error will be raised.
>> Otherwise the input section will be assigned as intended to an output
>> section specified by one of the matching clauses previous to /DISCARD/.
> 
> ... I also view this as an improvement for the commit message.

Fixed in the next revision.

> 
> Jan

Matthieu