[PATCH] Fix Gold/strip discrepancies for PR 11786

[PATCH] Fix Gold/strip discrepancies for PR 11786

[Doug Evans]

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"

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

[Doug Evans]

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.

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

[Jan Kratochvil]

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

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

[Doug Evans]

[-- 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"

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

[Cary Coutant]

>> 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

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

[Tom Tromey]

>> (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

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

[Jan Kratochvil]

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

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

[Doug Evans]

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. :-)

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

[Jan Kratochvil]

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

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

[Tom Tromey]

>> 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

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

[Pedro Alves]

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

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

[Doug Evans]

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?

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

[Jan Kratochvil]

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

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

[Doug Evans]

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

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

[Jan Kratochvil]

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

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

[Stan Shebs]

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

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

[Doug Evans]

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).

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

[Jan Kratochvil]

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

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

[Doug Evans]

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. :-)

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

[Jan Kratochvil]

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

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

[Doug Evans]

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.

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

[Jan Kratochvil]

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

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

[Doug Evans]

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.