[PATCH 0/2] PR 19947: throw right exception in read_code and read_stack

[PATCH 0/2] PR 19947: throw right exception in read_code and read_stack

[Yao Qi]

Hi,
Patch 1 triggers PR 19947, because it uses -fno-asynchronous-unwind-tables
in the compilation of test case, so that there is no DWARF info, and GDB
has to choose prologue unwinder.  In this way, the prologue unwinder can
be tested under the situation that memory/register is not available.

Patch 2 is the fix, by throwing the right exception in read_code and
read_stack.  See details in patch 2.

Regression tested on x86_64-linux.

*** BLURB HERE ***

Yao Qi (2):
  Use -fno-asynchronous-unwind-tables if C program is compiled without
    debug info on x86
  Throw NOT_AVAILABLE_ERROR in read_stack and read_code

 gdb/corefile.c            | 29 +++++++++++++++--------------
 gdb/testsuite/lib/gdb.exp |  9 +++++++++
 2 files changed, 24 insertions(+), 14 deletions(-)

-- 
1.9.1

[PATCH 1/2] Use -fno-asynchronous-unwind-tables if C program is compiled without debug info on x86

[Yao Qi]

.eh_frame is added in default on x86 and x86_64 so that DWARF unwinder
is always used, but prologue unwinders won't be used at all.  IOW,
prologue unwinders are not tested by regression tests.  If the test is
intended to compiled *without* debug info, we shouldn't generate any
debug info (.eh_frame for example) at all.

This patch is to disable async-unwind-tables generation if the C program
is intended to compile without debug info on x86-like target.

gdb/testsuite:

2016-04-18  Pierre Langlois  <pierre.langlois@arm.com>
	    Yao Qi  <yao.qi@linaro.org>

	PR gdb/19947
	* lib/gdb.exp (gdb_compile): Append
	"additional_flags=-fno-asynchronous-unwind-tables" if C program
	is compiled without debug info on x86.
---
 gdb/testsuite/lib/gdb.exp | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 0d8a3b0..5b789af 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3419,6 +3419,15 @@ proc gdb_compile {source dest type options} {
 	set options [lreplace $options $nowarnings $nowarnings $flag]
     }
 
