* [PATCH] Fix Gold/strip discrepancies for PR 11786 @ 2013-10-25 23:26 Doug Evans 2013-10-30 23:57 ` Doug Evans 2013-10-31 16:42 ` Jan Kratochvil 0 siblings, 2 replies; 23+ messages in thread From: Doug Evans @ 2013-10-25 23:26 UTC (permalink / raw) To: gdb-patches, jan.kratochvil; +Cc: ccoutant Hi. This patch addresses the discrepancy in the flags and align fields of PT_GNU_RELRO between Gold and strip. Ref: https://sourceware.org/bugzilla/show_bug.cgi?id=11786 Ok to check in? 2013-10-25 Doug Evans <dje@google.com> PR 11786 * solib-svr4.c (svr4_exec_displacement): Ignore 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 ddbbd94..d3b55e5 100644 --- 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; + + 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); + + 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; + + 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); + + 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 () +{ + *(int *) 0 = 0; +} + +void +foo () +{ + break_here (); +} + +void +bar () +{ + 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}]} { + 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 +} + +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" ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-10-25 23:26 [PATCH] Fix Gold/strip discrepancies for PR 11786 Doug Evans @ 2013-10-30 23:57 ` Doug Evans 2013-10-31 16:42 ` Jan Kratochvil 1 sibling, 0 replies; 23+ messages in thread From: Doug Evans @ 2013-10-30 23:57 UTC (permalink / raw) To: gdb-patches, Jan Kratochvil; +Cc: Cary Coutant On Fri, Oct 25, 2013 at 4:26 PM, Doug Evans <dje@google.com> wrote: > Hi. > > This patch addresses the discrepancy in the flags and align fields > of PT_GNU_RELRO between Gold and strip. > > Ref: https://sourceware.org/bugzilla/show_bug.cgi?id=11786 > > Ok to check in? > > 2013-10-25 Doug Evans <dje@google.com> > > PR 11786 > * solib-svr4.c (svr4_exec_displacement): Ignore 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. Ping. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-10-25 23:26 [PATCH] Fix Gold/strip discrepancies for PR 11786 Doug Evans 2013-10-30 23:57 ` Doug Evans @ 2013-10-31 16:42 ` Jan Kratochvil 2013-11-04 22:38 ` Doug Evans 1 sibling, 1 reply; 23+ messages in thread From: Jan Kratochvil @ 2013-10-31 16:42 UTC (permalink / raw) To: Doug Evans; +Cc: gdb-patches, ccoutant 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. 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. > + > + 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); > + > + 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) > +{ > + *(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"]]} { > + 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 > + > +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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-10-31 16:42 ` Jan Kratochvil @ 2013-11-04 22:38 ` Doug Evans 2013-11-04 23:04 ` Cary Coutant ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Doug Evans @ 2013-11-04 22:38 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Cary Coutant [-- 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" ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-04 22:38 ` Doug Evans @ 2013-11-04 23:04 ` Cary Coutant 2013-11-05 3:42 ` Tom Tromey 2013-11-05 17:04 ` Jan Kratochvil 2 siblings, 0 replies; 23+ messages in thread From: Cary Coutant @ 2013-11-04 23:04 UTC (permalink / raw) To: Doug Evans; +Cc: Jan Kratochvil, gdb-patches >> 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. I agree. In general, I think producers should be strict, and consumers should be lenient. That means that strip should leave the segment attributes the same as they were in the input, but it also means that GDB shouldn't care about fields that make no difference in whether two binaries are in fact equivalent. As far as strip goes, it's nice to hear that eu-strip does it right, but binutils strip, being BFD based, may not be so easy to fix. -cary ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-04 22:38 ` Doug Evans 2013-11-04 23:04 ` Cary Coutant @ 2013-11-05 3:42 ` Tom Tromey 2013-11-05 17:22 ` Doug Evans 2013-11-05 17:04 ` Jan Kratochvil 2 siblings, 1 reply; 23+ messages in thread From: Tom Tromey @ 2013-11-05 3:42 UTC (permalink / raw) To: Doug Evans; +Cc: Jan Kratochvil, gdb-patches, Cary Coutant >> (void) Doug> We don't have any such style rules for testcases. Doug> But ok, done. Doug> Going forward though, for my own patch reviews of other people's code, Doug> what's the story here? In this case, "()" is not idiomatic C, whereas "(void)" is. This is not the same as a coding style rule. Tom ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-05 3:42 ` Tom Tromey @ 2013-11-05 17:22 ` Doug Evans 2013-11-05 17:23 ` Jan Kratochvil ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Doug Evans @ 2013-11-05 17:22 UTC (permalink / raw) To: Tom Tromey; +Cc: Jan Kratochvil, gdb-patches, Cary Coutant On Mon, Nov 4, 2013 at 6:34 PM, Tom Tromey <tromey@redhat.com> wrote: >>> (void) > > Doug> We don't have any such style rules for testcases. > Doug> But ok, done. > > Doug> Going forward though, for my own patch reviews of other people's code, > Doug> what's the story here? > > In this case, "()" is not idiomatic C, whereas "(void)" is. > This is not the same as a coding style rule. I'm not sure how to read this. Is this an argument for requiring (void) in all testsuite C function definitions? It's ok by me, but it seems to me it's not a requirement today as there are plenty of existing examples, including recent ones. OTOH, if there is such a requirement we'd better get it written down so we can refer to it when requesting corrections in patches, and so people can know ahead of time what's expected. Obviously the rules state this for gdb itself, but it's been my understanding that these rules explicitly do not apply to the testsuite, and this understanding has been affirmed from time to time. All I'm asking for is clarity and consistency. Or is this just a point about a bad use of the word "style"? By itself "style" is a pretty nondescript word. Alas I'm not one for always assigning names with precision. If this isn't a style issue, coding *or* otherwise, let me know what to call it. If this is something else, let me know that too. :-) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-05 17:22 ` Doug Evans @ 2013-11-05 17:23 ` Jan Kratochvil 2013-11-05 18:01 ` Doug Evans 2013-11-05 17:32 ` Pedro Alves 2013-11-05 17:32 ` Tom Tromey 2 siblings, 1 reply; 23+ messages in thread From: Jan Kratochvil @ 2013-11-05 17:23 UTC (permalink / raw) To: Doug Evans; +Cc: Tom Tromey, gdb-patches, Cary Coutant On Tue, 05 Nov 2013 18:04:38 +0100, Doug Evans wrote: > I'm not sure how to read this. Primarily I do not understand if you really defend "()". "()" is wrong and obsolete syntax. Reason is that it does not check against passed parameters, making bugs more difficult to catch and therefore the code expensive to maintain. The real bug is that gdb/testsuite/ does not use the same CFLAGS like gdb/ does, including -Wmissing-prototypes. It would be nice to enable it there one day. But nowadays nobody is going to fix all the gdb/testsuite/ sources to make them compliant. But it is not right to (1) make the testsuite code more expensive to maintain, (2) needlessly different from the main GDB codebase code, (3) making the future work of enabling -Wmissing-prototypes for gdb/testsuite/ more expensive (if it ever happens). > It's ok by me, but it seems to me it's not a requirement today as > there are plenty of existing examples, Any existing code should be irrelevant, existing GDB code base is in a worse state than what should be required for new commits. > including recent ones. This is worse, I am aware of it. Just I do not want to spend more time catching such nitpicks when GDB has in several orders of magnitude more serious problems. Discussing such a clear thing like that "()" is forbidden seems also as a loss of time to me. C++ even does not know K&R prototype anymore so it will not even be possible in GDB anymore in some time, hopefully. Jan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-05 17:23 ` Jan Kratochvil @ 2013-11-05 18:01 ` Doug Evans 2013-11-05 18:13 ` Jan Kratochvil 0 siblings, 1 reply; 23+ messages in thread From: Doug Evans @ 2013-11-05 18:01 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Tom Tromey, gdb-patches, Cary Coutant On Tue, Nov 5, 2013 at 9:22 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Tue, 05 Nov 2013 18:04:38 +0100, Doug Evans wrote: >> I'm not sure how to read this. > > Primarily I do not understand if you really defend "()". "()" is wrong and > obsolete syntax. Reason is that it does not check against passed parameters, > making bugs more difficult to catch and therefore the code expensive to > maintain. All I'm asking for is clarity and consistency. [I'm not asking for perfection, but IWBN to make sure we're all on the same page.] > The real bug is that gdb/testsuite/ does not use the same CFLAGS like gdb/ > does, including -Wmissing-prototypes. > > It would be nice to enable it there one day. But nowadays nobody is going to > fix all the gdb/testsuite/ sources to make them compliant. But it is not > right to (1) make the testsuite code more expensive to maintain, > (2) needlessly different from the main GDB codebase code, (3) making the > future work of enabling -Wmissing-prototypes for gdb/testsuite/ more expensive > (if it ever happens). New tests go in all the time that are needlessly different from GDB. >> It's ok by me, but it seems to me it's not a requirement today as >> there are plenty of existing examples, > > Any existing code should be irrelevant, existing GDB code base is in a worse > state than what should be required for new commits. If the decision is to be more strict with the rules for testcases that's fine by me. Let's write it down, then discussions like these will become a *lot* shorter. >> including recent ones. > > This is worse, I am aware of it. Just I do not want to spend more time > catching such nitpicks when GDB has in several orders of magnitude more > serious problems. We spend time pointing out rule violations all the time. Why is this any different? Point (3) above says don't make the problem worse, but if I hadn't raised this issue I'm quite sure tests will continue to go in that violate any or all of 1,2,3. > Discussing such a clear thing like that "()" is forbidden > seems also as a loss of time to me. C++ even does not know K&R prototype > anymore so it will not even be possible in GDB anymore in some time, > hopefully. We also spend time discussing things from time to time. Why should this be any different? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-05 18:01 ` Doug Evans @ 2013-11-05 18:13 ` Jan Kratochvil 2013-11-06 21:16 ` Doug Evans 0 siblings, 1 reply; 23+ messages in thread From: Jan Kratochvil @ 2013-11-05 18:13 UTC (permalink / raw) To: Doug Evans; +Cc: Tom Tromey, gdb-patches, Cary Coutant On Tue, 05 Nov 2013 18:56:29 +0100, Doug Evans wrote: > If the decision is to be more strict with the rules for testcases > that's fine by me. > Let's write it down, then discussions like these will become a *lot* shorter. Adding more and more rules I do not find as a clear win. When I code GDB I have to think about so many established non-standard coding style rules my head is going to explode. Switching between multiple projects each having different coding style makes it worse. But sending a patch and getting it corrected here and there due to unwritten rules one could not find anywhere is also not great, though, I agree. Regards, Jan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-05 18:13 ` Jan Kratochvil @ 2013-11-06 21:16 ` Doug Evans 2013-11-06 21:28 ` Jan Kratochvil 0 siblings, 1 reply; 23+ messages in thread From: Doug Evans @ 2013-11-06 21:16 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Tom Tromey, gdb-patches, Pedro Alves On Tue, Nov 5, 2013 at 10:05 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Tue, 05 Nov 2013 18:56:29 +0100, Doug Evans wrote: >> If the decision is to be more strict with the rules for testcases >> that's fine by me. >> Let's write it down, then discussions like these will become a *lot* shorter. > > Adding more and more rules I do not find as a clear win. > When I code GDB I have to think about so many established non-standard coding > style rules my head is going to explode. Switching between multiple projects > each having different coding style makes it worse. > > But sending a patch and getting it corrected here and there due to unwritten > rules one could not find anywhere is also not great, though, I agree. At the end of the day I'm still lacking the clarity I seek. [It's not imperative, but it's more than "IWBN".] I'm not suggesting adding more rules (per se), but I do think there's no downside to writing down existing unwritten rules (for those things that are, indeed, rules). I'm going to propose the following, and if y'all are ok with it then this can be the end of it, this thread is done. If there are no objections, I will add a Testsuite section to the C-Coding-Standards section of the wiki: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards It will basically say tests are in general not required to following the GDB coding standards, but there are a few exceptions, and then have an enumeration of explicit rules, and for now just have the one: (void) is required over (). And for grin's sake, since there's so many of them, and less likely to be a problem, int main () is ok. I'll also mention that "Monkey See Monkey Do hacking should generally Just Work." [There's less need to go into detail if one can say one can just mimic existing code. That will keep it short-and-sweet. I'm ok with people adding more to the wiki, but I like taking "baby steps".] I'll probably also add a link to the new section to: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-06 21:16 ` Doug Evans @ 2013-11-06 21:28 ` Jan Kratochvil 2013-11-07 1:05 ` Stan Shebs 2013-11-07 18:01 ` Doug Evans 0 siblings, 2 replies; 23+ messages in thread From: Jan Kratochvil @ 2013-11-06 21:28 UTC (permalink / raw) To: Doug Evans; +Cc: Tom Tromey, gdb-patches, Pedro Alves On Wed, 06 Nov 2013 22:14:27 +0100, Doug Evans wrote: > It will basically say tests are in general not required to following > the GDB coding standards, What is the reason for it? I would find more logical to say tests should follow the same rules as GDB code, unless there is a specific reason for it. Such as importing an existing external reproducer, machine generated output etc. If GDB coding standards are not acceptable for testcases then it looks to me as an indication the GDB coding standards should be changed. Jan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-06 21:28 ` Jan Kratochvil @ 2013-11-07 1:05 ` Stan Shebs 2013-11-07 18:01 ` Doug Evans 1 sibling, 0 replies; 23+ messages in thread From: Stan Shebs @ 2013-11-07 1:05 UTC (permalink / raw) To: gdb-patches On 11/6/13 1:24 PM, Jan Kratochvil wrote: > On Wed, 06 Nov 2013 22:14:27 +0100, Doug Evans wrote: >> It will basically say tests are in general not required to following >> the GDB coding standards, > > What is the reason for it? > > I would find more logical to say tests should follow the same rules as GDB > code, unless there is a specific reason for it. Such as importing an existing > external reproducer, machine generated output etc. > > If GDB coding standards are not acceptable for testcases then it looks to me > as an indication the GDB coding standards should be changed. The point of the testsuite is to check GDB behavior on programs in general. We would look pretty foolish if someone were to encounter a GDB bug that only occurred in source that did not follow the GDB standard, and we were unable to detect it because our regression tests did not include code written in a variety of styles. As examples, the GNU style prefers to have braces on lines by themselves, and to not have side-effecting code in conditionals, even though there are millions of lines of code written that way. Without a testsuite including these common usages, it would be easy to introduce bugs in single-stepping and breakpoint handling that our regression testing could not catch. Stan stan@codesourcery.com ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-06 21:28 ` Jan Kratochvil 2013-11-07 1:05 ` Stan Shebs @ 2013-11-07 18:01 ` Doug Evans 2013-11-07 19:03 ` Jan Kratochvil 1 sibling, 1 reply; 23+ messages in thread From: Doug Evans @ 2013-11-07 18:01 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Tom Tromey, gdb-patches, Pedro Alves, Stan Shebs On Wed, Nov 6, 2013 at 1:24 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Wed, 06 Nov 2013 22:14:27 +0100, Doug Evans wrote: >> It will basically say tests are in general not required to following >> the GDB coding standards, > > What is the reason for it? To codify the existing rules, as I understand them, and as has been affirmed from time to time. > I would find more logical to say tests should follow the same rules as GDB > code, unless there is a specific reason for it. Such as importing an existing > external reproducer, machine generated output etc. > > If GDB coding standards are not acceptable for testcases then it looks to me > as an indication the GDB coding standards should be changed. I don't mind such changes, but these are changes. Agreed? I was trying to end the thread, and make some minimal mutually agreeable progress. Also, AIUI, this community generally frowns on cleanup projects that will drag on. I can mechanically run every .c file through indent with some settings people agree on, but I'm not signing up to audit the entire testsuite to make sure we don't accidentally change a test that has important reasons for why it is written the way it is. OTOH, if we can all agree that existing tests need only be lazily updated (or not at all - I don't have a strong opinion) then that works too (though coding standards work so much better when Monkey See Monkey Do hacking Just Works). ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-07 18:01 ` Doug Evans @ 2013-11-07 19:03 ` Jan Kratochvil 2013-11-08 17:57 ` Doug Evans 0 siblings, 1 reply; 23+ messages in thread From: Jan Kratochvil @ 2013-11-07 19:03 UTC (permalink / raw) To: Doug Evans; +Cc: Tom Tromey, gdb-patches, Pedro Alves, Stan Shebs On Thu, 07 Nov 2013 18:32:08 +0100, Doug Evans wrote: > I don't mind such changes, but these are changes. Agreed? So far I have expexted testsuite should follow the GDB coding standards and reviewers only have various reasons (*) why not to enforce the coding standards so strictly (or at all) for the testsuite. (*) save time of both the submitter and reviewer, making patch acceptance easier for submitters etc. > I was trying to end the thread, and make some minimal mutually agreeable > progress. I would also like so. Jan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-07 19:03 ` Jan Kratochvil @ 2013-11-08 17:57 ` Doug Evans 2013-11-08 19:17 ` Jan Kratochvil 0 siblings, 1 reply; 23+ messages in thread From: Doug Evans @ 2013-11-08 17:57 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Tom Tromey, gdb-patches, Pedro Alves, Stan Shebs Jan Kratochvil writes: > On Thu, 07 Nov 2013 18:32:08 +0100, Doug Evans wrote: > > I don't mind such changes, but these are changes. Agreed? > > So far I have expexted testsuite should follow the GDB coding standards and > reviewers only have various reasons (*) why not to enforce the coding > standards so strictly (or at all) for the testsuite. All the GDB coding standards? The testsuite is replete with various violations. [One might suggest requiring new tests to explicitly mark themselves as standard-compliant or non-compliant so that we can pass the plethora of -Wfoo that we pass for GDB. I wouldn't disagree that that's perhaps too much. :-)] I don't have a strong opinion, so I'm not the one you have to convince. [I do have a strong opinion that whatever the rules are, they be written down of course.] > (*) save time of both the submitter and reviewer, making patch acceptance > easier for submitters etc. > > > > I was trying to end the thread, and make some minimal mutually agreeable > > progress. > > I would also like so. I feel more comfortable getting approval for the modest changes I've proposed, making some progress, while leaving the general discussion to another thread. Either way is fine with me. No one has objected to my proposal (which isn't necessarily decisive of course). Propose yours and see what happens (please start a separate thread though so the Subject line is more appropriate). Being a more substantive change, I'd feel more comfortable with explicit approval from all the GMs, as opposed to passive non-disapproval. :-) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-08 17:57 ` Doug Evans @ 2013-11-08 19:17 ` Jan Kratochvil 2013-11-12 18:46 ` Doug Evans 0 siblings, 1 reply; 23+ messages in thread From: Jan Kratochvil @ 2013-11-08 19:17 UTC (permalink / raw) To: Doug Evans; +Cc: Tom Tromey, gdb-patches, Pedro Alves, Stan Shebs On Fri, 08 Nov 2013 18:37:58 +0100, Doug Evans wrote: > I'd feel more comfortable with explicit approval from all the GMs, > as opposed to passive non-disapproval. :-) I have only expressed my opinion and giving a passive non-disapproval to whatever you think is right. Not having to follow the GNU coding style for example permits at least the testcases to be written in a common modern coding style people already know. Jan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-08 19:17 ` Jan Kratochvil @ 2013-11-12 18:46 ` Doug Evans 2013-11-12 19:58 ` Jan Kratochvil 0 siblings, 1 reply; 23+ messages in thread From: Doug Evans @ 2013-11-12 18:46 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Tom Tromey, gdb-patches, Pedro Alves, Stan Shebs On Fri, Nov 8, 2013 at 9:50 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Fri, 08 Nov 2013 18:37:58 +0100, Doug Evans wrote: >> I'd feel more comfortable with explicit approval from all the GMs, >> as opposed to passive non-disapproval. :-) > > I have only expressed my opinion and giving a passive non-disapproval to > whatever you think is right. Not having to follow the GNU coding style for > example permits at least the testcases to be written in a common modern coding > style people already know. Ok, cool. I wrote this: https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Standards Feel free to tweak or add whatever community rules there are. I'll add more as it occurs to me. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-12 18:46 ` Doug Evans @ 2013-11-12 19:58 ` Jan Kratochvil 2013-11-12 22:05 ` Doug Evans 0 siblings, 1 reply; 23+ messages in thread From: Jan Kratochvil @ 2013-11-12 19:58 UTC (permalink / raw) To: Doug Evans; +Cc: Tom Tromey, gdb-patches, Pedro Alves, Stan Shebs On Tue, 12 Nov 2013 19:33:33 +0100, Doug Evans wrote: > https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Standards > > Feel free to tweak or add whatever community rules there are. > I'll add more as it occurs to me. FYI I find In C code, don't use K&R function definitions redundant against the part above The best guidance one can give is that unless a test requires a particular style (which is rare) you can't go wrong following the GDB coding standards. but it is OK even as a redundant text. Jan ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-12 19:58 ` Jan Kratochvil @ 2013-11-12 22:05 ` Doug Evans 0 siblings, 0 replies; 23+ messages in thread From: Doug Evans @ 2013-11-12 22:05 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Tom Tromey, gdb-patches, Pedro Alves, Stan Shebs On Tue, Nov 12, 2013 at 10:45 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Tue, 12 Nov 2013 19:33:33 +0100, Doug Evans wrote: >> https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Standards >> >> Feel free to tweak or add whatever community rules there are. >> I'll add more as it occurs to me. > > FYI I find > In C code, don't use K&R function definitions > > redundant against the part above > The best guidance one can give is that unless a test requires > a particular style (which is rare) you can't go wrong following the > GDB coding standards. > > but it is OK even as a redundant text. Eh? It is not redundant, that was the whole point of this thread. Following the GDB coding standards is not a rule. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-05 17:22 ` Doug Evans 2013-11-05 17:23 ` Jan Kratochvil @ 2013-11-05 17:32 ` Pedro Alves 2013-11-05 17:32 ` Tom Tromey 2 siblings, 0 replies; 23+ messages in thread From: Pedro Alves @ 2013-11-05 17:32 UTC (permalink / raw) To: Doug Evans; +Cc: Tom Tromey, Jan Kratochvil, gdb-patches, Cary Coutant On 11/05/2013 05:04 PM, Doug Evans wrote: > On Mon, Nov 4, 2013 at 6:34 PM, Tom Tromey <tromey@redhat.com> wrote: >>>> (void) >> >> Doug> We don't have any such style rules for testcases. >> Doug> But ok, done. >> >> Doug> Going forward though, for my own patch reviews of other people's code, >> Doug> what's the story here? >> >> In this case, "()" is not idiomatic C, whereas "(void)" is. >> This is not the same as a coding style rule. > > I'm not sure how to read this. > > Is this an argument for requiring (void) in all testsuite C function > definitions? > It's ok by me, but it seems to me it's not a requirement today as > there are plenty of existing examples, including recent ones. OTOH, > if there is such a requirement we'd better get it written down so we > can refer to it when requesting corrections in patches, and so people > can know ahead of time what's expected. Obviously the rules state > this for gdb itself, but it's been my understanding that these rules > explicitly do not apply to the testsuite, and this understanding has > been affirmed from time to time. All I'm asking for is clarity and > consistency. > > Or is this just a point about a bad use of the word "style"? > By itself "style" is a pretty nondescript word. > Alas I'm not one for always assigning names with precision. > If this isn't a style issue, coding *or* otherwise, let me know what to call it. > > If this is something else, let me know that too. :-) I wouldn't say this is a style issue, or even idiomatic vs not idiomatic. I'd just call it poorly written C code. In C, unlike C++, () is different from (void). (void) declares a function that takes no parameters while () declares a function that take any number of parameters. I.e., this compiles fine: void foo () {} void bar () { foo (1, 2, 3); } While this does not: void foo (void) {} void bar (void) { foo (1, 2, 3); } So, in C, use of () is usually a bug waiting to happen. In terms of coding style, GDB should be able to debug code of all sort of styles, so we aren't usually strict on having tests follow GDB's own coding style. Myself, I'll only point out coding style issues in test code if most of the code is clearly following GDB's coding style, except a place or too. E.g., if there's a whole series of comments that have double space after period except this one spot. Or if the test has 10 functions and function calls, and all them have space before parens, except one place, etc. Personally, I'm all for "write for what mean", so if the test's functions in question are not even remotely about () vs (void), I also see no point in writing (), and I'd probably also point it out if I were to review it, expecting it to be just the usual C vs C++ confusion. -- Pedro Alves ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-05 17:22 ` Doug Evans 2013-11-05 17:23 ` Jan Kratochvil 2013-11-05 17:32 ` Pedro Alves @ 2013-11-05 17:32 ` Tom Tromey 2 siblings, 0 replies; 23+ messages in thread From: Tom Tromey @ 2013-11-05 17:32 UTC (permalink / raw) To: Doug Evans; +Cc: Jan Kratochvil, gdb-patches, Cary Coutant >> In this case, "()" is not idiomatic C, whereas "(void)" is. >> This is not the same as a coding style rule. Doug> I'm not sure how to read this. Feel free to ignore it. I think this thread already outweighs its utility. It's fine to leave the test case as you wrote it. Doug> Is this an argument for requiring (void) in all testsuite C function Doug> definitions? No. Tom ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix Gold/strip discrepancies for PR 11786 2013-11-04 22:38 ` Doug Evans 2013-11-04 23:04 ` Cary Coutant 2013-11-05 3:42 ` Tom Tromey @ 2013-11-05 17:04 ` Jan Kratochvil 2 siblings, 0 replies; 23+ messages in thread From: Jan Kratochvil @ 2013-11-05 17:04 UTC (permalink / raw) To: Doug Evans; +Cc: gdb-patches, Cary Coutant On Mon, 04 Nov 2013 23:21:33 +0100, Doug Evans wrote: > On Thu, Oct 31, 2013 at 8:49 AM, Jan Kratochvil > > 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). OK, why not. Jan ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2013-11-12 21:17 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-10-25 23:26 [PATCH] Fix Gold/strip discrepancies for PR 11786 Doug Evans 2013-10-30 23:57 ` Doug Evans 2013-10-31 16:42 ` Jan Kratochvil 2013-11-04 22:38 ` Doug Evans 2013-11-04 23:04 ` Cary Coutant 2013-11-05 3:42 ` Tom Tromey 2013-11-05 17:22 ` Doug Evans 2013-11-05 17:23 ` Jan Kratochvil 2013-11-05 18:01 ` Doug Evans 2013-11-05 18:13 ` Jan Kratochvil 2013-11-06 21:16 ` Doug Evans 2013-11-06 21:28 ` Jan Kratochvil 2013-11-07 1:05 ` Stan Shebs 2013-11-07 18:01 ` Doug Evans 2013-11-07 19:03 ` Jan Kratochvil 2013-11-08 17:57 ` Doug Evans 2013-11-08 19:17 ` Jan Kratochvil 2013-11-12 18:46 ` Doug Evans 2013-11-12 19:58 ` Jan Kratochvil 2013-11-12 22:05 ` Doug Evans 2013-11-05 17:32 ` Pedro Alves 2013-11-05 17:32 ` Tom Tromey 2013-11-05 17:04 ` Jan Kratochvil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).