gold: Don't combine sections with non-zero/zero flags

Message ID 20190624195335.29719-1-hjl.tools@gmail.com
State New
Headers show
Series
  • gold: Don't combine sections with non-zero/zero flags
Related show

Commit Message

H.J. Lu June 24, 2019, 7:53 p.m.
Gold shouldn't combine sections with non-zero flags and zero flags.

	PR gold/17556
	* layout.cc (Layout::get_output_section): Don't combine sections
	with non-zero flags and zero flags.
---
 gold/layout.cc | 10 ----------
 1 file changed, 10 deletions(-)

-- 
2.20.1

Comments

Cary Coutant June 28, 2019, 5:16 p.m. | #1
> Gold shouldn't combine sections with non-zero flags and zero flags.

>

>         PR gold/17556

>         * layout.cc (Layout::get_output_section): Don't combine sections

>         with non-zero flags and zero flags.


Why shouldn't we just remove both parts of this logic? I.e., the
following patch:

diff --git a/gold/layout.cc b/gold/layout.cc
index b83e8e6e2d..5a0d0137ef 100644
--- a/gold/layout.cc
+++ b/gold/layout.cc
@@ -848,40 +848,9 @@ Layout::get_output_section(const char* name,
Stringpool::Key name_key,
   else
     {
       // This is the first time we've seen this name/type/flags
-      // combination.  For compatibility with the GNU linker, we
-      // combine sections with contents and zero flags with sections
-      // with non-zero flags.  This is a workaround for cases where
-      // assembler code forgets to set section flags.  FIXME: Perhaps
-      // there should be an option to control this.
-      Output_section* os = NULL;
-
-      if (lookup_type == elfcpp::SHT_PROGBITS)
-       {
-         if (flags == 0)
-           {
-             Output_section* same_name = this->find_output_section(name);
-             if (same_name != NULL
-                 && (same_name->type() == elfcpp::SHT_PROGBITS
-                     || same_name->type() == elfcpp::SHT_INIT_ARRAY
-                     || same_name->type() == elfcpp::SHT_FINI_ARRAY
-                     || same_name->type() == elfcpp::SHT_PREINIT_ARRAY)
-                 && (same_name->flags() & elfcpp::SHF_TLS) == 0)
-               os = same_name;
-           }
-         else if ((flags & elfcpp::SHF_TLS) == 0)
-           {
-             elfcpp::Elf_Xword zero_flags = 0;
-             const Key zero_key(name_key, std::make_pair(lookup_type,
-                                                         zero_flags));
-             Section_name_map::iterator p =
-                 this->section_name_map_.find(zero_key);
-             if (p != this->section_name_map_.end())
-               os = p->second;
-           }
-       }
-
-      if (os == NULL)
-       os = this->make_output_section(name, type, flags, order, is_relro);
+      // combination.
+      Output_section* os =
+         this->make_output_section(name, type, flags, order, is_relro);

       ins.first->second = os;
       return os;

-cary
H.J. Lu June 28, 2019, 5:26 p.m. | #2
On Fri, Jun 28, 2019 at 10:16 AM Cary Coutant <ccoutant@gmail.com> wrote:
>

> > Gold shouldn't combine sections with non-zero flags and zero flags.

> >

> >         PR gold/17556

> >         * layout.cc (Layout::get_output_section): Don't combine sections

> >         with non-zero flags and zero flags.

>

> Why shouldn't we just remove both parts of this logic? I.e., the

> following patch:

>

> diff --git a/gold/layout.cc b/gold/layout.cc

> index b83e8e6e2d..5a0d0137ef 100644

> --- a/gold/layout.cc

> +++ b/gold/layout.cc

> @@ -848,40 +848,9 @@ Layout::get_output_section(const char* name,

> Stringpool::Key name_key,

>    else

>      {

>        // This is the first time we've seen this name/type/flags

> -      // combination.  For compatibility with the GNU linker, we

> -      // combine sections with contents and zero flags with sections

> -      // with non-zero flags.  This is a workaround for cases where

> -      // assembler code forgets to set section flags.  FIXME: Perhaps

> -      // there should be an option to control this.

> -      Output_section* os = NULL;

> -

> -      if (lookup_type == elfcpp::SHT_PROGBITS)

> -       {

> -         if (flags == 0)

> -           {

> -             Output_section* same_name = this->find_output_section(name);

> -             if (same_name != NULL

> -                 && (same_name->type() == elfcpp::SHT_PROGBITS

> -                     || same_name->type() == elfcpp::SHT_INIT_ARRAY

> -                     || same_name->type() == elfcpp::SHT_FINI_ARRAY

> -                     || same_name->type() == elfcpp::SHT_PREINIT_ARRAY)

> -                 && (same_name->flags() & elfcpp::SHF_TLS) == 0)

> -               os = same_name;

> -           }

> -         else if ((flags & elfcpp::SHF_TLS) == 0)

> -           {

> -             elfcpp::Elf_Xword zero_flags = 0;

> -             const Key zero_key(name_key, std::make_pair(lookup_type,

> -                                                         zero_flags));

> -             Section_name_map::iterator p =

> -                 this->section_name_map_.find(zero_key);

> -             if (p != this->section_name_map_.end())

> -               os = p->second;

> -           }

> -       }

> -

> -      if (os == NULL)

> -       os = this->make_output_section(name, type, flags, order, is_relro);

> +      // combination.

> +      Output_section* os =

> +         this->make_output_section(name, type, flags, order, is_relro);

>

>        ins.first->second = os;

>        return os;

>


This should work.


-- 
H.J.

Patch

diff --git a/gold/layout.cc b/gold/layout.cc
index 73642b6b61..52908e92c1 100644
--- a/gold/layout.cc
+++ b/gold/layout.cc
@@ -868,16 +868,6 @@  Layout::get_output_section(const char* name, Stringpool::Key name_key,
 		  && (same_name->flags() & elfcpp::SHF_TLS) == 0)
 		os = same_name;
 	    }
-	  else if ((flags & elfcpp::SHF_TLS) == 0)
-	    {
-	      elfcpp::Elf_Xword zero_flags = 0;
-	      const Key zero_key(name_key, std::make_pair(lookup_type,
-							  zero_flags));
-	      Section_name_map::iterator p =
-		  this->section_name_map_.find(zero_key);
-	      if (p != this->section_name_map_.end())
-		os = p->second;
-	    }
 	}
 
       if (os == NULL)