[PATCH v2 0/1] Don't rewind PC for GHC generated frames

[PATCH v2 0/1] Don't rewind PC for GHC generated frames

[Bartosz Nitka]

After a failed attempt of sending this via gmail web client
I'm trying again with git-send-email. I think part of the
problem was that my vim was set up to remove trailing spaces
and I generated the patch with git diff -w.
Hopefully it works this time.

Bartosz Nitka (1):
  Don't rewind PC for GHC generated frames

 gdb/ChangeLog    | 7 +++++++
 gdb/dwarf2read.c | 4 ++++
 gdb/frame.c      | 9 ++++++++-
 gdb/symtab.h     | 3 +++
 4 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.13.5

[PATCH v2 1/1] Don't rewind PC for GHC generated frames

[Bartosz Nitka]

GHC - the Haskell compiler generates code that violates one of
GDB's assumptions.

GDB assumes that the address in a frame was generated by the call
instruction and that it is the address of the call instruction
plus 1 (I'm rephrasing the comment in get_frame_address_in_block).
So to get the real address, one has to substract 1. This is doubly
beneficial because some functions are "noreturn" and don't have
further instructions after call, so GDB would be looking at gibberish.

GHC generates completely different code. It uses jumps instead of call
and manages the stack itself. Furthermore every piece of code is
preceeded
by some metadata called the Info Table. If we substract from the
program counter it ends up pointing to the metadata, which is
undesirable.
GHC has a workaround for this [1] that works most of the time, it
basically
lies in the DWARF data and extends the function one byte backwards.
That helps with making unwinding succeed most of the time, but then the
address is also used for looking up symbols and they can't be resolved.

This change disables program counter rewinding for GHC generated
compilation
units.

Some additional context can be found here [2].

[1]
https://phabricator.haskell.org/diffusion/GHC/browse/master/compiler/nativeGen/Dwarf/Types.hs;e9ae0cae9eb6a340473b339b5711ae76c6bdd045$399-417
[2] https://ghc.haskell.org/trac/ghc/wiki/DWARF

gdb/ChangeLog:

       * dwarf2read.c (process_full_comp_unit): Populate
       * producer_is_ghc.
       * frame.c (get_frame_address_in_block): Don't rewind the program
       counter for code generated by GHC.
       * symtab.h (struct compunit_symtab): Add producer_is_ghc.
---
 gdb/ChangeLog    | 7 +++++++
 gdb/dwarf2read.c | 4 ++++
 gdb/frame.c      | 9 ++++++++-
 gdb/symtab.h     | 3 +++
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3ce980c8c3..6b35bf34b6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2018-02-01  Bartosz Nitka  <niteria@gmail.com>
+
+	* dwarf2read.c (process_full_comp_unit): Populate producer_is_ghc.
+	* frame.c (get_frame_address_in_block): Don't rewind the program
+	counter for code generated by GHC.
+	* symtab.h (struct compunit_symtab): Add producer_is_ghc.
+
 2018-02-01  Yao Qi  <yao.qi@linaro.org>
 
 	* arm-tdep.c (arm_record_data_proc_misc_ld_str): Rewrite it.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 51d0f39f75..2516c48741 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -10501,6 +10501,10 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
 	cust->epilogue_unwind_valid = 1;
 
       cust->call_site_htab = cu->call_site_htab;
+
+      if (startswith (cu->producer,
+            "The Glorious Glasgow Haskell Compilation System"))
+        cust->producer_is_ghc = 1;
     }
 
   if (dwarf2_per_objfile->using_index)
diff --git a/gdb/frame.c b/gdb/frame.c
index 1384ecca4f..9ff0dcb130 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2458,7 +2458,14 @@ get_frame_address_in_block (struct frame_info *this_frame)
       && (get_frame_type (this_frame) == NORMAL_FRAME
 	  || get_frame_type (this_frame) == TAILCALL_FRAME
 	  || get_frame_type (this_frame) == INLINE_FRAME))
-    return pc - 1;
+    {
+      /* GHC intermixes metadata (info tables) with code, going back is
+         guaranteed to land us in the metadata.  */
+      struct compunit_symtab *cust = find_pc_compunit_symtab (pc);
+      if (cust != NULL && cust->producer_is_ghc)
+        return pc;
+      return pc - 1;
+    }
 
   return pc;
 }
diff --git a/gdb/symtab.h b/gdb/symtab.h
index f9d52e7697..c164e5ba5f 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1432,6 +1432,9 @@ struct compunit_symtab
      instruction).  This is supported by GCC since 4.5.0.  */
   unsigned int epilogue_unwind_valid : 1;
 
+  /* This CU was produced by Glasgow Haskell Compiler */
+  unsigned int producer_is_ghc : 1;
+
   /* struct call_site entries for this compilation unit or NULL.  */
   htab_t call_site_htab;
 
-- 
2.13.5

Re: [PATCH v2 1/1] Don't rewind PC for GHC generated frames

[Simon Marchi]

Hi Bartosz,

Thanks for resending it, I am able to apply it now.  For those who
would like to play with the patch, there are good instructions on
the page Bartosz linked:

  https://ghc.haskell.org/trac/ghc/wiki/DWARF

The change seems good, but I'd like to understand the impact better.
Could you give examples of things that didn't work before and that
do work now?  It would be a good idea to add that to your commit log
too.

On 2018-02-01 06:06 PM, Bartosz Nitka wrote:
> GHC - the Haskell compiler generates code that violates one of
> GDB's assumptions.
> 
> GDB assumes that the address in a frame was generated by the call
> instruction and that it is the address of the call instruction
> plus 1 (I'm rephrasing the comment in get_frame_address_in_block).
> So to get the real address, one has to substract 1. This is doubly
> beneficial because some functions are "noreturn" and don't have
> further instructions after call, so GDB would be looking at gibberish.

To be more precise, the pc points to the instruction after the call which
is more than 1 byte after the address of the call (in the case of x86).
The pc returned by get_frame_address_in_block (pc - 1) isn't really a
valid PC, because it doesn't point at the beginning of an instruction.
But it's good enough because we are sure it's a value that falls within
the current block range.

Thanks,

Simon