RFC: Objcopy: Section alignment

RFC: Objcopy: Section alignment

[Nick Clifton]

Hi Guys,

  It was recently pointed out to me that objcopy's --section-alignment
  option does not actually set the alignment of any sections.  It just
  sets a field in the PE file format's optional header.  This is rather
  confusing for the user.

  Plus it turns out that even if a section's alignment is set via the
  --set=section-alignment option, its LMA and VMA will not be changed to
  match the new alignment.

  So I am proposing the patch below to fix these two things.  But I have
  a nagging feeling that there is something that I have missed, so I
  wonder what you guys think.  Any comments ?

Cheers
  Nick

diff --git a/binutils/doc/binutils.texi b/binutils/doc/binutils.texi
index 50cc4707e14..56db97d494b 100644
--- a/binutils/doc/binutils.texi
+++ b/binutils/doc/binutils.texi
@@ -1766,7 +1766,13 @@ SHF_X86_64_LARGE.
 @item --set-section-alignment @var{sectionpattern}=@var{align}
 Set the alignment for any sections matching @var{sectionpattern}.
 @var{align} specifies the alignment in bytes and must be a power of
-two, i.e. 1, 2, 4, 8@dots{}. 
+two, i.e. 1, 2, 4, 8@dots{}.
+
+In addition, for any section matching @var{sectionpattern} its LMA and
+VMA will be increased if this is needed to ensure that it starts on an
+@var{align} boundary.  The exception to this rule is if the section's
+LMA or VMA are being altered explicitly as the result of another
+command line option.
 
 @item --add-section @var{sectionname}=@var{filename}
 Add a new section named @var{sectionname} while copying the file.  The
@@ -2132,6 +2138,11 @@ for dlls.
 Sets the section alignment field in the PE header.  Sections in memory
 will always begin at addresses which are a multiple of this number.
 Defaults to 0x1000.
+
+Also ensures that all sections in the output file are aligned to at
+least @var{num} bytes, unless their LMA or VMA address have been
+explicitly set.
+
 [This option is specific to PE targets.]
 
 @item --stack @var{reserve}
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index a8e0f156f3e..ccf34c09ceb 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4100,6 +4100,24 @@ setup_bfd_headers (bfd *ibfd, bfd *obfd)
   return;
 }
 
+static inline signed int
+power_of_two (bfd_vma val)
+{
+  unsigned int result = 0;
+
+  while ((val & 1) == 0)
+    {
+      val >>= 1;
+      ++result;
+    }
+
+  if (val != 1)
+    /* Number has more than one 1, i.e. wasn't a power of 2.  */
+    return -1;
+
+  return result;
+}
+
 /* Create a section in OBFD with the same
    name and attributes as ISECTION in IBFD.  */
 
@@ -4259,6 +4277,10 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
 			 SECTION_CONTEXT_SET_ALIGNMENT);
   if (p != NULL)
     alignment = p->alignment;
+  else if (pe_section_alignment != (bfd_vma) -1
+	   && bfd_get_flavour (obfd) == bfd_target_coff_flavour
+	   && bfd_pei_p (obfd))
+    alignment = power_of_two (pe_section_alignment);
   else
     alignment = bfd_section_alignment (isection);
 
@@ -4267,6 +4289,30 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
   if (!bfd_set_section_alignment (osection, alignment))
     err = _("failed to set alignment");
 
+  /* If the VMA is not aligned and it was not set by the user
+     then adjust it.  */
+  if (osection->vma & ((1 << alignment) - 1)
+      && change_section_address == 0
+      && find_section_list (bfd_section_name (isection), false,
+			    SECTION_CONTEXT_ALTER_VMA
+			    | SECTION_CONTEXT_SET_VMA) == NULL)
+    {
+      osection->vma += (1 << alignment);
+      osection->vma &= ~ ((1 << alignment) -1);
+    }
+
+  /* If the LMA is not aligned and it was not set by the user
+     then adjust it.  */
+  if (osection->lma & ((1 << alignment) - 1)
+      && change_section_address == 0
+      && find_section_list (bfd_section_name (isection), false,
+			    SECTION_CONTEXT_ALTER_LMA
+			    | SECTION_CONTEXT_SET_LMA) == NULL)
+    {
+      osection->lma += (1 << alignment);
+      osection->lma &= ~ ((1 << alignment) -1);
+    }
+
   /* Copy merge entity size.  */
   osection->entsize = isection->entsize;
 
