[PATCH] gold: Place .note.gnu.property section before other note sections

[PATCH] gold: Place .note.gnu.property section before other note sections

[H.J. Lu]

Place the .note.gnu.property section before all other note sections to
avoid being placed between other note sections with different alignments.

	PR gold/28494
	* layout.cc (Layout::create_note): Set order to ORDER_PROPERTY_NOTE
	for the .note.gnu.property section.
	* layout.h (Output_section_order): Add ORDER_PROPERTY_NOTE.
---
 gold/layout.cc | 3 ++-
 gold/layout.h  | 9 +++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/gold/layout.cc b/gold/layout.cc
index a27cb071c75..38e9bceec7e 100644
--- a/gold/layout.cc
+++ b/gold/layout.cc
@@ -3245,7 +3245,8 @@ Layout::create_note(const char* name, int note_type,
   if (allocate)
     {
       flags = elfcpp::SHF_ALLOC;
-      order = ORDER_RO_NOTE;
+      order = (note_type == elfcpp::NT_GNU_PROPERTY_TYPE_0
+	       ?  ORDER_PROPERTY_NOTE : ORDER_RO_NOTE);
     }
   Output_section* os = this->choose_output_section(NULL, section_name,
 						   elfcpp::SHT_NOTE,
diff --git a/gold/layout.h b/gold/layout.h
index 0b378003679..05c31714e47 100644
--- a/gold/layout.h
+++ b/gold/layout.h
@@ -399,8 +399,13 @@ enum Output_section_order
   // linker can pick it up quickly.
   ORDER_INTERP,
 
-  // Loadable read-only note sections come next so that the PT_NOTE
-  // segment is on the first page of the executable.
+  // The .note.gnu.property section comes next so that the PT_NOTE
+  // segment is on the first page of the executable and it won't be
+  // placed between other note sections with different alignments.
+  ORDER_PROPERTY_NOTE,
+
+  // Loadable read-only note sections come after the .note.gnu.property
+  // section.
   ORDER_RO_NOTE,
 
   // Put read-only sections used by the dynamic linker early in the
-- 
2.32.0

Re: [PATCH] gold: Place .note.gnu.property section before other note sections

[Fangrui Song]

On Mon, Oct 25, 2021 at 8:51 AM H.J. Lu via Binutils
<binutils@sourceware.org> wrote:
>
> Place the .note.gnu.property section before all other note sections to
> avoid being placed between other note sections with different alignments.
>
>         PR gold/28494
>         * layout.cc (Layout::create_note): Set order to ORDER_PROPERTY_NOTE
>         for the .note.gnu.property section.
>         * layout.h (Output_section_order): Add ORDER_PROPERTY_NOTE.
> ---
>  gold/layout.cc | 3 ++-
>  gold/layout.h  | 9 +++++++--
>  2 files changed, 9 insertions(+), 3 deletions(-)

I think the order should be agnostic. The linker should just apply
usual heuristics: output sections are ordered by first-seen member
input section.

Then I use this strategy for LLD: SHT_NOTE sections with different
alignments belong to different PT_NOTE.
https://reviews.llvm.org/D61296


> diff --git a/gold/layout.cc b/gold/layout.cc
> index a27cb071c75..38e9bceec7e 100644
> --- a/gold/layout.cc
> +++ b/gold/layout.cc
> @@ -3245,7 +3245,8 @@ Layout::create_note(const char* name, int note_type,
>    if (allocate)
>      {
>        flags = elfcpp::SHF_ALLOC;
> -      order = ORDER_RO_NOTE;
> +      order = (note_type == elfcpp::NT_GNU_PROPERTY_TYPE_0
> +              ?  ORDER_PROPERTY_NOTE : ORDER_RO_NOTE);
>      }
>    Output_section* os = this->choose_output_section(NULL, section_name,
>                                                    elfcpp::SHT_NOTE,
> diff --git a/gold/layout.h b/gold/layout.h
> index 0b378003679..05c31714e47 100644
> --- a/gold/layout.h
> +++ b/gold/layout.h
> @@ -399,8 +399,13 @@ enum Output_section_order
>    // linker can pick it up quickly.
>    ORDER_INTERP,
>
> -  // Loadable read-only note sections come next so that the PT_NOTE
> -  // segment is on the first page of the executable.
> +  // The .note.gnu.property section comes next so that the PT_NOTE
> +  // segment is on the first page of the executable and it won't be
> +  // placed between other note sections with different alignments.
> +  ORDER_PROPERTY_NOTE,
> +
> +  // Loadable read-only note sections come after the .note.gnu.property
> +  // section.
>    ORDER_RO_NOTE,
>
>    // Put read-only sections used by the dynamic linker early in the
> --
> 2.32.0
>

Re: [PATCH] gold: Place .note.gnu.property section before other note sections

[H.J. Lu]

On Mon, Oct 25, 2021 at 1:53 PM Fangrui Song <i@maskray.me> wrote:
>
> On Mon, Oct 25, 2021 at 8:51 AM H.J. Lu via Binutils
> <binutils@sourceware.org> wrote:
> >
> > Place the .note.gnu.property section before all other note sections to
> > avoid being placed between other note sections with different alignments.
> >
> >         PR gold/28494
> >         * layout.cc (Layout::create_note): Set order to ORDER_PROPERTY_NOTE
> >         for the .note.gnu.property section.
> >         * layout.h (Output_section_order): Add ORDER_PROPERTY_NOTE.
> > ---
> >  gold/layout.cc | 3 ++-
> >  gold/layout.h  | 9 +++++++--
> >  2 files changed, 9 insertions(+), 3 deletions(-)
>
> I think the order should be agnostic. The linker should just apply
> usual heuristics: output sections are ordered by first-seen member
> input section.

Gold orders output sections with by Output_section_order:

// Add an Output_section to a PT_LOAD Output_segment.

void
Output_segment::add_output_section_to_load(Layout* layout,
                                           Output_section* os,
                                           elfcpp::Elf_Word seg_flags)
{
  gold_assert(this->type() == elfcpp::PT_LOAD);
  gold_assert((os->flags() & elfcpp::SHF_ALLOC) != 0);
  gold_assert(!this->is_max_align_known_);
  gold_assert(os->is_large_data_section() == this->is_large_data_segment());

  this->update_flags_for_output_section(seg_flags);

  // We don't want to change the ordering if we have a linker script
  // with a SECTIONS clause.
  Output_section_order order = os->order();
  if (layout->script_options()->saw_sections_clause())
    order = static_cast<Output_section_order>(0);
  else
    gold_assert(order != ORDER_INVALID);

  this->output_lists_[order].push_back(os);
 }

Add ORDER_PROPERTY_NOTE is the simplest way to deal
with it.

> Then I use this strategy for LLD: SHT_NOTE sections with different
> alignments belong to different PT_NOTE.
> https://reviews.llvm.org/D61296
>
>
> > diff --git a/gold/layout.cc b/gold/layout.cc
> > index a27cb071c75..38e9bceec7e 100644
> > --- a/gold/layout.cc
> > +++ b/gold/layout.cc
> > @@ -3245,7 +3245,8 @@ Layout::create_note(const char* name, int note_type,
> >    if (allocate)
> >      {
> >        flags = elfcpp::SHF_ALLOC;
> > -      order = ORDER_RO_NOTE;
> > +      order = (note_type == elfcpp::NT_GNU_PROPERTY_TYPE_0
> > +              ?  ORDER_PROPERTY_NOTE : ORDER_RO_NOTE);
> >      }
> >    Output_section* os = this->choose_output_section(NULL, section_name,
> >                                                    elfcpp::SHT_NOTE,
> > diff --git a/gold/layout.h b/gold/layout.h
> > index 0b378003679..05c31714e47 100644
> > --- a/gold/layout.h
> > +++ b/gold/layout.h
> > @@ -399,8 +399,13 @@ enum Output_section_order
> >    // linker can pick it up quickly.
> >    ORDER_INTERP,
> >
> > -  // Loadable read-only note sections come next so that the PT_NOTE
> > -  // segment is on the first page of the executable.
> > +  // The .note.gnu.property section comes next so that the PT_NOTE
> > +  // segment is on the first page of the executable and it won't be
> > +  // placed between other note sections with different alignments.
> > +  ORDER_PROPERTY_NOTE,
> > +
> > +  // Loadable read-only note sections come after the .note.gnu.property
> > +  // section.
> >    ORDER_RO_NOTE,
> >
> >    // Put read-only sections used by the dynamic linker early in the
> > --
> > 2.32.0
> >



-- 
H.J.

Re: [PATCH] gold: Place .note.gnu.property section before other note sections

[Cary Coutant]

>         PR gold/28494
>         * layout.cc (Layout::create_note): Set order to ORDER_PROPERTY_NOTE
>         for the .note.gnu.property section.
>         * layout.h (Output_section_order): Add ORDER_PROPERTY_NOTE.

I'd prefer something more general, but it would be considerably more
involved. Ideally, we'd recognize which sections would be broken by
internal padding (like SHT_NOTE sections, but also perhaps others),
and we could set the "must sort attached input sections" flag, then
sort the input sections so that 8-byte-aligned sections always come
first.

This is OK, but it's a reluctant OK.

I'm telling myself that this treatment is more compatible with the
notion that these particular SHT_NOTE sections really ought to be
SHT_GNU_PROPERTY sections, and would go into a PT_GNU_PROPERTY
segment. Except for the SHT_ and PT_ names, this is probably how we
would have handled it in that alternate universe.

-cary

Re: [PATCH] gold: Place .note.gnu.property section before other note sections

[Cary Coutant]

> I'd prefer something more general, but it would be considerably more
> involved. Ideally, we'd recognize which sections would be broken by
> internal padding (like SHT_NOTE sections, but also perhaps others),
> and we could set the "must sort attached input sections" flag, then
> sort the input sections so that 8-byte-aligned sections always come
> first.
>
> This is OK, but it's a reluctant OK.

Upon further reflection, I realize that the "more general" approach I
was describing would only apply for mixed alignments of input sections
within one output section, whereas what we have here is multiple
output sections that get grouped into one segment. Thus, using a
separate ORDER bucket really is the right approach. Sorry; I withdraw
the "reluctant" bit.

-cary

Re: [PATCH] gold: Place .note.gnu.property section before other note sections

[H.J. Lu]

On Tue, Oct 26, 2021 at 4:40 PM Cary Coutant <ccoutant@gmail.com> wrote:
>
> > I'd prefer something more general, but it would be considerably more
> > involved. Ideally, we'd recognize which sections would be broken by
> > internal padding (like SHT_NOTE sections, but also perhaps others),
> > and we could set the "must sort attached input sections" flag, then
> > sort the input sections so that 8-byte-aligned sections always come
> > first.
> >
> > This is OK, but it's a reluctant OK.
>
> Upon further reflection, I realize that the "more general" approach I
> was describing would only apply for mixed alignments of input sections
> within one output section, whereas what we have here is multiple
> output sections that get grouped into one segment. Thus, using a
> separate ORDER bucket really is the right approach. Sorry; I withdraw
> the "reluctant" bit.
>
> -cary

Thanks for clarification.

-- 
H.J.

Re: [PATCH] gold: Place .note.gnu.property section before other note sections

[H.J. Lu]

On Tue, Oct 26, 2021 at 4:42 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Oct 26, 2021 at 4:40 PM Cary Coutant <ccoutant@gmail.com> wrote:
> >
> > > I'd prefer something more general, but it would be considerably more
> > > involved. Ideally, we'd recognize which sections would be broken by
> > > internal padding (like SHT_NOTE sections, but also perhaps others),
> > > and we could set the "must sort attached input sections" flag, then
> > > sort the input sections so that 8-byte-aligned sections always come
> > > first.
> > >
> > > This is OK, but it's a reluctant OK.
> >
> > Upon further reflection, I realize that the "more general" approach I
> > was describing would only apply for mixed alignments of input sections
> > within one output section, whereas what we have here is multiple
> > output sections that get grouped into one segment. Thus, using a
> > separate ORDER bucket really is the right approach. Sorry; I withdraw
> > the "reluctant" bit.
> >
> > -cary
>
> Thanks for clarification.

I am backporting it to binutils 2.37 branch.


-- 
H.J.

Re: [PATCH] gold: Place .note.gnu.property section before other note sections

[Ian Lance Taylor]

On Tue, Oct 26, 2021 at 4:09 PM Cary Coutant via Binutils
<binutils@sourceware.org> wrote:
>
> >         PR gold/28494
> >         * layout.cc (Layout::create_note): Set order to ORDER_PROPERTY_NOTE
> >         for the .note.gnu.property section.
> >         * layout.h (Output_section_order): Add ORDER_PROPERTY_NOTE.
>
> I'd prefer something more general, but it would be considerably more
> involved. Ideally, we'd recognize which sections would be broken by
> internal padding (like SHT_NOTE sections, but also perhaps others),
> and we could set the "must sort attached input sections" flag, then
> sort the input sections so that 8-byte-aligned sections always come
> first.
>
> This is OK, but it's a reluctant OK.
>
> I'm telling myself that this treatment is more compatible with the
> notion that these particular SHT_NOTE sections really ought to be
> SHT_GNU_PROPERTY sections, and would go into a PT_GNU_PROPERTY
> segment. Except for the SHT_ and PT_ names, this is probably how we
> would have handled it in that alternate universe.

I think the problem with the approach here is that it only works for
one particular note section.  Other note sections can introduce
similar problems.  Given the current handling of p_align in PT_NOTE
segments, I think we need to separate SHT_NOTE sections with 4 byte
alignment and SHT_NOTE sections with 8 byte alignment.  That is, I
think this patch is a step in the right direction, but it keys off the
wrong thing.

Right now if we see a SHT_NOTE section with 4 byte alignment, then one
with 8 byte alignment, then one with 4 byte alignment, we can wind up
with overlapping PT_NOTE segments.  And we won't necessarily be able
to parse the notes in the 4 byte PT_NOTE segment, because it will
depend on the position of the interior SHT_NOTE sections with 8 byte
alignment.

Ian