[patch][gold] Fix case where a empty section defines the segment of a following section

[patch][gold] Fix case where a empty section defines the segment of a following section

[Rafael Espindola]

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

The attached patch fixes the case where a linker script defines the
segment a section should be in (.init in the test), but for a particular input
that section is empty.

Without this patch Output_section_definition::allocate_to_segment will not
set phdrs_list and the specified segment will not be used in the following
sections (.text in the test).

2010-05-17  Rafael Espindola  <espindola@google.com>

	* script-sections.cc (Output_section_definition::allocate_to_segment):
	Update the phdrs_list even when if the output section is NULL.
	* testsuite/Makefile.am: Add test.
	* testsuite/Makefile.in: Regenerate.
	* testsuite/script_test_9.cc: New.
	* testsuite/script_test_9.sh: New.
	* testsuite/script_test_9.t: New.

Cheers,
-- 
Rafael Ávila de Espíndola

[-- Attachment #2: script.patch --]
[-- Type: text/x-diff, Size: 6298 bytes --]

diff --git a/gold/script-sections.cc b/gold/script-sections.cc
index b4cbb40..6531600 100644
--- a/gold/script-sections.cc
+++ b/gold/script-sections.cc
@@ -2161,13 +2161,16 @@ Output_section*
 Output_section_definition::allocate_to_segment(String_list** phdrs_list,
 					       bool* orphan)
 {
+  // Update phdrs_list even if we don't have an output section. It
+  // might be used by the following sections.
+  if (this->phdrs_ != NULL)
+    *phdrs_list = this->phdrs_;
+
   if (this->output_section_ == NULL)
     return NULL;
   if ((this->output_section_->flags() & elfcpp::SHF_ALLOC) == 0)
     return NULL;
   *orphan = false;
-  if (this->phdrs_ != NULL)
-    *phdrs_list = this->phdrs_;
   return this->output_section_;
 }
 
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 435eeab..9a91995 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -1167,6 +1167,14 @@ script_test_8: basic_test.o gcctestdir/ld $(srcdir)/script_test_7.t
 script_test_8.stdout: script_test_8
 	$(TEST_READELF) -SlW script_test_8 > script_test_8.stdout
 
+
+check_SCRIPTS += script_test_9.sh
+check_DATA += script_test_9
+script_test_9.o: script_test_9.cc
+	$(CXXCOMPILE) -O0 -c -o $@ $<
+script_test_9: gcctestdir/ld $(srcdir)/script_test_9.t script_test_9.o
+	$(CXXLINK) -Bgcctestdir/ script_test_9.o -T $(srcdir)/script_test_9.t
+
 # Test --dynamic-list, --dynamic-list-data, --dynamic-list-cpp-new,
 # and --dynamic-list-cpp-typeinfo
 
diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in
index 968d010..eb93d47 100644
--- a/gold/testsuite/Makefile.in
+++ b/gold/testsuite/Makefile.in
@@ -75,7 +75,8 @@ check_PROGRAMS = object_unittest$(EXEEXT) binary_unittest$(EXEEXT) \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_5.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_6.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_7.sh \
-@GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_8.sh dynamic_list.sh
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_8.sh \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_9.sh dynamic_list.sh
 
 # Create the data files that debug_msg.sh analyzes.
 
@@ -110,6 +111,7 @@ check_PROGRAMS = object_unittest$(EXEEXT) binary_unittest$(EXEEXT) \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_6.stdout \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_7.stdout \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_8.stdout \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_9 \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	dynamic_list.stdout
 @GCC_TRUE@@NATIVE_LINKER_TRUE@am__append_3 = incremental_test \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	gc_comdat_test gc_tls_test \
@@ -2967,6 +2969,10 @@ uninstall-am:
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	-Wl,-Ttext=0x20001000 -Wl,-Tdata=0x20200000 -Wl,-Tbss=0x20400000
 @GCC_TRUE@@NATIVE_LINKER_TRUE@script_test_8.stdout: script_test_8
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	$(TEST_READELF) -SlW script_test_8 > script_test_8.stdout
+@GCC_TRUE@@NATIVE_LINKER_TRUE@script_test_9.o: script_test_9.cc
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -O0 -c -o $@ $<
+@GCC_TRUE@@NATIVE_LINKER_TRUE@script_test_9: gcctestdir/ld $(srcdir)/script_test_9.t script_test_9.o
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXLINK) -Bgcctestdir/ script_test_9.o -T $(srcdir)/script_test_9.t
 @GCC_TRUE@@NATIVE_LINKER_TRUE@dynamic_list: basic_test.o gcctestdir/ld $(srcdir)/dynamic_list.t
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXLINK) -Bgcctestdir/ basic_test.o \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	  -Wl,--dynamic-list $(srcdir)/dynamic_list.t \
diff --git a/gold/testsuite/script_test_9.cc b/gold/testsuite/script_test_9.cc
new file mode 100644
index 0000000..682be8b
--- /dev/null
+++ b/gold/testsuite/script_test_9.cc
@@ -0,0 +1,29 @@
+// script_test_9.cc -- a test case for gold
+
+// Copyright 2009 Free Software Foundation, Inc.
+// Written by Cary Coutant <ccoutant@google.com>.
+
+// This file is part of gold.
+
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software
+// Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+// MA 02110-1301, USA.
+
+// This program checks that the default renaming of ".text.xxx"
+// sections does not take place in the presence of a linker script
+// with a SECTIONS clause.
+
+int main() {
+  return 0;
+}
diff --git a/gold/testsuite/script_test_9.sh b/gold/testsuite/script_test_9.sh
new file mode 100755
index 0000000..c965a51
--- /dev/null
+++ b/gold/testsuite/script_test_9.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+# script_test_8.sh -- dummy scirpt, we just want to check that the
+# program links.
+
+# Copyright 2009 Free Software Foundation, Inc.
+# Written by Doug Kwan <dougkwan@google.com>.
+
+# This file is part of gold.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+exit 0
diff --git a/gold/testsuite/script_test_9.t b/gold/testsuite/script_test_9.t
new file mode 100644
index 0000000..58cc350
--- /dev/null
+++ b/gold/testsuite/script_test_9.t
@@ -0,0 +1,18 @@
+PHDRS
+{
+  text    PT_LOAD FLAGS(5);
+  data    PT_LOAD FLAGS(6);
+}
+
+SECTIONS
+{
+  .init           :
+  {
+  } :text
+  .text           :
+  {
+  }
+  .data :
+  {
+  } :data
+}

