RFC: Objcopy: Section alignment

  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
  

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
  

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
  

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
  

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
  

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
>
  

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
  

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