[patch+7.12.1 2/2] Fix TLS (such as 'errno') regression

[patch+7.12.1 2/2] Fix TLS (such as 'errno') regression

[Jan Kratochvil]

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

Hi,

2273f0ac95a79ce29ef42025c63f90e82cf907d7 is the first bad commit
commit 2273f0ac95a79ce29ef42025c63f90e82cf907d7
Author: Tom Tromey <tromey@redhat.com>
Date:   Tue Oct 15 13:28:57 2013 -0600
    change minsyms not to be relocated at read-time
[FYI v3 06/10] change minsyms not to be relocated at read-time
Message-Id: <1393441273-32268-7-git-send-email-tromey@redhat.com>
https://sourceware.org/ml/gdb-patches/2014-02/msg00798.html

mv /usr/lib/debug /usr/lib/debug-x
echo 'int main(){}'|gcc -pthread -x c -
./gdb -q -ex start -ex 'set debug expr 1' -ex 'p errno' ./a.out 
	    0  UNOP_MEMVAL_TLS       TLS type @0x35df7e0 (__thread /* "/lib64/libc.so.6" */ <thread local variable, no debug info>)
	    4    OP_LONG               Type @0x35df850 (__CORE_ADDR), value 140737345728528 (0x7ffff77fb010)
Cannot access memory at address 0xffffef7c9698
->
	    0  UNOP_MEMVAL_TLS       TLS type @0x3ad9520 (__thread /* "/lib64/libc.so.6" */ <thread local variable, no debug info>)
	    4    OP_LONG               Type @0x3ad9590 (__CORE_ADDR), value 16 (0x10)
$1 = 0

With glibc debuginfo, that is without: mv /usr/lib/debug /usr/lib/debug-x
	    0  OP_VAR_VALUE          Block @0x3b30e70, symbol @0x3b30d10 (errno)
$1 = 0
So such case is unrelated to this patch and the regression is not visible with
glibc debuginfo installed.

I guess all these issues will be solved by Gary Benson's Infinity.
But at least for older non-Infinity glibcs GDB should not regress.

For the testcase it is important the variable is in objfile with non-zero base
address.  glibc is a shared library for 'errno' but I found easier for the
testcase to use PIE instead of a shlib.  For TLS variables in PT_EXEC the
regression obviously does not happen.

It has been found by a more complete testcase present in Fedora, the fix there
also solves more cases where FSF GDB currently cannot resolve 'errno':
	http://pkgs.fedoraproject.org/cgit/rpms/gdb.git/tree/gdb-6.5-bz185337-resolve-tls-without-debuginfo-v2.patch
	FAIL: gdb.dwarf2/dw2-errno2.exp: macros=N threads=Y: print errno for core

No regressions on {x86_64,x86_64-m32,i686}-fedora26pre-linux-gnu.

OK for check-in?


Thanks,
Jan