+    if {([lsearch -exact $options debug] == -1
+	 || [lsearch -exact $options nodebug] != -1)
+	 && ([istarget "i?86-*-*"] || [istarget "x86_64-*-*"])
+	 && ([lsearch -regexp $options {(c\+\+|ada|f77|f90)}] == -1) } {
+	# C program is intended to compile without debug information
+	# on x86 target.
+	lappend options "additional_flags=-fno-asynchronous-unwind-tables"
+    }
+
     if { $type == "executable" } {
 	if { ([istarget "*-*-mingw*"]
 	      || [istarget "*-*-*djgpp"]
-- 
1.9.1

Re: [PATCH 1/2] Use -fno-asynchronous-unwind-tables if C program is compiled without debug info on x86

[Pedro Alves]

I think the subject would be clearer if it mentioned this is about gdb tests.  E.g.:

Use -fno-asynchronous-unwind-tables on x86 for gdb C tests without debug info

On 04/19/2016 08:50 AM, Yao Qi wrote:
> .eh_frame is added in default on x86 and x86_64 so that DWARF unwinder
> is always used, but prologue unwinders won't be used at all.  IOW,
> prologue unwinders are not tested by regression tests.  If the test is
> intended to compiled *without* debug info, we shouldn't generate any
> debug info (.eh_frame for example) at all.
> 
> This patch is to disable async-unwind-tables generation if the C program
> is intended to compile without debug info on x86-like target.

No sure about this.  This is an ABI change on x86_64 -- the x86_64 ABI
requires eh_frame.

Should we instead add a new "nounwind" option, and a few
prologue-unwinder-specific tests?

Thanks,
Pedro Alves

Re: [PATCH 1/2] Use -fno-asynchronous-unwind-tables if C program is compiled without debug info on x86

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> No sure about this.  This is an ABI change on x86_64 -- the x86_64 ABI
> requires eh_frame.

This is because people want to unwind frames without using frame
pointer register  (registers are very limited on x86), so different
tools, like glibc's backtrace and libunwind, can use eh_frame to unwind
correctly.  Then, ABI requires eh_frame.

GDB is different, because it can still rely on the prologue unwinder if
no debug info.  On the other hand, GDB prologue unwinder should be
tested in case that no debug info is produced by compiler.  We just use
-fno-asynchronous-unwind-tables to produce binaries to exercise GDB, so
I don't worry about ABI incompliance.

>
> Should we instead add a new "nounwind" option, and a few
> prologue-unwinder-specific tests?

We don't know how prologue unwinder is used in each test case with
option "nodebug", so  I am afraid we need to add "nounwind" to every
test case where option "nodebug" is used.

-- 
Yao (齐尧)

Re: [PATCH 1/2] Use -fno-asynchronous-unwind-tables if C program is compiled without debug info on x86

[Pedro Alves]

On 04/20/2016 09:37 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> No sure about this.  This is an ABI change on x86_64 -- the x86_64 ABI
>> requires eh_frame.
> 
> This is because people want to unwind frames without using frame
> pointer register  (registers are very limited on x86), so different
> tools, like glibc's backtrace and libunwind, can use eh_frame to unwind
> correctly.  Then, ABI requires eh_frame.

I believe unwind frames are also necessary for pthread cancellation
and thread_cleanup_push implemented in terms of gcc's
__attribute__ __cleanup__, in C programs.

> 
> GDB is different, because it can still rely on the prologue unwinder if
> no debug info. 

Which is heuristic and can (and does) thus fail sometimes.

> On the other hand, GDB prologue unwinder should be
> tested in case that no debug info is produced by compiler.

Though that's not testing what the compiler will normally output
with just "-On" and no -g.  That's what in part what I was trying
to convey -- the ABI change means we're not testing what
people normally use.

> We just use
> -fno-asynchronous-unwind-tables to produce binaries to exercise GDB, so
> I don't worry about ABI incompliance.

Right.

> 
>>
>> Should we instead add a new "nounwind" option, and a few
>> prologue-unwinder-specific tests?
> 
> We don't know how prologue unwinder is used in each test case with
> option "nodebug", so  I am afraid we need to add "nounwind" to every
> test case where option "nodebug" is used.

I think most, if not all, "nodebug" tests will be about no symbol info,
and aren't really about the unwinder at all.

I was suggesting new tests specific for the prologue unwinder,
not changing the existing tests.  E.g., just something simple that runs
to a function and does "backtrace" making sure the expected frames are
visible.

If we want to cover more cases, maybe do a test that single-steps through
some body of code, and issues backtrace at every single-step,
prologues/epilogues included, making sure "main" is always visible
in the backtrace.

We could actually have that test try "backtrace" with dwarf unwinding,
and then the same but with the prologue unwinder.

Whether forcing use of the prologue unwinder through compilation flags
is the best way, not sure.  A "maint set force-prologue-unwinder on" command
would make it easier to make sure we test what we intend to test on all
architectures, independent of program language, and independent
of architecture.  Plus, a test unit testing the prologue unwinder could
then conveniently make use of debug info.

(If such a test steps through dynamic symbol resolution / plts, then it's
likely that it can only really work with dwarf unwinding, though I think
it'd be a nice test to have and would likely expose bugs in glibc's
cfi markers in assembly code.)

Thanks,
Pedro Alves

Re: [PATCH 1/2] Use -fno-asynchronous-unwind-tables if C program is compiled without debug info on x86

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> I think most, if not all, "nodebug" tests will be about no symbol info,
> and aren't really about the unwinder at all.
>

It is quite common that gdb tests find some bugs of some thing which
isn't these tests about.  The test was written for feature A, but it may
find bugs in feature B or C.

> I was suggesting new tests specific for the prologue unwinder,
> not changing the existing tests.  E.g., just something simple that runs
> to a function and does "backtrace" making sure the expected frames are
> visible.
>
> If we want to cover more cases, maybe do a test that single-steps through
> some body of code, and issues backtrace at every single-step,
> prologues/epilogues included, making sure "main" is always visible
> in the backtrace.
>
> We could actually have that test try "backtrace" with dwarf unwinding,
> and then the same but with the prologue unwinder.
>

That must be useful.  If debug infor is produced, it can be a good test
to compiler.

> Whether forcing use of the prologue unwinder through compilation flags
> is the best way, not sure.  A "maint set force-prologue-unwinder on" command
> would make it easier to make sure we test what we intend to test on all
> architectures, independent of program language, and independent
> of architecture.  Plus, a test unit testing the prologue unwinder could
> then conveniently make use of debug info.

We had this idea too when we wrote aarch64 tracepoint support.  We
wanted to know which unwinder is chosen by GDB for a given frame.

>
> (If such a test steps through dynamic symbol resolution / plts, then it's
> likely that it can only really work with dwarf unwinding, though I think
> it'd be a nice test to have and would likely expose bugs in glibc's
> cfi markers in assembly code.)

I want to focus on handling unavailable memory in frame unwinder, so I
cannot do all of them above.  What I can do are:

 1. Name some unwinders, such as dwarf unwinders, prologue unwinders,
 sigtramp unwinders, stub unwinders, etc.  Other unwinders are nameless.
 2. Add a new maint command "maint set/show preferred-unwinder".  This
 command tell GDB to prefer a unwinder during frame unwinding.  If the
 unwinder can't be applied to the frame (sniffer fails), it is not chosen.
 3. Prefer different unwinders (dwarf vs. prologue) in the different
 runs of gdb.trace tests, like

 foreach_with_prefix unwinder { "dwarf" "prologue" } {
   gdb_test "maint set preferred-unwinder $unwinder"
   gdb_test "tfind start"
   .....
   gdb_test "tfind stop"
 }

 so that the PR 19947 can be reproduced.  I'll stop here.

 What I don't plan to do in the short term are:

 - Write a case, let GDB single-step through its body, including
   prologue, epilogue and its callees, and check "bt" can show the
   frames correctly with different unwinders.
 - Single-step through dynamic symbol resolution / plt, prefer "dwarf"
   unwinder, and check "bt" can show the frames correctly.

Is it OK to you?

-- 
Yao (齐尧)

Re: [PATCH 1/2] Use -fno-asynchronous-unwind-tables if C program is compiled without debug info on x86

[Pedro Alves]

On 04/22/2016 03:23 PM, Yao Qi wrote:

> I want to focus on handling unavailable memory in frame unwinder, so I
> cannot do all of them above.  What I can do are:
> 
>  1. Name some unwinders, such as dwarf unwinders, prologue unwinders,
>  sigtramp unwinders, stub unwinders, etc.  Other unwinders are nameless.
>  2. Add a new maint command "maint set/show preferred-unwinder".  This
>  command tell GDB to prefer a unwinder during frame unwinding.  If the
>  unwinder can't be applied to the frame (sniffer fails), it is not chosen.

Instead of a name, maybe a frame-unwinder-class enum, based on 
https://sourceware.org/bugzilla/show_bug.cgi?id=19288#c13

~~~~~~~~~~~~~~~
- The "Accurate unwinders"

  These would be the DWARF / x64 SEH based ones.

- JIT unwinders 

  Python/Guile unwind API unwinders, and also the C JIT-reader
  API unwinder, in jit.c.

- Fallback prologue unwinders
~~~~~~~~~~~~~~~

Then it could be "required" instead of "preferred", which would
catch problems with the unwinder's sniffer as well.

>  3. Prefer different unwinders (dwarf vs. prologue) in the different
>  runs of gdb.trace tests, like
> 
>  foreach_with_prefix unwinder { "dwarf" "prologue" } {
>    gdb_test "maint set preferred-unwinder $unwinder"
>    gdb_test "tfind start"
>    .....
>    gdb_test "tfind stop"
>  }
> 
>  so that the PR 19947 can be reproduced.  I'll stop here.
> 
>  What I don't plan to do in the short term are:
> 
>  - Write a case, let GDB single-step through its body, including
>    prologue, epilogue and its callees, and check "bt" can show the
>    frames correctly with different unwinders.
>  - Single-step through dynamic symbol resolution / plt, prefer "dwarf"
>    unwinder, and check "bt" can show the frames correctly.
> 
> Is it OK to you?
> 

Yes, that sounds good to me.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Use -fno-asynchronous-unwind-tables if C program is compiled without debug info on x86

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Instead of a name, maybe a frame-unwinder-class enum, based on 
> https://sourceware.org/bugzilla/show_bug.cgi?id=19288#c13
>
> ~~~~~~~~~~~~~~~
> - The "Accurate unwinders"
>
>   These would be the DWARF / x64 SEH based ones.
>
> - JIT unwinders 
>
>   Python/Guile unwind API unwinders, and also the C JIT-reader
>   API unwinder, in jit.c.
>
> - Fallback prologue unwinders
> ~~~~~~~~~~~~~~~
>
> Then it could be "required" instead of "preferred", which would
> catch problems with the unwinder's sniffer as well.

I am fine on this, but I don't think this is helpful to PR 19288.

What we need in PR 19288 is a dynamic list of unwinders (based on
priority), so that user can change the orders of unwinders in the list
by changing their priorities.  IIUC, in PR 19288, each unwinder works
well if the desired unwinder in the chain is selected.  The problem is
the desired unwinder can't be selected in the chain.

If we want to handle these two problems together, we need command line
interface to set and change the priorities of unwinders or classes of
unwinders.  This would be useful to PR 19288.  We can set priority to -1
or 0 to disable the unwinder completely.

However, my concern on this feature is that we may expose too much to
the user, and usually, it is tricky and sensitive on changing the order of
unwinders.

-- 
Yao (齐尧)

[PATCH 2/2] Throw NOT_AVAILABLE_ERROR in read_stack and read_code

[Yao Qi]

Nowadays, read_memory may throw NOT_AVAILABLE_ERROR (it is done by
patch http://sourceware.org/ml/gdb-patches/2013-08/msg00625.html)
however, read_stack and read_code still throws MEMORY_ERROR only.  This
causes PR 19947, that is prologue unwinder is unable unwind because
code memory isn't available, but MEMORY_ERROR is thrown, while unwinder
catches NOT_AVAILABLE_ERROR.

 #0  memory_error (err=err@entry=TARGET_XFER_E_IO, memaddr=memaddr@entry=140737349781158) at /home/yao/SourceCode/gnu/gdb/git/gdb/corefile.c:217
 #1  0x000000000065f5ba in read_code (memaddr=memaddr@entry=140737349781158, myaddr=myaddr@entry=0x7fffffffd7b0 "\340\023<\001", len=len@entry=1)
     at /home/yao/SourceCode/gnu/gdb/git/gdb/corefile.c:288
 #2  0x000000000065f7b5 in read_code_unsigned_integer (memaddr=memaddr@entry=140737349781158, len=len@entry=1, byte_order=byte_order@entry=BFD_ENDIAN_LITTLE)
     at /home/yao/SourceCode/gnu/gdb/git/gdb/corefile.c:363
 #3  0x00000000004717e0 in amd64_analyze_prologue (gdbarch=gdbarch@entry=0x13c13e0, pc=140737349781158, current_pc=140737349781165, cache=cache@entry=0xda0cb0)
     at /home/yao/SourceCode/gnu/gdb/git/gdb/amd64-tdep.c:2267
 #4  0x0000000000471f6d in amd64_frame_cache_1 (cache=0xda0cb0, this_frame=0xda0bf0) at /home/yao/SourceCode/gnu/gdb/git/gdb/amd64-tdep.c:2437
 #5  amd64_frame_cache (this_frame=0xda0bf0, this_cache=<optimised out>) at /home/yao/SourceCode/gnu/gdb/git/gdb/amd64-tdep.c:2508
 #6  0x000000000047214d in amd64_frame_this_id (this_frame=<optimised out>, this_cache=<optimised out>, this_id=0xda0c50)
     at /home/yao/SourceCode/gnu/gdb/git/gdb/amd64-tdep.c:2541
 #7  0x00000000006b94c4 in compute_frame_id (fi=0xda0bf0) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:481
 #8  get_prev_frame_if_no_cycle (this_frame=this_frame@entry=0xda0b20) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1809
 #9  0x00000000006bb6c9 in get_prev_frame_always_1 (this_frame=0xda0b20) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1983
 #10 get_prev_frame_always (this_frame=this_frame@entry=0xda0b20) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1999
 #11 0x00000000006bbe11 in get_prev_frame (this_frame=this_frame@entry=0xda0b20) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:2241
 #12 0x00000000006bc13c in unwind_to_current_frame (ui_out=<optimised out>, args=args@entry=0xda0b20) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1485

The fix is to let read_stack and read_code throw NOT_AVAILABLE_ERROR too,
in order to align with read_memory.

gdb:

2016-04-18  Yao Qi  <yao.qi@linaro.org>

	PR gdb/19947
	* corefile.c (read_memory): Rename it to ...
	(read_memory_object): ... it.  Add parameter object.
	(read_memory): Call read_memory_object.
	(read_stack): Likewise.
	(read_code): Likewise.
---
 gdb/corefile.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/gdb/corefile.c b/gdb/corefile.c
index 5ad4d40..6cc2afc 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -237,10 +237,11 @@ memory_error (enum target_xfer_status err, CORE_ADDR memaddr)
   throw_error (exception, ("%s"), str);
 }
 
-/* Same as target_read_memory, but report an error if can't read.  */
+/* Helper function.  */
 
-void
-read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
+static void
+read_memory_object (enum target_object object, CORE_ADDR memaddr,
+		    gdb_byte *myaddr, ssize_t len)
 {
   ULONGEST xfered = 0;
 
@@ -250,7 +251,7 @@ read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
       ULONGEST xfered_len;
 
       status = target_xfer_partial (current_target.beneath,
-				    TARGET_OBJECT_MEMORY, NULL,
+				    object, NULL,
 				    myaddr + xfered, NULL,
 				    memaddr + xfered, len - xfered,
 				    &xfered_len);
@@ -264,16 +265,20 @@ read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
     }
 }
 
+/* Same as target_read_memory, but report an error if can't read.  */
+
+void
+read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
+{
+  read_memory_object (TARGET_OBJECT_MEMORY, memaddr, myaddr, len);
+}
+
 /* Same as target_read_stack, but report an error if can't read.  */
 
 void
 read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 {
-  int status;
-
-  status = target_read_stack (memaddr, myaddr, len);
-  if (status != 0)
-    memory_error (TARGET_XFER_E_IO, memaddr);
+  read_memory_object (TARGET_OBJECT_STACK_MEMORY, memaddr, myaddr, len);
 }
 
 /* Same as target_read_code, but report an error if can't read.  */
@@ -281,11 +286,7 @@ read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 void
 read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 {
-  int status;
-
-  status = target_read_code (memaddr, myaddr, len);
-  if (status != 0)
-    memory_error (TARGET_XFER_E_IO, memaddr);
+  read_memory_object (TARGET_OBJECT_CODE_MEMORY, memaddr, myaddr, len);
 }
 
 /* Read memory at MEMADDR of length LEN and put the contents in
-- 
1.9.1

Re: [PATCH 2/2] Throw NOT_AVAILABLE_ERROR in read_stack and read_code

[Pedro Alves]

On 04/19/2016 08:50 AM, Yao Qi wrote:

> 2016-04-18  Yao Qi  <yao.qi@linaro.org>
> 
> 	PR gdb/19947
> 	* corefile.c (read_memory): Rename it to ...
> 	(read_memory_object): ... it.  Add parameter object.
> 	(read_memory): Call read_memory_object.
> 	(read_stack): Likewise.
> 	(read_code): Likewise.

LGTM.

Thanks,
Pedro Alves

Re: [PATCH 2/2] Throw NOT_AVAILABLE_ERROR in read_stack and read_code

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> LGTM.

Patch is pushed in.

-- 
Yao (齐尧)