Re: [patch][gold] Fix case where a empty section defines the segment of a following section

[Rafael Espindola]

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

The updated patch attached runs readelf to check that .init and .text
are in the same segment.

Cheers,
-- 
Rafael Ávila de Espíndola

[-- Attachment #2: script.patch --]
[-- Type: text/x-diff, Size: 6944 bytes --]

diff --git a/gold/script-sections.cc b/gold/script-sections.cc
index b4cbb40..6531600 100644
--- a/gold/script-sections.cc
+++ b/gold/script-sections.cc
@@ -2161,13 +2161,16 @@ Output_section*
 Output_section_definition::allocate_to_segment(String_list** phdrs_list,
 					       bool* orphan)
 {
+  // Update phdrs_list even if we don't have an output section. It
+  // might be used by the following sections.
+  if (this->phdrs_ != NULL)
+    *phdrs_list = this->phdrs_;
+
   if (this->output_section_ == NULL)
     return NULL;
   if ((this->output_section_->flags() & elfcpp::SHF_ALLOC) == 0)
     return NULL;
   *orphan = false;
-  if (this->phdrs_ != NULL)
-    *phdrs_list = this->phdrs_;
   return this->output_section_;
 }
 
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 435eeab..4cd69ac 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -1167,6 +1167,17 @@ script_test_8: basic_test.o gcctestdir/ld $(srcdir)/script_test_7.t
 script_test_8.stdout: script_test_8
 	$(TEST_READELF) -SlW script_test_8 > script_test_8.stdout
 
+
+check_SCRIPTS += script_test_9.sh
+check_DATA += script_test_9.stdout
+script_test_9.o: script_test_9.cc
+	$(CXXCOMPILE) -O0 -c -o $@ $<
+script_test_9: gcctestdir/ld $(srcdir)/script_test_9.t script_test_9.o
+	$(CXXLINK) -Bgcctestdir/ script_test_9.o -T $(srcdir)/script_test_9.t
+script_test_9.stdout: script_test_9
+	$(TEST_READELF) -lW script_test_9 > script_test_9.stdout
+
+
 # Test --dynamic-list, --dynamic-list-data, --dynamic-list-cpp-new,
 # and --dynamic-list-cpp-typeinfo
 
diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in
index 968d010..d5aec90 100644
--- a/gold/testsuite/Makefile.in
+++ b/gold/testsuite/Makefile.in
@@ -75,7 +75,8 @@ check_PROGRAMS = object_unittest$(EXEEXT) binary_unittest$(EXEEXT) \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_5.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_6.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_7.sh \
-@GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_8.sh dynamic_list.sh
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_8.sh \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_9.sh dynamic_list.sh
 
 # Create the data files that debug_msg.sh analyzes.
 
@@ -110,6 +111,7 @@ check_PROGRAMS = object_unittest$(EXEEXT) binary_unittest$(EXEEXT) \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_6.stdout \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_7.stdout \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_8.stdout \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	script_test_9.stdout \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	dynamic_list.stdout
 @GCC_TRUE@@NATIVE_LINKER_TRUE@am__append_3 = incremental_test \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	gc_comdat_test gc_tls_test \
@@ -2967,6 +2969,12 @@ uninstall-am:
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	-Wl,-Ttext=0x20001000 -Wl,-Tdata=0x20200000 -Wl,-Tbss=0x20400000
 @GCC_TRUE@@NATIVE_LINKER_TRUE@script_test_8.stdout: script_test_8
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	$(TEST_READELF) -SlW script_test_8 > script_test_8.stdout
+@GCC_TRUE@@NATIVE_LINKER_TRUE@script_test_9.o: script_test_9.cc
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXCOMPILE) -O0 -c -o $@ $<
+@GCC_TRUE@@NATIVE_LINKER_TRUE@script_test_9: gcctestdir/ld $(srcdir)/script_test_9.t script_test_9.o
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXLINK) -Bgcctestdir/ script_test_9.o -T $(srcdir)/script_test_9.t
+@GCC_TRUE@@NATIVE_LINKER_TRUE@script_test_9.stdout: script_test_9
+@GCC_TRUE@@NATIVE_LINKER_TRUE@	$(TEST_READELF) -lW script_test_9 > script_test_9.stdout
 @GCC_TRUE@@NATIVE_LINKER_TRUE@dynamic_list: basic_test.o gcctestdir/ld $(srcdir)/dynamic_list.t
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	$(CXXLINK) -Bgcctestdir/ basic_test.o \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@	  -Wl,--dynamic-list $(srcdir)/dynamic_list.t \
diff --git a/gold/testsuite/script_test_9.cc b/gold/testsuite/script_test_9.cc
new file mode 100644
index 0000000..682be8b
--- /dev/null
+++ b/gold/testsuite/script_test_9.cc
@@ -0,0 +1,29 @@
+// script_test_9.cc -- a test case for gold
+
+// Copyright 2009 Free Software Foundation, Inc.
+// Written by Cary Coutant <ccoutant@google.com>.
+
+// This file is part of gold.
+
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program; if not, write to the Free Software
+// Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+// MA 02110-1301, USA.
+
+// This program checks that the default renaming of ".text.xxx"
+// sections does not take place in the presence of a linker script
+// with a SECTIONS clause.
+
+int main() {
+  return 0;
+}
diff --git a/gold/testsuite/script_test_9.sh b/gold/testsuite/script_test_9.sh
new file mode 100755
index 0000000..9f2b7ce
--- /dev/null
+++ b/gold/testsuite/script_test_9.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+# script_test_8.sh -- Check that the script_test_9.t script has placed
+# .init and .text in the same segment.
+
+# Copyright 2009 Free Software Foundation, Inc.
+# Written by Doug Kwan <dougkwan@google.com>.
+
+# This file is part of gold.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+check()
+{
+    if ! grep -q "$2" "$1"
+    then
+	echo "Did not find expected section in $1:"
+	echo "   $2"
+	echo ""
+	echo "Actual output below:"
+	cat "$1"
+	exit 1
+    fi
+}
+
+check script_test_9.stdout "LOAD .*R E "
+check script_test_9.stdout "LOAD .*RW "
+check script_test_9.stdout "00     .text .init"
+check script_test_9.stdout "01     .data "
diff --git a/gold/testsuite/script_test_9.t b/gold/testsuite/script_test_9.t
new file mode 100644
index 0000000..58cc350
--- /dev/null
+++ b/gold/testsuite/script_test_9.t
@@ -0,0 +1,18 @@
+PHDRS
+{
+  text    PT_LOAD FLAGS(5);
+  data    PT_LOAD FLAGS(6);
+}
+
+SECTIONS
+{
+  .init           :
+  {
+  } :text
+  .text           :
+  {
+  }
+  .data :
+  {
+  } :data
+}

