Re: [PATCH] Fix Gold/strip discrepancies for PR 11786

Doug Evans
2013-11-04T22:21:45Z

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

On Thu, Oct 31, 2013 at 8:49 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hi Doug,
>
> On Sat, 26 Oct 2013 01:26:22 +0200, Doug Evans wrote:
>> This patch addresses the discrepancy in the flags and align fields
>> of PT_GNU_RELRO between Gold and strip.
>
> originally I thought the discrepancy happened due to some speciality of Gold;
> but you state Gold probably just because you use Gold by default.
> On CentOS-5 x86_64 the problem happens to me even with GNU ld.
>
> And the problem is binutils strip. It is known it modifies the executable
> more than it has to, AFAIK (IIRC according to Jakub) due to the generalization
> by bfd.
>
> Red Hat OSes have been always using eu-strip instead (=elfutils strip), which
> is native for ELF and modifies the file only minimally.
>
> During my test on CentOS-5 x86_64 really binutils strip changed it:
>
> Program Headers:
> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
> - GNU_RELRO 0x000de8 0x0000000000200de8 0x0000000000200de8 0x000218 0x000218 R 0x1
> + GNU_RELRO 0x000de8 0x0000000000200de8 0x0000000000200de8 0x000200 0x000200 R 0x1
>
> while elfutils strip left Program Headers intact. So reviewed the patch below
> and I am fine with it that way but I do not think it is the right solution to
> your problem.

What would be the right solution? [keeping in mind that I need this to
work with existing tools]
I can imagine multiple "solutions" are in fact needed.

> On Sat, 26 Oct 2013 01:26:22 +0200, Doug Evans wrote:
>> --- a/gdb/solib-svr4.c
>> +++ b/gdb/solib-svr4.c
>> @@ -2608,6 +2608,22 @@ svr4_exec_displacement (CORE_ADDR *displacementp)
>> if (memcmp (phdrp, phdr2p, sizeof (*phdrp)) == 0)
>> continue;
>>
>> + /* Gold and strip differ on the flags and alignment of
>> + PT_GNU_RELRO. See PR 11786. */
>> + if (phdr2[i].p_type == PT_GNU_RELRO)
>> + {
>> + Elf32_External_Phdr tmp_phdr = *phdrp;
>> + Elf32_External_Phdr tmp_phdr2 = *phdr2p;
>
> One can modify directly *phdrp and *phdr2p, when we decide to ignore the
> PT_GNU_RELRO differences there is no other use for that data.

I like the locality, and not imposing the side effect outside the
scope of the task at hand (PT_GNU_RELRO).

>> +
>> + memset (tmp_phdr.p_flags, 0, 4);
>> + memset (tmp_phdr.p_align, 0, 4);
>> + memset (tmp_phdr2.p_flags, 0, 4);
>> + memset (tmp_phdr2.p_align, 0, 4);
>
> Missing here also FileSiz and MemSiz, during my test on CentOS-5 x86_64 the
> testcase was still FAILing due to:
>
> Program Headers:
> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
> - GNU_RELRO 0x000de8 0x0000000000200de8 0x0000000000200de8 0x000218 0x000218 R 0x1
> + GNU_RELRO 0x000de8 0x0000000000200de8 0x0000000000200de8 0x000200 0x000200 R 0x1
>
> Therefore tested as PASSing with:
>
> memset (tmp_phdr.p_filesz, 0, 4);
> memset (tmp_phdr.p_memsz, 0, 4);
> memset (tmp_phdr.p_flags, 0, 4);
> memset (tmp_phdr.p_align, 0, 4);
> memset (tmp_phdr2.p_filesz, 0, 4);
> memset (tmp_phdr2.p_memsz, 0, 4);
> memset (tmp_phdr2.p_flags, 0, 4);
> memset (tmp_phdr2.p_align, 0, 4);

"works for me"

>> +
>> + if (memcmp (&tmp_phdr, &tmp_phdr2, sizeof (tmp_phdr)) == 0)
>> + continue;
>> + }
>> +
>> /* prelink can convert .plt SHT_NOBITS to SHT_PROGBITS. */
>> plt2_asect = bfd_get_section_by_name (exec_bfd, ".plt");
>> if (plt2_asect)
>> @@ -2717,6 +2733,22 @@ svr4_exec_displacement (CORE_ADDR *displacementp)
>> if (memcmp (phdrp, phdr2p, sizeof (*phdrp)) == 0)
>> continue;
>>
>> + /* Gold and strip differ on the flags and alignment of
>> + PT_GNU_RELRO. See PR 11786. */
>> + if (phdr2[i].p_type == PT_GNU_RELRO)
>> + {
>> + Elf64_External_Phdr tmp_phdr = *phdrp;
>> + Elf64_External_Phdr tmp_phdr2 = *phdr2p;
>
> Likewise.
>
>
>> +
>> + memset (tmp_phdr.p_flags, 0, 4);
>> + memset (tmp_phdr.p_align, 0, 8);
>> + memset (tmp_phdr2.p_flags, 0, 4);
>> + memset (tmp_phdr2.p_align, 0, 8);
>
> Change to:
> memset (tmp_phdr.p_filesz, 0, 8);
> memset (tmp_phdr.p_memsz, 0, 8);
> memset (tmp_phdr.p_flags, 0, 4);
> memset (tmp_phdr.p_align, 0, 8);
> memset (tmp_phdr2.p_filesz, 0, 8);
> memset (tmp_phdr2.p_memsz, 0, 8);
> memset (tmp_phdr2.p_flags, 0, 4);
> memset (tmp_phdr2.p_align, 0, 8);
>
>
>> +
>> + if (memcmp (&tmp_phdr, &tmp_phdr2, sizeof (tmp_phdr)) == 0)
>> + continue;
>> + }
>> +
>> /* prelink can convert .plt SHT_NOBITS to SHT_PROGBITS. */
>> plt2_asect = bfd_get_section_by_name (exec_bfd, ".plt");
>> if (plt2_asect)
>> diff --git a/gdb/testsuite/gdb.base/gcore-relro-pie.c b/gdb/testsuite/gdb.base/gcore-relro-pie.c
>> new file mode 100644
>> index 0000000..1594385
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/gcore-relro-pie.c
>> @@ -0,0 +1,41 @@
>> +/* Copyright 2013 Free Software Foundation, Inc.
>> +
>> + This file is part of GDB.
>> +
>> + 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, see <http://www.gnu.org/licenses/>. */
>> +
>> +void
>> +break_here ()
>
> (void)