@@ -5713,15 +5759,8 @@ copy_main (int argc, char *argv[])
 	      fatal (_("bad format for --set-section-alignment: numeric argument needed"));
 
 	    /* Convert integer alignment into a power-of-two alignment.  */
-	    palign = 0;
-	    while ((align & 1) == 0)
-	      {
-	    	align >>= 1;
-	    	++palign;
-	      }
-
-	    if (align != 1)
-	      /* Number has more than on 1, i.e. wasn't a power of 2.  */
+	    palign = power_of_two (align);
+	    if (palign == -1)
 	      fatal (_("bad format for --set-section-alignment: alignment is not a power of two"));
 
 	    /* Add the alignment setting to the section list.  */
@@ -5938,6 +5977,11 @@ copy_main (int argc, char *argv[])
 	case OPTION_PE_SECTION_ALIGNMENT:
 	  pe_section_alignment = parse_vma (optarg,
 					    "--section-alignment");
+	  if (power_of_two (pe_section_alignment) == -1)
+	    {
+	      non_fatal (_("pe_section_alignment argument is not a power of two: %s - ignoring"), optarg);
+	      pe_section_alignment = (bfd_vma) -1;
+	    }
 	  break;
 
 	case OPTION_SUBSYSTEM:


  

Re: RFC: Objcopy: Section alignment

[Jan Beulich]

On 21.03.2024 15:55, Nick Clifton wrote:
>   It was recently pointed out to me that objcopy's --section-alignment
>   option does not actually set the alignment of any sections.  It just
>   sets a field in the PE file format's optional header.  This is rather
>   confusing for the user.

Hmm. For one --file-alignment would then want similar treatment, but
afaict that might be more difficult to achieve (sections would need
moving around in the file). Plus I seem to vaguely recall that there
wants to be a certain correlation between section and file alignments
of individual sections, which you may break with such an adjustment.

>   Plus it turns out that even if a section's alignment is set via the
>   --set=section-alignment option, its LMA and VMA will not be changed to
>   match the new alignment.

Perhaps do just this change in a first step? Albeit ...

> @@ -4267,6 +4289,30 @@ setup_section (bfd *ibfd, sec_ptr isection, void *obfdarg)
>    if (!bfd_set_section_alignment (osection, alignment))
>      err = _("failed to set alignment");
>  
> +  /* If the VMA is not aligned and it was not set by the user
> +     then adjust it.  */
> +  if (osection->vma & ((1 << alignment) - 1)
> +      && change_section_address == 0
> +      && find_section_list (bfd_section_name (isection), false,
> +			    SECTION_CONTEXT_ALTER_VMA
> +			    | SECTION_CONTEXT_SET_VMA) == NULL)
> +    {
> +      osection->vma += (1 << alignment);
> +      osection->vma &= ~ ((1 << alignment) -1);
> +    }
> +
> +  /* If the LMA is not aligned and it was not set by the user
> +     then adjust it.  */
> +  if (osection->lma & ((1 << alignment) - 1)
> +      && change_section_address == 0
> +      && find_section_list (bfd_section_name (isection), false,
> +			    SECTION_CONTEXT_ALTER_LMA
> +			    | SECTION_CONTEXT_SET_LMA) == NULL)
> +    {
> +      osection->lma += (1 << alignment);
> +      osection->lma &= ~ ((1 << alignment) -1);
> +    }

... by doing this adjustment, aren't you at risk of creating overlapping
sections? Should --set-section-alignment= perhaps be limited to
relocatable files?

Jan

Re: RFC: Objcopy: Section alignment

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

Hi Jan,

>>    It was recently pointed out to me that objcopy's --section-alignment
>>    option does not actually set the alignment of any sections.  It just
>>    sets a field in the PE file format's optional header.  This is rather
>>    confusing for the user.
> 
> Hmm. For one --file-alignment would then want similar treatment, but
> afaict that might be more difficult to achieve (sections would need
> moving around in the file). Plus I seem to vaguely recall that there
> wants to be a certain correlation between section and file alignments
> of individual sections, which you may break with such an adjustment.