Re: [patch][gold] Fix case where a empty section defines the segment of a following section

[Ian Lance Taylor]

Rafael Espindola <espindola@google.com> writes:

> The updated patch attached runs readelf to check that .init and .text
> are in the same segment.


> --- /dev/null
> +++ b/gold/testsuite/script_test_9.cc
> @@ -0,0 +1,29 @@
> +// script_test_9.cc -- a test case for gold
> +
> +// Copyright 2009 Free Software Foundation, Inc.
> +// Written by Cary Coutant <ccoutant@google.com>.

Fix copyright year and author.


> diff --git a/gold/testsuite/script_test_9.sh b/gold/testsuite/script_test_9.sh
> new file mode 100755
> index 0000000..9f2b7ce
> --- /dev/null
> +++ b/gold/testsuite/script_test_9.sh
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +
> +# script_test_8.sh -- Check that the script_test_9.t script has placed
> +# .init and .text in the same segment.
> +
> +# Copyright 2009 Free Software Foundation, Inc.
> +# Written by Doug Kwan <dougkwan@google.com>.

Fix file name, copyright year, and author.


> +check script_test_9.stdout "LOAD .*R E "
> +check script_test_9.stdout "LOAD .*RW "
> +check script_test_9.stdout "00     .text .init"
> +check script_test_9.stdout "01     .data "