We don't have any such style rules for testcases.
But ok, done.

Going forward though, for my own patch reviews of other people's code,
what's the story here?
Do we augment coding style rules for testcases to match the rest of gdb?
I like rules being applied consistently.

>> +{
>> + *(int *) 0 = 0;
>> +}
>> +
>> +void
>> +foo ()
>
> (void)
>
>> +{
>> + break_here ();
>> +}
>> +
>> +void
>> +bar ()
>
> (void)
>
>> +{
>> + foo ();
>> +}
>> +
>> +int
>> +main (void)
>> +{
>> + bar ();
>> + return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.base/gcore-relro-pie.exp b/gdb/testsuite/gdb.base/gcore-relro-pie.exp
>> new file mode 100644
>> index 0000000..1fcfd8c
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/gcore-relro-pie.exp
>> @@ -0,0 +1,70 @@
>> +# Copyright 2013 Free Software Foundation, Inc.
>> +
>> +# 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, see <http://www.gnu.org/licenses/>.
>> +
>> +# PR 11786 (Gold and strip differ on flags,align fields of PT_GNU_RELRO).
>> +# Generate a core file from the stripped version of the program,
>> +# and then try to debug the core with the unstripped version.
>> +
>> +standard_testfile
>> +
>> +if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug additional_flags=-fpie additional_flags=-pie additional_flags=-Wl,-z,relro}]} {
>
> CentOS-5 (older GCC) compatibility:
> gdb compile failed, gcc: -z: linker input file unused because linking not done
> gcc: relro: linker input file unused because linking not done
>
> coding style: Three times repeating additional_flags=...
>
> ->
>
> if {[prepare_for_testing $testfile.exp $testfile $srcfile [list debug additional_flags=-fpie "ldflags=-pie -Wl,-z,relro"]]} {

Done.

>> + return -1
>> +}
>> +
>> +set stripped_binfile ${binfile}.stripped
>> +set gcorefile ${binfile}.gcore
>> +
>> +set strip_program [transform strip]
>> +remote_file host delete ${stripped_binfile}
>> +if [run_on_host "strip" "$strip_program" "-g -o ${stripped_binfile} $binfile"] {
>> + return -1
>> +}
>
> For CentOS-5 is missing this part, similar to what is in lib/gdb.exp:
>
> # Workaround PR binutils/10802:
> # Preserve the 'x' bit also for PIEs (Position Independent Executables).
> set perm [file attributes ${binfile} -permissions]
> file attributes ${stripped_binfile} -permissions $perm

Done.

>> +
>> +clean_restart ${stripped_binfile}
>> +
>> +# Does this gdb support gcore?
>> +set test "help gcore"
>> +gdb_test_multiple $test $test {
>> + -re "Undefined command: .gcore.*\r\n$gdb_prompt $" {
>> + # gcore command not supported -- nothing to test here.
>> + unsupported "gdb does not support gcore on this target"
>> + return -1
>> + }
>> + -re "Save a core file .*\r\n$gdb_prompt $" {
>> + pass $test
>> + }
>> +}
>> +
>> +# The binary is stripped of debug info, but not minsyms.
>> +if ![runto break_here] {
>> + fail "Can't run to break_here"
>> + return -1
>> +}
>> +
>> +if {![gdb_gcore_cmd $gcorefile "save a corefile"]} {
>> + return -1
>> +}
>> +
>> +# Now restart gdb with the unstripped binary and load the corefile.
>> +
>> +clean_restart ${binfile}
>> +
>> +gdb_test "core ${gcorefile}" \
>> + "Core was generated by .*" "re-load generated corefile"
>> +
>> +# Put $pc in gdb.log for debug purposes for comparison with stripped case.
>> +gdb_test "x/i \$pc" "break_here.*"
>> +
>> +gdb_test "frame" "#0 \[^\r\n\]* break_here .*" "unstripped + core ok"
>
>
> Thanks,
> Jan

I will check this in in a few days pending further requests for changes.
[which I'm happy to do ... I just hate unnecessarily requiring people
to send (and read) email, there's so much of it already]

[-- Attachment #2: gdb-131104-11786-2.patch.txt --]
[-- Type: text/plain, Size: 6366 bytes --]

2013-11-04 Doug Evans <dje@google.com>

PR 11786
* solib-svr4.c (svr4_exec_displacement): Ignore filesz, memsz, flags
and align fields for PT_GNU_RELRO segments.

testsuite/
* gdb.base/gcore-relro-pie.c: New file.
* gdb.base/gcore-relro-pie.exp: New file.

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 3eea057..9538af6 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -2604,6 +2604,28 @@ svr4_exec_displacement (CORE_ADDR *displacementp)
if (memcmp (phdrp, phdr2p, sizeof (*phdrp)) == 0)
continue;

+ /* Strip modifies the flags and alignment of PT_GNU_RELRO.
+ CentOS-5 has problems with filesz, memsz as well.
+ See PR 11786. */
+ if (phdr2[i].p_type == PT_GNU_RELRO)
+ {
+ Elf32_External_Phdr tmp_phdr = *phdrp;
+ Elf32_External_Phdr tmp_phdr2 = *phdr2p;
+
+ memset (tmp_phdr.p_filesz, 0, 4);
+ memset (tmp_phdr.p_memsz, 0, 4);
+ memset (tmp_phdr.p_flags, 0, 4);
+ memset (tmp_phdr.p_align, 0, 4);
+ memset (tmp_phdr2.p_filesz, 0, 4);
+ memset (tmp_phdr2.p_memsz, 0, 4);
+ memset (tmp_phdr2.p_flags, 0, 4);
+ memset (tmp_phdr2.p_align, 0, 4);
+
+ if (memcmp (&tmp_phdr, &tmp_phdr2, sizeof (tmp_phdr))
+ == 0)
+ continue;
+ }
+
/* prelink can convert .plt SHT_NOBITS to SHT_PROGBITS. */
plt2_asect = bfd_get_section_by_name (exec_bfd, ".plt");
if (plt2_asect)
@@ -2713,6 +2735,28 @@ svr4_exec_displacement (CORE_ADDR *displacementp)
if (memcmp (phdrp, phdr2p, sizeof (*phdrp)) == 0)
continue;

+ /* Strip modifies the flags and alignment of PT_GNU_RELRO.
+ CentOS-5 has problems with filesz, memsz as well.
+ See PR 11786. */
+ if (phdr2[i].p_type == PT_GNU_RELRO)
+ {
+ Elf64_External_Phdr tmp_phdr = *phdrp;
+ Elf64_External_Phdr tmp_phdr2 = *phdr2p;
+
+ memset (tmp_phdr.p_filesz, 0, 8);
+ memset (tmp_phdr.p_memsz, 0, 8);
+ memset (tmp_phdr.p_flags, 0, 4);
+ memset (tmp_phdr.p_align, 0, 8);
+ memset (tmp_phdr2.p_filesz, 0, 8);
+ memset (tmp_phdr2.p_memsz, 0, 8);
+ memset (tmp_phdr2.p_flags, 0, 4);
+ memset (tmp_phdr2.p_align, 0, 8);
+
+ if (memcmp (&tmp_phdr, &tmp_phdr2, sizeof (tmp_phdr))
+ == 0)
+ continue;
+ }
+
/* prelink can convert .plt SHT_NOBITS to SHT_PROGBITS. */
plt2_asect = bfd_get_section_by_name (exec_bfd, ".plt");
if (plt2_asect)
diff --git a/gdb/testsuite/gdb.base/gcore-relro-pie.c b/gdb/testsuite/gdb.base/gcore-relro-pie.c
new file mode 100644
index 0000000..d72969b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gcore-relro-pie.c
@@ -0,0 +1,41 @@
+/* Copyright 2013 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ 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, see <http://www.gnu.org/licenses/>. */
+
+void
+break_here (void)
+{
+ *(int *) 0 = 0;
+}
+
+void
+foo (void)
+{
+ break_here ();
+}
+
+void
+bar (void)
+{
+ foo ();
+}
+
+int
+main (void)
+{
+ bar ();
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/gcore-relro-pie.exp b/gdb/testsuite/gdb.base/gcore-relro-pie.exp
new file mode 100644
index 0000000..eb45c52
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gcore-relro-pie.exp
@@ -0,0 +1,75 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# 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, see <http://www.gnu.org/licenses/>.
+
+# PR 11786 (Gold and strip differ on flags,align fields of PT_GNU_RELRO).
+# Generate a core file from the stripped version of the program,
+# and then try to debug the core with the unstripped version.
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug additional_flags=-fpie "ldflags=-pie -Wl,-z,relro"}]} {
+ return -1
+}
+
+set stripped_binfile ${binfile}.stripped
+set gcorefile ${binfile}.gcore
+
+set strip_program [transform strip]
+remote_file host delete ${stripped_binfile}
+if [run_on_host "strip" "$strip_program" "-g -o ${stripped_binfile} $binfile"] {
+ return -1
+}
+
+# Workaround PR binutils/10802:
+# Preserve the 'x' bit also for PIEs (Position Independent Executables).
+set perm [file attributes ${binfile} -permissions]
+file attributes ${stripped_binfile} -permissions $perm
+
+clean_restart ${stripped_binfile}
+
+# Does this gdb support gcore?
+set test "help gcore"
+gdb_test_multiple $test $test {
+ -re "Undefined command: .gcore.*\r\n$gdb_prompt $" {
+ # gcore command not supported -- nothing to test here.
+ unsupported "gdb does not support gcore on this target"
+ return -1
+ }
+ -re "Save a core file .*\r\n$gdb_prompt $" {
+ pass $test
+ }
+}
+
+# The binary is stripped of debug info, but not minsyms.
+if ![runto break_here] {
+ fail "Can't run to break_here"
+ return -1
+}
+
+if {![gdb_gcore_cmd $gcorefile "save a corefile"]} {
+ return -1
+}
+
+# Now restart gdb with the unstripped binary and load the corefile.
+
+clean_restart ${binfile}
+
+gdb_test "core ${gcorefile}" \
+ "Core was generated by .*" "re-load generated corefile"
+
+# Put $pc in gdb.log for debug purposes for comparison with stripped case.
+gdb_test "x/i \$pc" "break_here.*"
+
+gdb_test "frame" "#0 \[^\r\n\]* break_here .*" "unstripped + core ok"