diff mbox series

[v1,5/5] ld: clarify comments on /DISCARD/ output section behavior

Message ID 20250918150606.1630916-6-matthieu.longo@arm.com
State New
Headers
Series ld: fix segfaults when non-contiguous memory support is enabled |

Commit Message

Matthieu Longo Sept. 18, 2025, 3:06 p.m. UTC 
  The previous comments made it difficult to understand how the /DISCARD/
output section interacts with non-contiguous regions.

In summary, the general rule is that the first (top-most) clause takes
precedence over subsequent ones:
- If /DISCARD/ appears first, the section is dropped. There is no need
  to warn about potential behavior changes with non-contiguous regions
  when the section is already discarded.
- 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.
---
 ld/ldlang.c                                     | 17 ++++++++++++-----
 .../non-contiguous-mem/non-contiguous-ok-5.warn |  4 ++--
 2 files changed, 14 insertions(+), 7 deletions(-)

Comments

Jan Beulich Sept. 26, 2025, 1:19 p.m. UTC | #1 
On 18.09.2025 17:06, Matthieu Longo wrote:
> The previous comments made it difficult to understand how the /DISCARD/
> output section interacts with non-contiguous regions.
> 
> In summary, the general rule is that the first (top-most) clause takes
> precedence over subsequent ones:
> - If /DISCARD/ appears first, the section is dropped. There is no need
>   to warn about potential behavior changes with non-contiguous regions
>   when the section is already discarded.
> - 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.

Looks okay to me, just that again ...

> --- a/ld/ldlang.c
> +++ b/ld/ldlang.c
> @@ -2709,18 +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"),
> -	       NULL, section, section->owner);
> +		 "section `%pA' from `%pB' match /DISCARD/ clause.  If the "
> +		 "section can be assigned to an output section, it won't be "
> +		 "discarded.\n"), NULL, section, section->owner);

... original line splitting style wants retaining here.

Jan
Matthieu Longo Oct. 13, 2025, 3:24 p.m. UTC | #2 
On 2025-09-26 14:19, Jan Beulich wrote:
> On 18.09.2025 17:06, Matthieu Longo wrote:
>> The previous comments made it difficult to understand how the /DISCARD/
>> output section interacts with non-contiguous regions.
>>
>> In summary, the general rule is that the first (top-most) clause takes
>> precedence over subsequent ones:
>> - If /DISCARD/ appears first, the section is dropped. There is no need
>>    to warn about potential behavior changes with non-contiguous regions
>>    when the section is already discarded.
>> - 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.
> 
> Looks okay to me, just that again ...
> 
>> --- a/ld/ldlang.c
>> +++ b/ld/ldlang.c
>> @@ -2709,18 +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"),
>> -	       NULL, section, section->owner);
>> +		 "section `%pA' from `%pB' match /DISCARD/ clause.  If the "
>> +		 "section can be assigned to an output section, it won't be "
>> +		 "discarded.\n"), NULL, section, section->owner);
> 
> ... original line splitting style wants retaining here.
> 
> Jan


Fixed in the next revision.

Matthieu
diff mbox series

Patch

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 454887d71f4..75cee7d605e 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -2709,18 +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"),
-	       NULL, section, section->owner);
+		 "section `%pA' from `%pB' match /DISCARD/ clause.  If the "
+		 "section can be assigned to an output section, it won't be "
+		 "discarded.\n"), NULL, section, section->owner);
 
       return true;
     }
diff --git a/ld/testsuite/ld-aarch64/non-contiguous-mem/non-contiguous-ok-5.warn b/ld/testsuite/ld-aarch64/non-contiguous-mem/non-contiguous-ok-5.warn
index 730599ef289..7cf8af73a56 100644
--- a/ld/testsuite/ld-aarch64/non-contiguous-mem/non-contiguous-ok-5.warn
+++ b/ld/testsuite/ld-aarch64/non-contiguous-mem/non-contiguous-ok-5.warn
@@ -2,8 +2,8 @@ 
 .*: warning: .* may change behaviour for section .\.code\.2. from .* \(assigned to \.raml, but additional match: \.ramu\)
 .*: warning: .* may change behaviour for section .\.code\.3. from .* \(assigned to \.raml, but additional match: \.ramu\)
 .*: warning: .* may change behaviour for section .\.code\.4. from .* \(assigned to \.raml, but additional match: \.ramu\)
-.*: warning: .* makes section .\.code\.2. from .* match /DISCARD/ clause\.
-.*: warning: .* makes section .\.code\.4. from .* match /DISCARD/ clause\.
+.*: warning: .* makes section .\.code\.2. from .* match /DISCARD/ clause\.  If the section can be assigned to an output section, it won't be discarded\.
+.*: warning: .* makes section .\.code\.4. from .* match /DISCARD/ clause\.  If the section can be assigned to an output section, it won't be discarded\.
 .*: warning: .* may change behaviour for section .\.code\.1. from .* \(assigned to \.ramu, but additional match: \.ramz\)
 .*: warning: .* may change behaviour for section .\.code\.2. from .* \(assigned to \.ramu, but additional match: \.ramz\)
 .*: warning: .* may change behaviour for section .\.code\.3. from .* \(assigned to \.ramu, but additional match: \.ramz\)