The last two tests should probably be more flexible about white
space.


This is OK with those changes.

Thanks.

Ian

Re: [patch][gold] Fix case where a empty section defines the segment of a following section

[H.J. Lu]

On Mon, May 17, 2010 at 1:37 PM, Rafael Espindola <espindola@google.com> wrote:
> The updated patch attached runs readelf to check that .init and .text
> are in the same segment.
>
> Cheers,

Hi,

gold/testsuite/script_test_9.sh isn't executable:

[hjl@gnu-6 src]$ ls -l gold/testsuite/script_test_9.sh
-rw-r--r-- 1 hjl hjl 1312 2010-05-19 10:49 gold/testsuite/script_test_9.sh
[hjl@gnu-6 src]$

Could someone please fix it? Jim, can you fix git?

Thanks.


-- 
H.J.

Re: [patch][gold] Fix case where a empty section defines the segment of a following section

[Rafael Espindola]

> Hi,
>
> gold/testsuite/script_test_9.sh isn't executable:
>
> [hjl@gnu-6 src]$ ls -l gold/testsuite/script_test_9.sh
> -rw-r--r-- 1 hjl hjl 1312 2010-05-19 10:49 gold/testsuite/script_test_9.sh
> [hjl@gnu-6 src]$
>
> Could someone please fix it? Jim, can you fix git?

Very sorry about that. I confess my ignorance, how do I mark a file
executable in cvs? It has something like svn:executable?

> Thanks.
>
>
> --
> H.J.
>


Cheers,
-- 
Rafael Ávila de Espíndola

Re: [patch][gold] Fix case where a empty section defines the segment of a following section

[Jim Meyering]

Rafael Espindola wrote:
>> gold/testsuite/script_test_9.sh isn't executable:
>>
>> [hjl@gnu-6 src]$ ls -l gold/testsuite/script_test_9.sh
>> -rw-r--r-- 1 hjl hjl 1312 2010-05-19 10:49 gold/testsuite/script_test_9.sh
>> [hjl@gnu-6 src]$
>>
>> Could someone please fix it? Jim, can you fix git?
>
> Very sorry about that. I confess my ignorance, how do I mark a file
> executable in cvs? It has something like svn:executable?

Are you guys planning to switch to a real version control system?

I've cleaned up, again.
http://thread.gmane.org/gmane.comp.gnu.binutils/46632/focus=46658

For my reference, in case I do this again:
(slightly contorted, since I didn't own the file, and thus
could not chmod it directly)

    cd /cvs/src/src/gold/testsuite/
    t=script_test_9.sh,v
    cp -a $t k && chmod a+x k && mv -f k $t

and did this to the git mirror:

    git clone ~/mirror-git-to-cvs/repo/binutils &&
    cd binutils/gold/testsuite
    chmod a+x script_test_9.sh
    git ci -m 'gold: set execute bit on a test script' -a
    git push

If it hasn't propagated in an hour or two, let me know.