[-- Attachment #2: tls1-2.patch --]
[-- Type: text/plain, Size: 3354 bytes --]

gdb/ChangeLog
2016-10-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* parse.c (write_exp_msymbol): Fix ADDR computation.

gdb/testsuite/ChangeLog
2016-10-09  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/tls-nodebug-pie.c: New file.
	* gdb.threads/tls-nodebug-pie.exp: New file.

--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -480,13 +480,17 @@ write_exp_msymbol (struct parser_state *ps,
   struct objfile *objfile = bound_msym.objfile;
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
 
-  CORE_ADDR addr = BMSYMBOL_VALUE_ADDRESS (bound_msym);
   struct obj_section *section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
   enum minimal_symbol_type type = MSYMBOL_TYPE (msymbol);
-  CORE_ADDR pc;
+  CORE_ADDR addr, pc;
   const int is_tls = (section != NULL
 		      && section->the_bfd_section->flags & SEC_THREAD_LOCAL);
 
+  if (is_tls)
+    addr = MSYMBOL_VALUE_RAW_ADDRESS (bound_msym.minsym);
+  else
+    addr = BMSYMBOL_VALUE_ADDRESS (bound_msym);
+
   /* The minimal symbol might point to a function descriptor;
      resolve it to the actual code address instead.  */
   pc = gdbarch_convert_from_func_ptr_addr (gdbarch, addr, &current_target);
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/tls-nodebug-pie.c
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 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/>.  */
+
+#include <pthread.h>
+
+__thread int thread_local = 42;
+
+int main(void)
+{
+  /* Ensure we link against pthreads even with --as-needed.  */
+  pthread_testcancel();
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/tls-nodebug-pie.exp
@@ -0,0 +1,29 @@
+# Copyright 2016 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/>.
+
+standard_testfile
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
+			  [list "additional_flags=-fPIE -pie"]] != "" } {
+    return -1
+}
+
+clean_restart ${binfile}
+if ![runto_main] then {
+   return 0
+}
+
+# Formerly: Cannot access memory at address 0xd5554d5216fc
+gdb_test "p thread_local" " = 42" "thread local storage"

Re: [patch+7.12.1 2/2] Fix TLS (such as 'errno') regression

[Mark Wielaard]

On Sun, 2016-10-09 at 20:56 +0200, Jan Kratochvil wrote:
> 2273f0ac95a79ce29ef42025c63f90e82cf907d7 is the first bad commit
> commit 2273f0ac95a79ce29ef42025c63f90e82cf907d7
> Author: Tom Tromey <tromey@redhat.com>
> Date:   Tue Oct 15 13:28:57 2013 -0600
>     change minsyms not to be relocated at read-time
> [FYI v3 06/10] change minsyms not to be relocated at read-time
> Message-Id: <1393441273-32268-7-git-send-email-tromey@redhat.com>
> https://sourceware.org/ml/gdb-patches/2014-02/msg00798.html
> 
> mv /usr/lib/debug /usr/lib/debug-x
> echo 'int main(){}'|gcc -pthread -x c -
> ./gdb -q -ex start -ex 'set debug expr 1' -ex 'p errno' ./a.out 
> 	    0  UNOP_MEMVAL_TLS       TLS type @0x35df7e0 (__thread /* "/lib64/libc.so.6" */ <thread local variable, no debug info>)
> 	    4    OP_LONG               Type @0x35df850 (__CORE_ADDR), value 140737345728528 (0x7ffff77fb010)
> Cannot access memory at address 0xffffef7c9698
> ->
> 	    0  UNOP_MEMVAL_TLS       TLS type @0x3ad9520 (__thread /* "/lib64/libc.so.6" */ <thread local variable, no debug info>)
> 	    4    OP_LONG               Type @0x3ad9590 (__CORE_ADDR), value 16 (0x10)
> $1 = 0
> 
> With glibc debuginfo, that is without: mv /usr/lib/debug /usr/lib/debug-x
> 	    0  OP_VAR_VALUE          Block @0x3b30e70, symbol @0x3b30d10 (errno)
> $1 = 0
> So such case is unrelated to this patch and the regression is not visible with
> glibc debuginfo installed.

FYI. This issue, cause and fix look very similar to PR varobj/18564
"regression in showing __thread so extern variable".

Might there be other places that need similar adjustments for getting at
the values of tls variables?

Re: [patch+7.12.1 2/2] Fix TLS (such as 'errno') regression

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> For the testcase it is important the variable is in objfile with non-zero base
Jan> address.

My only question about this patch is why the symbol's section has a
non-zero section offset.  Is that the reason why?

Maybe it would make sense to make a SEC_THREAD_LOCAL section always have
a zero offset.  Then touching all the users wouldn't be necessary.

On the third hand it seems strange to even try to get the "address" of a
TLS symbol in this way.

Tom

Re: [patch+7.12.1 2/2] Fix TLS (such as 'errno') regression

[Jan Kratochvil]

On Mon, 10 Oct 2016 16:50:38 +0200, Tom Tromey wrote:
> My only question about this patch is why the symbol's section has a
> non-zero section offset.  Is that the reason why?
> 
> Maybe it would make sense to make a SEC_THREAD_LOCAL section always have
> a zero offset.  Then touching all the users wouldn't be necessary.

IIUC you mean this subexpression could be zero (which it is not):
	ANOFFSET ((objfile)->section_offsets, ((symbol)->mginfo.section)

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [19] .tdata            PROGBITS        0000000000200db4 000db4 000004 00 WAT  0   0  4
Symbol table '.symtab' contains 71 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
    54: 0000000000000000     4 TLS     GLOBAL DEFAULT   19 thread_local

All the sections have the same offset for PIE - the PIE load base address.

And I do not think it is correct to set that offset to zero for SHF_TLS as the
thread initialization data is really located at the section address relocated
by that PIE-load-base address.


> On the third hand it seems strange to even try to get the "address" of a
> TLS symbol in this way.

There could be a symbol's getter which asserts/throws on reading address of
a TLS symbol?  Maybe, not a part of this patch.


Jan

Re: [patch+7.12.1 2/2] Fix TLS (such as 'errno') regression

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> And I do not think it is correct to set that offset to zero for
Jan> SHF_TLS as the thread initialization data is really located at the
Jan> section address relocated by that PIE-load-base address.

If it's useful then that makes sense.

>> On the third hand it seems strange to even try to get the "address" of a
>> TLS symbol in this way.

Jan> There could be a symbol's getter which asserts/throws on reading
Jan> address of a TLS symbol?

Yes.

Tom

Re: [patch+7.12.1 2/2] Fix TLS (such as 'errno') regression

[Pedro Alves]

On 10/09/2016 07:56 PM, Jan Kratochvil wrote:
> Hi,
> 
> 2273f0ac95a79ce29ef42025c63f90e82cf907d7 is the first bad commit
> commit 2273f0ac95a79ce29ef42025c63f90e82cf907d7
> Author: Tom Tromey <tromey@redhat.com>
> Date:   Tue Oct 15 13:28:57 2013 -0600
>     change minsyms not to be relocated at read-time
> [FYI v3 06/10] change minsyms not to be relocated at read-time
> Message-Id: <1393441273-32268-7-git-send-email-tromey@redhat.com>
> https://sourceware.org/ml/gdb-patches/2014-02/msg00798.html
> 
> mv /usr/lib/debug /usr/lib/debug-x
> echo 'int main(){}'|gcc -pthread -x c -
> ./gdb -q -ex start -ex 'set debug expr 1' -ex 'p errno' ./a.out 
> 	    0  UNOP_MEMVAL_TLS       TLS type @0x35df7e0 (__thread /* "/lib64/libc.so.6" */ <thread local variable, no debug info>)
> 	    4    OP_LONG               Type @0x35df850 (__CORE_ADDR), value 140737345728528 (0x7ffff77fb010)
> Cannot access memory at address 0xffffef7c9698
> ->
> 	    0  UNOP_MEMVAL_TLS       TLS type @0x3ad9520 (__thread /* "/lib64/libc.so.6" */ <thread local variable, no debug info>)
> 	    4    OP_LONG               Type @0x3ad9590 (__CORE_ADDR), value 16 (0x10)
> $1 = 0
> 
> With glibc debuginfo, that is without: mv /usr/lib/debug /usr/lib/debug-x
> 	    0  OP_VAR_VALUE          Block @0x3b30e70, symbol @0x3b30d10 (errno)
> $1 = 0
> So such case is unrelated to this patch and the regression is not visible with
> glibc debuginfo installed.
> 
> I guess all these issues will be solved by Gary Benson's Infinity.
> But at least for older non-Infinity glibcs GDB should not regress.
> 
> For the testcase it is important the variable is in objfile with non-zero base
> address.  glibc is a shared library for 'errno' but I found easier for the
> testcase to use PIE instead of a shlib.  For TLS variables in PT_EXEC the
> regression obviously does not happen.
> 
> It has been found by a more complete testcase present in Fedora, the fix there
> also solves more cases where FSF GDB currently cannot resolve 'errno':
> 	http://pkgs.fedoraproject.org/cgit/rpms/gdb.git/tree/gdb-6.5-bz185337-resolve-tls-without-debuginfo-v2.patch
> 	FAIL: gdb.dwarf2/dw2-errno2.exp: macros=N threads=Y: print errno for core
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora26pre-linux-gnu.
> 
> OK for check-in?

Looking at the backlog, I noticed today that this patch wasn't
in master yet.  Since I had errno things swapped in, I took a
look, found that it looks good to me, and pushed it in.

The patch is essentially unmodified, though I did a few minor tweaks:

 - we now need a cast to int in the testcase, since gdb no longer
   assumes no-debug-info variables have type int.
 - use "bool".
 - add a comment.
 - tried to clarify/simplify the commit log a bit.
   - note: removed "set debug expr 1" output because
     UNOP_MEMVAL_TLS no longer exists.
 - likewise ChangeLog.

Thanks, and sorry for the delay in getting this in.

From fbd1b77155bd8139033b72871dbe7bf5be6031b1 Mon Sep 17 00:00:00 2001
From: Jan Kratochvil <jan.kratochvil@redhat.com>
Date: Wed, 6 Sep 2017 12:32:46 +0100
Subject: [PATCH] Fix accessing TLS variables with no debug info

Since 2273f0ac95a7 ("change minsyms not to be relocated at
read-time"), printing TLS symbols of objfiles with a non-zero base
address, without debug info, fails.

E.g., with:

 $ mv /usr/lib/debug /usr/lib/debug-x

to get debug info out of the way, we get:

 $ echo 'int main(){}' | gcc -pthread -x c -
 $ ./gdb -q -ex start -ex 'p (int) errno' ./a.out
 Cannot access memory at address 0xffffef7c0698

instead of the expected:

 $1 = 0

The regression is not visible with glibc debuginfo installed.

The problem is that we compute the address of TLS minsyms incorrectly.

To trigger the problem, it is important that the variable is in an
objfile with a non-zero base address.  While glibc is a shared library
for 'errno', it's easier for the testcase to use PIE instead of a
shlib.  For TLS variables in PT_EXEC the regression obviously does not
happen.

gdb/ChangeLog
2017-09-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* parse.c (find_minsym_type_and_address): Don't relocate addresses
	of TLS symbols.

gdb/testsuite/ChangeLog
2017-09-06  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/tls-nodebug-pie.c: New file.
	* gdb.threads/tls-nodebug-pie.exp: New file.
---
 gdb/ChangeLog                                 |  5 +++++
 gdb/testsuite/ChangeLog                       |  5 +++++
 gdb/parse.c                                   | 12 +++++++++--
 gdb/testsuite/gdb.threads/tls-nodebug-pie.c   | 28 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.threads/tls-nodebug-pie.exp | 29 +++++++++++++++++++++++++++
 5 files changed, 77 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/tls-nodebug-pie.c
 create mode 100644 gdb/testsuite/gdb.threads/tls-nodebug-pie.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6d2eae5..ee15c60 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-09-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* parse.c (find_minsym_type_and_address): Don't relocate addresses
+	of TLS symbols.
+
 2017-09-05  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
 
 	* objfiles.c (get_objfile_bfd_data): Remove useless obstack_init
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b3bed5c..3f64c6c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2017-09-06  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.threads/tls-nodebug-pie.c: New file.
+	* gdb.threads/tls-nodebug-pie.exp: New file.
+
 2017-09-05  Tom Tromey  <tom@tromey.com>
 
 	* lib/gdb.exp (gdb_compile): Don't use universal_compile_options
diff --git a/gdb/parse.c b/gdb/parse.c
index 7971f6c..a11689b 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -491,11 +491,19 @@ find_minsym_type_and_address (minimal_symbol *msymbol,
 {
   bound_minimal_symbol bound_msym = {msymbol, objfile};
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
-  CORE_ADDR addr = BMSYMBOL_VALUE_ADDRESS (bound_msym);
   struct obj_section *section = MSYMBOL_OBJ_SECTION (objfile, msymbol);
   enum minimal_symbol_type type = MSYMBOL_TYPE (msymbol);
   CORE_ADDR pc;
 
+  bool is_tls = (section != NULL
+		 && section->the_bfd_section->flags & SEC_THREAD_LOCAL);
+
+  /* Addresses of TLS symbols are really offsets into a
+     per-objfile/per-thread storage block.  */
+  CORE_ADDR addr = (is_tls
+		    ? MSYMBOL_VALUE_RAW_ADDRESS (bound_msym.minsym)
+		    : BMSYMBOL_VALUE_ADDRESS (bound_msym));
+
   /* The minimal symbol might point to a function descriptor;
      resolve it to the actual code address instead.  */
   pc = gdbarch_convert_from_func_ptr_addr (gdbarch, addr, &current_target);
@@ -525,7 +533,7 @@ find_minsym_type_and_address (minimal_symbol *msymbol,
   if (overlay_debugging)
     addr = symbol_overlayed_address (addr, section);
 
-  if (section && section->the_bfd_section->flags & SEC_THREAD_LOCAL)
+  if (is_tls)
     {
       /* Skip translation if caller does not need the address.  */
       if (address_p != NULL)
diff --git a/gdb/testsuite/gdb.threads/tls-nodebug-pie.c b/gdb/testsuite/gdb.threads/tls-nodebug-pie.c
new file mode 100644
index 0000000..c2f62f2
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/tls-nodebug-pie.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016-2017 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/>.  */
+
+#include <pthread.h>
+
+__thread int thread_local = 42;
+
+int
+main (void)
+{
+  /* Ensure we link against pthreads even with --as-needed.  */
+  pthread_testcancel ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/tls-nodebug-pie.exp b/gdb/testsuite/gdb.threads/tls-nodebug-pie.exp
new file mode 100644
index 0000000..ca384a0
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/tls-nodebug-pie.exp
@@ -0,0 +1,29 @@
+# Copyright 2016-2017 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/>.
+
+standard_testfile
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
+			  [list "additional_flags=-fPIE -pie"]] != "" } {
+    return -1
+}
+
+clean_restart ${binfile}
+if ![runto_main] then {
+    return 0
+}
+
+# Formerly: Cannot access memory at address 0xffffef7c0698
+gdb_test "p (int) thread_local" " = 42" "thread local storage"
-- 
2.5.5