Agreed.  I think that I will leave file alignment alone for now.  If necessary
that can be the subject of a future patch.

> ... by doing this adjustment, aren't you at risk of creating overlapping
> sections? Should --set-section-alignment= perhaps be limited to
> relocatable files?

Or better yet - not done at all and just left to the user to fix...

So here is a revised patch that does not change the VMAs or LMAs to match
new section aligments, but instead issues an error message if they do not
match.  It also updates the documentation to warn users about this behaviour
and about the dangers of changing the VMAs of fully linked binaries.

Any further comments ?

Cheers
   Nick


[-- Attachment #2: objcopy-section-align.patch.2 --]
[-- Type: application/x-troff-man, Size: 5500 bytes --]

Re: RFC: Objcopy: Section alignment

[Jan Beulich]

On 26.03.2024 13:44, Nick Clifton wrote:
> Hi Jan,
> 
>>>    It was recently pointed out to me that objcopy's --section-alignment
>>>    option does not actually set the alignment of any sections.  It just
>>>    sets a field in the PE file format's optional header.  This is rather
>>>    confusing for the user.
>>
>> Hmm. For one --file-alignment would then want similar treatment, but
>> afaict that might be more difficult to achieve (sections would need
>> moving around in the file). Plus I seem to vaguely recall that there
>> wants to be a certain correlation between section and file alignments
>> of individual sections, which you may break with such an adjustment.
> 
> Agreed.  I think that I will leave file alignment alone for now.  If necessary
> that can be the subject of a future patch.
> 
>> ... by doing this adjustment, aren't you at risk of creating overlapping
>> sections? Should --set-section-alignment= perhaps be limited to
>> relocatable files?
> 
> Or better yet - not done at all and just left to the user to fix...
> 
> So here is a revised patch that does not change the VMAs or LMAs to match
> new section aligments, but instead issues an error message if they do not
> match.  It also updates the documentation to warn users about this behaviour
> and about the dangers of changing the VMAs of fully linked binaries.
> 
> Any further comments ?

Actually, there are: First, is the last doc hunk still applicable /
appropriately worded? "ensures" in particular I'm not sure about.

Perhaps related to that, the two new diagnostics look more like
errors than warnings to me. I think those ought to be just warnings
(by default at least), especially also with the FIXME for the LMA
related code. Which would in particular mean to not bail early from
setup_section(), and perhaps also to not set "status" to non-zero.

Further, while the new power_of_two() is merely split out code,
that code doesn't (already didn't before) deal with 0 properly. It
would spin indefinitely then afaict, rather than reporting "not a
power of two".

Finally, in the new --section-alignment related diagnostic, wouldn't
you better mention the option name, and not the internal variable's?

Jan

Re: RFC: Objcopy: Section alignment

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

Hi Jan,

>> Any further comments ?
> 
> Actually, there are: First, is the last doc hunk still applicable /
> appropriately worded? "ensures" in particular I'm not sure about.
> 
> Perhaps related to that, the two new diagnostics look more like
> errors than warnings to me. I think those ought to be just warnings
> (by default at least), especially also with the FIXME for the LMA
> related code. Which would in particular mean to not bail early from
> setup_section(), and perhaps also to not set "status" to non-zero.
> 
> Further, while the new power_of_two() is merely split out code,
> that code doesn't (already didn't before) deal with 0 properly. It
> would spin indefinitely then afaict, rather than reporting "not a
> power of two".
> 
> Finally, in the new --section-alignment related diagnostic, wouldn't
> you better mention the option name, and not the internal variable's?

All very good points.  Plus I also realised that I had not included a
testcase to check the new behaviour.  Plus, as it turns out, I was
assuming that calling bfd_set_section_alignment() was enough to set
the alignment for a PE section.  But it seems that this function does
not set the necessary IMAGE_SCN_ALIGN_xxx flag in the section's header.

So here is a v3 patch with all of these bugs corrected.

Cheers
   Nick


[-- Attachment #2: objcopy-section-align.patch.3 --]
[-- Type: application/x-troff-man, Size: 11253 bytes --]

Re: RFC: Objcopy: Section alignment

[Jan Beulich]

On 28.03.2024 14:08, Nick Clifton wrote:
> Hi Jan,
> 
>>> Any further comments ?
>>
>> Actually, there are: First, is the last doc hunk still applicable /
>> appropriately worded? "ensures" in particular I'm not sure about.
>>
>> Perhaps related to that, the two new diagnostics look more like
>> errors than warnings to me. I think those ought to be just warnings
>> (by default at least), especially also with the FIXME for the LMA
>> related code. Which would in particular mean to not bail early from
>> setup_section(), and perhaps also to not set "status" to non-zero.
>>
>> Further, while the new power_of_two() is merely split out code,
>> that code doesn't (already didn't before) deal with 0 properly. It
>> would spin indefinitely then afaict, rather than reporting "not a
>> power of two".
>>
>> Finally, in the new --section-alignment related diagnostic, wouldn't
>> you better mention the option name, and not the internal variable's?
> 
> All very good points.  Plus I also realised that I had not included a
> testcase to check the new behaviour.  Plus, as it turns out, I was
> assuming that calling bfd_set_section_alignment() was enough to set
> the alignment for a PE section.

Which also shouldn't be necessary? Isn't section alignment relevant for
COFF objects only? And didn't Alan, not so long ago, remove its
propagation into PE binaries? (This then also being a reason why the
alignment values weren't dumped for PE.)

Jan

>  But it seems that this function does
> not set the necessary IMAGE_SCN_ALIGN_xxx flag in the section's header.
> 
> So here is a v3 patch with all of these bugs corrected.
> 
> Cheers
>    Nick
> 

Re: RFC: Objcopy: Section alignment

[Nick Clifton]

Hi Jan,

>> Plus, as it turns out, I was
>> assuming that calling bfd_set_section_alignment() was enough to set
>> the alignment for a PE section.
> 
> Which also shouldn't be necessary? Isn't section alignment relevant for
> COFF objects only?

I am not sure.  But if it is not relevant for PE sections then shouldn't
changing the SectionAlignment field in the PE header generate an error if
any section's VMA does not match that alignment ?

> And didn't Alan, not so long ago, remove its
> propagation into PE binaries?

I must admit that I do not remember this, but it certainly could have happened.

> (This then also being a reason why the
> alignment values weren't dumped for PE.)

I think that we should not hide the alignment values from the user, even if
they are not going to be honoured.  We do not dictate the policy for the how
PE binaries are going to be treated by the OS, and there could be good
reasons for why a user needs to know them.

Cheers
   Nick

Re: RFC: Objcopy: Section alignment

[Jan Beulich]

On 02.04.2024 10:45, Nick Clifton wrote:
>>> Plus, as it turns out, I was
>>> assuming that calling bfd_set_section_alignment() was enough to set
>>> the alignment for a PE section.
>>
>> Which also shouldn't be necessary? Isn't section alignment relevant for
>> COFF objects only?
> 
> I am not sure.  But if it is not relevant for PE sections then shouldn't
> changing the SectionAlignment field in the PE header generate an error if
> any section's VMA does not match that alignment ?

Well, this I'm unsure about, largely because of not really knowing any good
spec for PE/COFF (which would cover such details).

>> And didn't Alan, not so long ago, remove its
>> propagation into PE binaries?
> 
> I must admit that I do not remember this, but it certainly could have happened.

Commit 6f8f6017a0c4 ("PR27567, Linking PE files adds alignment section flags
to executables"), to deal with PR 27567. But indeed ...

>> (This then also being a reason why the
>> alignment values weren't dumped for PE.)
> 
> I think that we should not hide the alignment values from the user, even if
> they are not going to be honoured.  We do not dictate the policy for the how
> PE binaries are going to be treated by the OS, and there could be good
> reasons for why a user needs to know them.

... I was mis-remembering that said change would also have removed the
dumping that you now add. In principle I'm okay with such dumping being
added; I merely would have been puzzled if it was re-added after having
been removed.

Jan