[PATCH 0/3] Fix gdb.base/func-ptrs.exp fails in ppc64 and arm thumb mode

[PATCH 0/3] Fix gdb.base/func-ptrs.exp fails in ppc64 and arm thumb mode

[Yao Qi]

I see some test fails in gdb.base/func-ptrs.exp in arm thumb mode,
because the LSB bit is lost when we assign a function to a function
pointer in GDB.  I proposed a patch
https://sourceware.org/ml/gdb-patches/2016-06/msg00217.html but we
decided to fix this problem rather than skip the test, and agreed
that assignment in GDB should end up the same value as we do the
same in .c file.

When I looked into this problem again, I find such problem exists
on all targets which have function descriptors, IOW, function pointer
address isn't function address, like ppc64, ia64, frv and hppa32.
(Function pointer with LSB set in arm thumb mode can be regarded
as a function descriptor as well, IMO)

This issue was discussed in GNU Cauldron this year, and I got some
helpful insights from Pedro and Ulrich.  I finally have a RFC in
patch #2, which is the major part of this patch series.  As I
mentioned in patch #2, it has some user visible changes, and you can
see these changes in patch #3.  Comments are welcome!

Regression tested on ppc64-linux (gcc110) native and arm-linux gdbserver.
I failed to run ppc64-linux tests with gdbserver, because
process-dies-while-detaching.exp, IIRC, generates a lot of zombie
GDBservers and the following tests are slowed down and timeout.  I
didn't look into them in details.

*** BLURB HERE ***

Yao Qi (3):
  Use get_var_address in test cases
  Record function descriptor address instead of function address in
    value
  Update test cases

 gdb/arch-utils.c                                   |  6 +++++
 gdb/arch-utils.h                                   |  1 +
 gdb/arm-tdep.c                                     | 23 +++++++++++++++++
 gdb/ax-gdb.c                                       | 12 ++++++---
 gdb/findvar.c                                      |  5 ++++
 gdb/gdbarch.c                                      | 23 +++++++++++++++++
 gdb/gdbarch.h                                      |  6 +++++
 gdb/gdbarch.sh                                     |  4 +++
 gdb/infcall.c                                      |  2 +-
 gdb/ppc-linux-tdep.c                               |  2 ++
 gdb/ppc64-tdep.c                                   | 30 ++++++++++++++++++++++
 gdb/ppc64-tdep.h                                   |  3 ++-
 gdb/ppcfbsd-tdep.c                                 |  2 ++
 gdb/testsuite/gdb.base/break-fun-addr.exp          |  4 +--
 gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp |  2 +-
 gdb/testsuite/gdb.base/examine-backward.exp        |  3 ++-
 gdb/testsuite/gdb.mi/mi-var-cmd.exp                |  4 +--
 gdb/testsuite/gdb.python/py-events.exp             |  7 +----
 gdb/testsuite/gdb.trace/tracecmd.exp               |  9 +++----
 gdb/testsuite/lib/dwarf.exp                        |  5 ++--
 gdb/testsuite/lib/gdb.exp                          |  3 ++-
 gdb/value.c                                        |  4 ++-
 22 files changed, 134 insertions(+), 26 deletions(-)

-- 
1.9.1

[RFC 3/3] Update test cases

[Yao Qi]

If "func" means the function descriptor,

- the compare like "$pc == func" makes no sense,

- "x/2i func+10" shows the instructions on func function descriptor
   address + 10, there shouldn't be any instructions,

- "p func_label - func" no longer shows the offset from function func
   to label func_label.

gdb/testsuite:

2016-10-14  Yao Qi  <yao.qi@linaro.org>

	* gdb.base/break-fun-addr.exp: Test whether $pc is in the start
	of function main.
	* gdb.base/breakpoint-in-ro-region.exp (get_function_bounds): Use
	function start address rather than function name.
	* gdb.base/examine-backward.exp: Stop using "x/i main".
	* gdb.mi/mi-var-cmd.exp: Match possible function descriptor
	address in the output.
	* lib/dwarf.exp (function_range): Use function address instead of
	function name.
	* lib/gdb.exp (get_var_address): Match possible function descriptor
	address in the output.
---
 gdb/testsuite/gdb.base/break-fun-addr.exp          | 4 ++--
 gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp | 2 +-
 gdb/testsuite/gdb.base/examine-backward.exp        | 3 ++-
 gdb/testsuite/gdb.mi/mi-var-cmd.exp                | 4 ++--
 gdb/testsuite/lib/dwarf.exp                        | 5 +++--
 gdb/testsuite/lib/gdb.exp                          | 3 ++-
 6 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/gdb/testsuite/gdb.base/break-fun-addr.exp b/gdb/testsuite/gdb.base/break-fun-addr.exp
index e8bed3f..ec4aeb5 100644
--- a/gdb/testsuite/gdb.base/break-fun-addr.exp
+++ b/gdb/testsuite/gdb.base/break-fun-addr.exp
@@ -57,7 +57,7 @@ with_test_prefix "${binfile1}" {
              "run to breakpoint at *main"
 
     # Verify also that we stopped at the start of the function...
-    gdb_test "p \$pc == main" " = 1"
+    gdb_test "x/i \$pc" "<main>:.*"
 }
 
 set testfile2 "break-fun-addr2"
@@ -80,5 +80,5 @@ with_test_prefix "${binfile2}" {
              "Breakpoint.* main \\(\\) at .*$srcfile2:.*" \
              "run to breakpoint at *main"
 
-    gdb_test "p \$pc == main" " = 1"
+    gdb_test "x/i \$pc" "<main>:.*"
 }
diff --git a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
index 0c954c8..d4b2529 100644
--- a/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
+++ b/gdb/testsuite/gdb.base/breakpoint-in-ro-region.exp
@@ -76,7 +76,7 @@ proc get_function_bounds {function func_lo func_hi} {
 
     # Account for the size of the last instruction.
     set test "get hi address of $function"
-    gdb_test_multiple "x/2i $function+$size" $test {
+    gdb_test_multiple "x/2i $lo+$size" $test {
 	-re ".*$hex <$function\\+$size>:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" {
 	    set hi $expect_out(1,string)
 	    pass $test
diff --git a/gdb/testsuite/gdb.base/examine-backward.exp b/gdb/testsuite/gdb.base/examine-backward.exp
index e03cbfd..16a74c7 100644
--- a/gdb/testsuite/gdb.base/examine-backward.exp
+++ b/gdb/testsuite/gdb.base/examine-backward.exp
@@ -301,7 +301,8 @@ with_test_prefix "backward disassemble general" {
     set length_to_examine {1 2 3 4 10}
     set disassmbly {}
 
-    gdb_test "x/i main" "0x\[0-9a-fA-F\]+ <main>:\t.*" \
+    set main_addr [get_var_address "main"]
+    gdb_test "x/i $main_addr" "0x\[0-9a-fA-F\]+ <main>:\t.*" \
         "move the current position to main (x/i)"
     gdb_test "x/-i" "0x\[0-9a-fA-F\]+ <main>:\t.*" \
         "move the current position to main (x/-i)"
diff --git a/gdb/testsuite/gdb.mi/mi-var-cmd.exp b/gdb/testsuite/gdb.mi/mi-var-cmd.exp
index 558cd6c..af7307d 100644
--- a/gdb/testsuite/gdb.mi/mi-var-cmd.exp
+++ b/gdb/testsuite/gdb.mi/mi-var-cmd.exp
@@ -349,7 +349,7 @@ mi_gdb_test "-var-assign ldouble 5.333318284590435" \
 	"assign to ldouble"
 
 mi_gdb_test "-var-assign func do_block_tests" \
-	"\\^done,value=\"$hex <do_block_tests>\"" \
+	"\\^done,value=\"(@$hex: )?$hex <do_block_tests>\"" \
 	"assign to func"
 
 mi_gdb_test "-var-assign lsimple.character 'd'" \
@@ -373,7 +373,7 @@ mi_gdb_test "-var-update *" \
 # pointer before comparing with the existing value, 
 # and does not incorrectly make the value as changed.
 mi_gdb_test "-var-assign func do_block_tests" \
-	"\\^done,value=\"$hex <do_block_tests>\"" \
+	"\\^done,value=\"(@$hex: )?$hex <do_block_tests>\"" \
 	"assign same value to func"
 
 mi_gdb_test "-var-update *" \
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 717cbb0..c328c9f 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -127,8 +127,9 @@ proc function_range { func src } {
 
     # Compute the label offset, and we can get the function start address
     # by "${func}_label - $func_label_offset".
+    set func_addr [get_var_address ${func}]
     set func_label_offset ""
-    set test "p ${func}_label - ${func}"
+    set test "p/d ${func}_label - ${func_addr}"
     gdb_test_multiple $test $test {
 	-re ".* = ($decimal)\r\n$gdb_prompt $" {
 	    set func_label_offset $expect_out(1,string)
@@ -151,7 +152,7 @@ proc function_range { func src } {
     } else {
 	set func_pattern "$func\\+$func_length"
     }
-    set test "x/2i $func+$func_length"
+    set test "x/2i $func_addr+$func_length"
     gdb_test_multiple $test $test {
 	-re ".*($hex) <$func_pattern>:\[^\r\n\]+\r\n\[ \]+($hex).*\.\r\n$gdb_prompt $" {
 	    set start $expect_out(1,string)
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 378eea0..364ee46 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5595,9 +5595,10 @@ proc get_var_address { var } {
     # $1 = (int *) 0x0
     # $5 = (int (*)()) 0
     # $6 = (int (*)()) 0x24 <function_bar>
+    # $7 = (int (*)()) @x12: 0x24 <function_bar>
 
     gdb_test_multiple "print &${var}" "get address of ${var}" {
-	-re "\\\$\[0-9\]+ = \\(.*\\) (0|$hex)( <${var}>)?\[\r\n\]+${gdb_prompt} $"
+	-re "\\\$\[0-9\]+ = \\(.*\\)(?: @$hex:)? (0|$hex)( <${var}>)?\[\r\n\]+${gdb_prompt} $"
 	{
 	    pass "get address of ${var}"
 	    if { $expect_out(1,string) == "0" } {
-- 
1.9.1

[PATCH 1/3] Use get_var_address in test cases

[Yao Qi]

This patch uses get_var_address more.

gdb/testsuite:

2016-10-13  Yao Qi  <yao.qi@linaro.org>

	* gdb.python/py-events.exp: Call get_var_address.
	* gdb.trace/tracecmd.exp: Call get_var_address and match possible
	leading zeros.
---
 gdb/testsuite/gdb.python/py-events.exp | 7 +------
 gdb/testsuite/gdb.trace/tracecmd.exp   | 9 ++++-----
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/gdb/testsuite/gdb.python/py-events.exp b/gdb/testsuite/gdb.python/py-events.exp
index de8de07..602fbae 100644
--- a/gdb/testsuite/gdb.python/py-events.exp
+++ b/gdb/testsuite/gdb.python/py-events.exp
@@ -88,12 +88,7 @@ gdb_test_multiple "info threads" "get current thread" {
     }
 }
 
-gdb_test_multiple "print do_nothing" "get address of do_nothing" {
-    -re "\[^\n\r\]*(0x\[0-9a-f\]+) \<do_nothing\>.*$gdb_prompt $" {
-	set addr $expect_out(1,string)
-	pass "get address of do_nothing"
-    }
-}
+set addr [get_var_address "do_nothing"]
 
 set expected [list "event type: pre-call"]
 lappend expected "ptid: \\($process_id, $process_id, 0\\)" "address: $addr"
diff --git a/gdb/testsuite/gdb.trace/tracecmd.exp b/gdb/testsuite/gdb.trace/tracecmd.exp
index 11ef691..7e1dc4b 100644
--- a/gdb/testsuite/gdb.trace/tracecmd.exp
+++ b/gdb/testsuite/gdb.trace/tracecmd.exp
@@ -114,11 +114,10 @@ gdb_test "info trace" "$asm_test_addr.*gdb_asm_test.*" \
 
 # 1.7 trace at function's exact address
 #     Collect the address of the function for comparison
-gdb_test_multiple "print gdb_recursion_test" "" {
-    -re "\[$\]\[0-9\].*0x(\[0-9a-fA-F\]+).*$gdb_prompt $" {
-	set c_test_addr $expect_out(1,string)
-    }
-}
+set c_test_addr [get_var_address "gdb_recursion_test"]
+# Match leading zeros in address.
+set c_test_addr [string range $c_test_addr 2 [expr {[string length $c_test_addr] -1}]]
+set c_test_addr "0x(0)*$c_test_addr"
 
 gdb_delete_tracepoints
 gdb_test "trace \*gdb_recursion_test" \
-- 
1.9.1

[RFC 2/3] Record function descriptor address instead of function address in value

[Yao Qi]

1. Problem statement

Nowadays, function address is recorded in value in GDB.  It is convenient
to use GDB on some targets function pointer address isn't function
address, like ppc64 and arm thumb mode.  For example, on ppc64,

(gdb) p incr
$4 = {int (int)} 0x1000069c <incr>
(gdb) disassemble incr
Dump of assembler code for function incr:
   0x000000001000069c <+0>:	mflr    r0

"incr" is the address of "incr" function, which differs from its meaning
in C language.  In .c file, "incr" is the address of "incr" function
descriptor, if we write

  printf ("incr %llx, &incr %llx\n", incr, &incr);

it prints the function descriptor address.  Then this brings confusion
in GDB when we do expression evaluation,

int incr (int i) {}
int (*calc) (int) = incr;

in C, the statement above means assign "incr" function descriptor
address to "calc", or "calc" pointers to "incr" function descriptor.
However, if we evaluation expressions in gdb with function involved,
we'll see something interesting,

(gdb) p if calc == incr

this prints 0 because calc pointers to "incr" function descriptor,
but "incr" is its function address, so they are different,

(gdb) set calc = incr

"incr" function address is assigned to "calc", but it is expected
to get function descriptor address.

2.  Solution

GCC vs GDB divergence on the meaning of "incr" brings the confusion.
We should reduce such divergence as much as we can.  However, this
divergence was added in https://sourceware.org/ml/gdb-patches/2001-11/msg00001.html
I agree with Jim, but I'd like use function descriptor address in value,
which makes the whole expression evaluation look more reasonable and
more close to compiler's behavior.

In this patch, I add a new gdbarch method convert_from_func_addr, which
converts function address back to function descriptor address or
function pointer address.  It is the reversed operation of
convert_from_func_ptr_addr.  We convert function address to function
descriptor address when,

 - we create value for a function,
 - we generate ax code for a function,

I don't change the meaning of return value of value_as_address, because
it is widely used, so value_as_address still return the function
address if type is TYPE_CODE_FUNC or TYPE_CODE_METHOD.

3. User visible changes

This patch brings several user visible changes, which look more
accurate, shown by this table below,

 COMMAND           BEFORE                       AFTER
 p main            main function address        main function descriptor
                                                address
 disass main       disassembly function main    not changed
 disass main+4,+4  disassembly 4 bytes from     disassembly 4 bytes from
                   function main address + 4    main's function descriptor + 4
 x/i main          show one instruction on      show one instruction on main's
                   function main address        function descriptor

Although the latter looks inconvenient, that is consistent to the
meaning on C language level.  Due to these changes, test cases are
adjusted accordingly.

gdb:

2016-10-14  Yao Qi  <yao.qi@linaro.org>

	* arch-utils.c (convert_from_func_addr_identity): New function.
	* arch-utils.h (convert_from_func_addr_identity): Declare.
	* arm-tdep.c (arm_convert_from_func_addr): New function.
	(arm_convert_from_func_ptr_addr): New function.
	(arm_gdbarch_init): Install gdbarch hook convert_from_func_addr
	and convert_from_func_ptr_addr.
	* ax-gdb.c (gen_var_ref): Call gdbarch_convert_from_func_addr.
	* findvar.c (default_read_var_value): Likewise.
	* gdbarch.sh (convert_from_func_addr): New.
	* gdbarch.h, gdbarch.c: Re-generated.
	* infcall.c (find_function_addr): Call value_as_address
	instead of value_address.
	* ppc64-tdep.c (ppc64_convert_from_func_addr): New function.
	* ppc64-tdep.h (ppc64_convert_from_func_addr): Declare.
	* ppc-linux-tdep.c (ppc_linux_init_abi): Install
	convert_from_func_addr.
	* ppcfbsd-tdep.c: Likewise.
	* value.c (value_as_address): Call gdbarch_convert_from_func_ptr_addr.
---
 gdb/arch-utils.c     |  6 ++++++
 gdb/arch-utils.h     |  1 +
 gdb/arm-tdep.c       | 23 +++++++++++++++++++++++
 gdb/ax-gdb.c         | 12 +++++++++---
 gdb/findvar.c        |  5 +++++
 gdb/gdbarch.c        | 23 +++++++++++++++++++++++
 gdb/gdbarch.h        |  6 ++++++
 gdb/gdbarch.sh       |  4 ++++
 gdb/infcall.c        |  2 +-
 gdb/ppc-linux-tdep.c |  2 ++
 gdb/ppc64-tdep.c     | 30 ++++++++++++++++++++++++++++++
 gdb/ppc64-tdep.h     |  3 ++-
 gdb/ppcfbsd-tdep.c   |  2 ++
 gdb/value.c          |  4 +++-
 14 files changed, 117 insertions(+), 6 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 776dabc..b5ec892 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -168,6 +168,12 @@ convert_from_func_ptr_addr_identity (struct gdbarch *gdbarch, CORE_ADDR addr,
   return addr;
 }
 
+CORE_ADDR
+convert_from_func_addr_identity (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  return addr;
+}
+
 int
 no_op_reg_to_regnum (struct gdbarch *gdbarch, int reg)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index bbb0878..d86e2a7 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -63,6 +63,7 @@ extern int core_addr_greaterthan (CORE_ADDR lhs, CORE_ADDR rhs);
 
 extern CORE_ADDR core_addr_identity (struct gdbarch *gdbarch, CORE_ADDR addr);
 extern gdbarch_convert_from_func_ptr_addr_ftype convert_from_func_ptr_addr_identity;
+extern gdbarch_convert_from_func_addr_ftype convert_from_func_addr_identity;
 
 /* No-op conversion of reg to regnum.  */
 
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 27a3ebe..cf4a5e8 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -537,6 +537,26 @@ arm_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR val)
     return (val & 0x03fffffc);
 }
 
+/* Implement the convert_from_func_addr gdbarch method.  */
+
+static CORE_ADDR
+arm_convert_from_func_addr (struct gdbarch *gdbarch, CORE_ADDR val)
+{
+  if (arm_pc_is_thumb (gdbarch, val))
+    return MAKE_THUMB_ADDR (val);
+  else
+    return val;
+}
+
+/* Implement the convert_from_func_ptr_addr gdbarch method.  */
+
+static CORE_ADDR
+arm_convert_from_func_ptr_addr (struct gdbarch *gdbarch, CORE_ADDR addr,
+				struct target_ops *targ)
+{
+  return IS_THUMB_ADDR (addr) ? UNMAKE_THUMB_ADDR (addr) : addr;
+}
+
 /* Return 1 if PC is the start of a compiler helper function which
    can be safely ignored during prologue skipping.  IS_THUMB is true
    if the function is known to be a Thumb function due to the way it
@@ -9396,6 +9416,9 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   /* Address manipulation.  */
   set_gdbarch_addr_bits_remove (gdbarch, arm_addr_bits_remove);
+  set_gdbarch_convert_from_func_addr (gdbarch, arm_convert_from_func_addr);
+  set_gdbarch_convert_from_func_ptr_addr (gdbarch,
+					  arm_convert_from_func_ptr_addr);
 
   /* Advance PC across function entry code.  */
   set_gdbarch_skip_prologue (gdbarch, arm_skip_prologue);
diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 7c6cb64..a698741 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -689,10 +689,16 @@ gen_var_ref (struct gdbarch *gdbarch, struct agent_expr *ax,
       break;
 
     case LOC_BLOCK:
-      ax_const_l (ax, BLOCK_START (SYMBOL_BLOCK_VALUE (var)));
-      value->kind = axs_rvalue;
-      break;
+      {
+	CORE_ADDR addr = BLOCK_START (SYMBOL_BLOCK_VALUE (var));
 
+	/* Get address of function pointer rather than function
+	   address.  */
+	addr = gdbarch_convert_from_func_addr (gdbarch, addr);
+	ax_const_l (ax, addr);
+	value->kind = axs_rvalue;
+	break;
+      }
     case LOC_REGISTER:
       /* Don't generate any code at all; in the process of treating
          this as an lvalue or rvalue, the caller will generate the
diff --git a/gdb/findvar.c b/gdb/findvar.c
index 6e28a29..3b4bd15 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -698,6 +698,11 @@ default_read_var_value (struct symbol *var, const struct block *var_block,
 	   SYMBOL_OBJ_SECTION (symbol_objfile (var), var));
       else
 	addr = BLOCK_START (SYMBOL_BLOCK_VALUE (var));
+
+      /* Get address of function pointer rather than function
+	 address.  */
+      addr = gdbarch_convert_from_func_addr (get_type_arch (type),
+					     addr);
       break;
 
     case LOC_REGISTER:
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 4d8ef18..a213d9b 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -246,6 +246,7 @@ struct gdbarch
   gdbarch_stabs_argument_has_addr_ftype *stabs_argument_has_addr;
   int frame_red_zone_size;
   gdbarch_convert_from_func_ptr_addr_ftype *convert_from_func_ptr_addr;
+  gdbarch_convert_from_func_addr_ftype *convert_from_func_addr;
   gdbarch_addr_bits_remove_ftype *addr_bits_remove;
   gdbarch_software_single_step_ftype *software_single_step;
   gdbarch_single_step_through_delay_ftype *single_step_through_delay;
@@ -409,6 +410,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->remote_register_number = default_remote_register_number;
   gdbarch->stabs_argument_has_addr = default_stabs_argument_has_addr;
   gdbarch->convert_from_func_ptr_addr = convert_from_func_ptr_addr_identity;
+  gdbarch->convert_from_func_addr = convert_from_func_addr_identity;
   gdbarch->addr_bits_remove = core_addr_identity;
   gdbarch->skip_trampoline_code = generic_skip_trampoline_code;
   gdbarch->skip_solib_resolver = generic_skip_solib_resolver;
@@ -598,6 +600,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of frame_align, has predicate.  */
   /* Skip verify of stabs_argument_has_addr, invalid_p == 0 */
   /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */
+  /* Skip verify of convert_from_func_addr, invalid_p == 0 */
   /* Skip verify of addr_bits_remove, invalid_p == 0 */
   /* Skip verify of software_single_step, has predicate.  */
   /* Skip verify of single_step_through_delay, has predicate.  */
@@ -820,6 +823,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: coff_make_msymbol_special = <%s>\n",
                       host_address_to_string (gdbarch->coff_make_msymbol_special));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: convert_from_func_addr = <%s>\n",
+                      host_address_to_string (gdbarch->convert_from_func_addr));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: convert_from_func_ptr_addr = <%s>\n",
                       host_address_to_string (gdbarch->convert_from_func_ptr_addr));
   fprintf_unfiltered (file,
@@ -3096,6 +3102,23 @@ set_gdbarch_convert_from_func_ptr_addr (struct gdbarch *gdbarch,
 }
 
 CORE_ADDR
+gdbarch_convert_from_func_addr (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->convert_from_func_addr != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_convert_from_func_addr called\n");
+  return gdbarch->convert_from_func_addr (gdbarch, addr);
+}
+
+void
+set_gdbarch_convert_from_func_addr (struct gdbarch *gdbarch,
+                                    gdbarch_convert_from_func_addr_ftype convert_from_func_addr)
+{
+  gdbarch->convert_from_func_addr = convert_from_func_addr;
+}
+
+CORE_ADDR
 gdbarch_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR addr)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index cd01718..a93a603 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -642,6 +642,12 @@ typedef CORE_ADDR (gdbarch_convert_from_func_ptr_addr_ftype) (struct gdbarch *gd
 extern CORE_ADDR gdbarch_convert_from_func_ptr_addr (struct gdbarch *gdbarch, CORE_ADDR addr, struct target_ops *targ);
 extern void set_gdbarch_convert_from_func_ptr_addr (struct gdbarch *gdbarch, gdbarch_convert_from_func_ptr_addr_ftype *convert_from_func_ptr_addr);
 
+/* Convert function address to function pointer address. */
+
+typedef CORE_ADDR (gdbarch_convert_from_func_addr_ftype) (struct gdbarch *gdbarch, CORE_ADDR addr);
+extern CORE_ADDR gdbarch_convert_from_func_addr (struct gdbarch *gdbarch, CORE_ADDR addr);
+extern void set_gdbarch_convert_from_func_addr (struct gdbarch *gdbarch, gdbarch_convert_from_func_addr_ftype *convert_from_func_addr);
+
 /* On some machines there are bits in addresses which are not really
    part of the address, but are used by the kernel, the hardware, etc.
    for special purposes.  gdbarch_addr_bits_remove takes out any such bits so
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 1663156..57b541a 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -598,6 +598,10 @@ m:int:stabs_argument_has_addr:struct type *type:type::default_stabs_argument_has
 v:int:frame_red_zone_size
 #
 m:CORE_ADDR:convert_from_func_ptr_addr:CORE_ADDR addr, struct target_ops *targ:addr, targ::convert_from_func_ptr_addr_identity::0
+
+# Convert function address to function pointer address.
+m:CORE_ADDR:convert_from_func_addr:CORE_ADDR addr:addr::convert_from_func_addr_identity::0
+
 # On some machines there are bits in addresses which are not really
 # part of the address, but are used by the kernel, the hardware, etc.
 # for special purposes.  gdbarch_addr_bits_remove takes out any such bits so
diff --git a/gdb/infcall.c b/gdb/infcall.c
index ab7426d..718e479 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -260,7 +260,7 @@ find_function_addr (struct value *function, struct type **retval_type)
   /* Determine address to call.  */
   if (TYPE_CODE (ftype) == TYPE_CODE_FUNC
       || TYPE_CODE (ftype) == TYPE_CODE_METHOD)
-    funaddr = value_address (function);
+    funaddr = value_as_address (function);
   else if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
     {
       funaddr = value_as_address (function);
diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index ee158e3..4626ab6 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1744,6 +1744,8 @@ ppc_linux_init_abi (struct gdbarch_info info,
 	     function descriptors).  */
 	  set_gdbarch_convert_from_func_ptr_addr
 	    (gdbarch, ppc64_convert_from_func_ptr_addr);
+	  set_gdbarch_convert_from_func_addr
+	    (gdbarch, ppc64_convert_from_func_entry);
 
 	  set_gdbarch_elf_make_msymbol_special
 	    (gdbarch, ppc64_elf_make_msymbol_special);
diff --git a/gdb/ppc64-tdep.c b/gdb/ppc64-tdep.c
index b7357e3..6b2be83 100644
--- a/gdb/ppc64-tdep.c
+++ b/gdb/ppc64-tdep.c
@@ -24,6 +24,7 @@
 #include "ppc-tdep.h"
 #include "ppc64-tdep.h"
 #include "elf-bfd.h"
+#include "objfiles.h"
 
 /* Macros for matching instructions.  Note that, since all the
    operands are masked off before they're or-ed into the instruction,
@@ -615,6 +616,35 @@ ppc64_convert_from_func_ptr_addr (struct gdbarch *gdbarch,
   return addr;
 }
 
+/* Implement the convert_from_func_addr gdbarch method.  */
+
+CORE_ADDR
+ppc64_convert_from_func_addr (struct gdbarch *gdbarch, CORE_ADDR addr)
+{
+  struct symbol *sym = find_pc_function (addr);
+
+  if (sym != NULL)
+    {
+      struct bound_minimal_symbol msym;
+      struct obj_section *dot_fn_section;
+
+      dot_fn_section = find_pc_section (addr);
+      if (dot_fn_section == NULL || dot_fn_section->objfile == NULL)
+	return addr;
+
+      msym = lookup_minimal_symbol (SYMBOL_LINKAGE_NAME (sym), NULL,
+				    dot_fn_section->objfile);
+
+      /* That is the address of function descriptor.  */
+      if (msym.minsym != NULL)
+	return BMSYMBOL_VALUE_ADDRESS (msym);
+      else
+	return addr;
+    }
+  else
+    return addr;
+}
+
 /* A synthetic 'dot' symbols on ppc64 has the udata.p entry pointing
    back to the original ELF symbol it was derived from.  Get the size
    from that symbol.  */
diff --git a/gdb/ppc64-tdep.h b/gdb/ppc64-tdep.h
index 202ffca..b9f315b 100644
--- a/gdb/ppc64-tdep.h
+++ b/gdb/ppc64-tdep.h
@@ -30,7 +30,8 @@ extern CORE_ADDR ppc64_skip_trampoline_code (struct frame_info *frame,
 extern CORE_ADDR ppc64_convert_from_func_ptr_addr (struct gdbarch *gdbarch,
 						   CORE_ADDR addr,
 						   struct target_ops *targ);
-
+extern CORE_ADDR ppc64_convert_from_func_addr (struct gdbarch *gdbarch,
+					       CORE_ADDR addr);
 extern void ppc64_elf_make_msymbol_special (asymbol *,
 					    struct minimal_symbol *);
 #endif /* PPC64_TDEP_H  */
diff --git a/gdb/ppcfbsd-tdep.c b/gdb/ppcfbsd-tdep.c
index 10b41b0..3133a81 100644
--- a/gdb/ppcfbsd-tdep.c
+++ b/gdb/ppcfbsd-tdep.c
@@ -320,6 +320,8 @@ ppcfbsd_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
     {
       set_gdbarch_convert_from_func_ptr_addr
 	(gdbarch, ppc64_convert_from_func_ptr_addr);
+      set_gdbarch_convert_from_func_addr
+	(gdbarch, ppc64_convert_from_func_entry);
       set_gdbarch_elf_make_msymbol_special (gdbarch,
 					    ppc64_elf_make_msymbol_special);
 
diff --git a/gdb/value.c b/gdb/value.c
index b825aec..039789d 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -2853,7 +2853,9 @@ value_as_address (struct value *val)
      function, just return its address directly.  */
   if (TYPE_CODE (value_type (val)) == TYPE_CODE_FUNC
       || TYPE_CODE (value_type (val)) == TYPE_CODE_METHOD)
-    return value_address (val);
+    return gdbarch_convert_from_func_ptr_addr (gdbarch,
+					       value_address (val),
+					       &current_target);
 
   val = coerce_array (val);
 
-- 
1.9.1

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Simon Marchi]

On 2016-10-14 06:53, Yao Qi wrote:
> In this patch, I add a new gdbarch method convert_from_func_addr, which
> converts function address back to function descriptor address or
> function pointer address.  It is the reversed operation of
> convert_from_func_ptr_addr.  We convert function address to function
> descriptor address when,

I think these could be better named, byt saying what it converts from 
_and_ what it converts to.  With convert_from_func_addr, we know it 
takes as input a function address, but we don't know what it converts it 
to.  What about something like convert_func_address_to_descriptor (or 
convert_func_addr_to_desc if you prefer shorter names)?

I think that it would also be clearer if we always used the same 
terminology (address and descriptor seem good).  It is not very 
intuitive what is the difference between convert_from_func_ptr_addr and 
convert_from_func_addr.  "function pointer address" and "function 
address" sound too close to each other, so it's easy to confuse them...

> 3. User visible changes
> 
> This patch brings several user visible changes, which look more
> accurate, shown by this table below,
> 
>  COMMAND           BEFORE                       AFTER
>  p main            main function address        main function 
> descriptor
>                                                 address
>  disass main       disassembly function main    not changed
>  disass main+4,+4  disassembly 4 bytes from     disassembly 4 bytes 
> from
>                    function main address + 4    main's function 
> descriptor + 4
>  x/i main          show one instruction on      show one instruction on 
> main's
>                    function main address        function descriptor
> 
> Although the latter looks inconvenient, that is consistent to the
> meaning on C language level.  Due to these changes, test cases are
> adjusted accordingly.

Could you provide example of the actual output of those commands, before 
and after?  It is not clear to me what the difference will be.

Thanks,

Simon

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Yao Qi]

On Fri, Oct 14, 2016 at 6:35 PM, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> On 2016-10-14 06:53, Yao Qi wrote:
>>
>> In this patch, I add a new gdbarch method convert_from_func_addr, which
>> converts function address back to function descriptor address or
>> function pointer address.  It is the reversed operation of
>> convert_from_func_ptr_addr.  We convert function address to function
>> descriptor address when,
>
>
> I think these could be better named, byt saying what it converts from _and_
> what it converts to.  With convert_from_func_addr, we know it takes as input
> a function address, but we don't know what it converts it to.  What about
> something like convert_func_address_to_descriptor (or
> convert_func_addr_to_desc if you prefer shorter names)?
>
> I think that it would also be clearer if we always used the same terminology
> (address and descriptor seem good).  It is not very intuitive what is the
> difference between convert_from_func_ptr_addr and convert_from_func_addr.
> "function pointer address" and "function address" sound too close to each
> other, so it's easy to confuse them...
>

If we want to know what these two methods convert to, I'd like to use
"func_addr" and "func_ptr_addr".  These two methods can be named as
convert_func_addr_to_func_ptr_addr and
convert_func_ptr_addr_to_func_addr, but they are a little bit long.  Maybe,
remove "convert_" from the names?

>> 3. User visible changes
>>
>> This patch brings several user visible changes, which look more
>> accurate, shown by this table below,
>>
>>  COMMAND           BEFORE                       AFTER
>>  p main            main function address        main function descriptor
>>                                                 address
>>  disass main       disassembly function main    not changed
>>  disass main+4,+4  disassembly 4 bytes from     disassembly 4 bytes from
>>                    function main address + 4    main's function descriptor
>> + 4
>>  x/i main          show one instruction on      show one instruction on
>> main's
>>                    function main address        function descriptor
>>
>> Although the latter looks inconvenient, that is consistent to the
>> meaning on C language level.  Due to these changes, test cases are
>> adjusted accordingly.
>
>
> Could you provide example of the actual output of those commands, before and
> after?  It is not clear to me what the difference will be.
>

"p incr" shows the function descriptor address instead of function address.

old ppc64 gdb:
(gdb) p incr
$2 = {int (int)} 0x1000069c <incr>
(gdb) p &incr
$3 = (int (*)(int)) 0x1000069c <incr>

old arm gdb:
(gdb) p incr
$2 = {int (int)} 0x104fe <incr>
(gdb) p &incr
$3 = (int (*)(int)) 0x104fe <incr>

new ppc64 gdb:
(gdb) p incr
$2 = {int (int)} 0x10020090 <incr>
(gdb) p &incr
$3 = (int (*)(int)) @0x10020090: 0x1000069c <incr>

new arm gdb:
(gdb) p incr
$1 = {int (int)} 0x104ff <incr>
(gdb) p &incr
$2 = (int (*)(int)) @0x104ff: 0x104fe <incr>

"disassemble incr" is not changed,

(gdb) disassemble incr
Dump of assembler code for function incr:
   0x000000001000069c <+0>: mflr    r0
   0x00000000100006a0 <+4>: std     r0,16(r1)

"disassemble incr+4,+4" is changed, "incr" means function address
in old gdb, but it means function descriptor address in new gdb.

old gdb:
(gdb) disassemble incr+4,+4
Dump of assembler code from 0x100006a0 to 0x100006a4:
   0x00000000100006a0 <incr+4>: std     r0,16(r1)

new gdb:
(gdb) disassemble incr+4,+4
Dump of assembler code from 0x10020094 to 0x10020098:
   0x0000000010020094 <incr+4>: ps_madds0 f0,f0,f26,f0

... so "disassemble incr+4,+4" makes no sense in new gdb.  However,
you can use address instead.

Similarly, in old gdb, "x/i incr" is to show the first instruction at function
incr, but in new gdb, it shows the first instruction at function descriptor.

(gdb) x/i incr
   0x1000069c <incr>: mflr    r0
(gdb) x/i incr
   0x10020090 <incr>: .long 0x0

-- 
Yao (齐尧)

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Simon Marchi]

On 2016-10-17 07:40, Yao Qi wrote:
> On Fri, Oct 14, 2016 at 6:35 PM, Simon Marchi <simon.marchi@polymtl.ca> 
> wrote:
>> On 2016-10-14 06:53, Yao Qi wrote:
>>> 
>>> In this patch, I add a new gdbarch method convert_from_func_addr, 
>>> which
>>> converts function address back to function descriptor address or
>>> function pointer address.  It is the reversed operation of
>>> convert_from_func_ptr_addr.  We convert function address to function
>>> descriptor address when,
>> 
>> 
>> I think these could be better named, byt saying what it converts from 
>> _and_
>> what it converts to.  With convert_from_func_addr, we know it takes as 
>> input
>> a function address, but we don't know what it converts it to.  What 
>> about
>> something like convert_func_address_to_descriptor (or
>> convert_func_addr_to_desc if you prefer shorter names)?
>> 
>> I think that it would also be clearer if we always used the same 
>> terminology
>> (address and descriptor seem good).  It is not very intuitive what is 
>> the
>> difference between convert_from_func_ptr_addr and 
>> convert_from_func_addr.
>> "function pointer address" and "function address" sound too close to 
>> each
>> other, so it's easy to confuse them...
>> 
> 
> If we want to know what these two methods convert to, I'd like to use
> "func_addr" and "func_ptr_addr".  These two methods can be named as
> convert_func_addr_to_func_ptr_addr and
> convert_func_ptr_addr_to_func_addr, but they are a little bit long.  
> Maybe,
> remove "convert_" from the names?

As you wish, I think it's still clear if the "convert_" is omitted.

> "p incr" shows the function descriptor address instead of function 
> address.
> 
> old ppc64 gdb:
> (gdb) p incr
> $2 = {int (int)} 0x1000069c <incr>
> (gdb) p &incr
> $3 = (int (*)(int)) 0x1000069c <incr>
> 
> old arm gdb:
> (gdb) p incr
> $2 = {int (int)} 0x104fe <incr>
> (gdb) p &incr
> $3 = (int (*)(int)) 0x104fe <incr>
> 
> new ppc64 gdb:
> (gdb) p incr
> $2 = {int (int)} 0x10020090 <incr>
> (gdb) p &incr
> $3 = (int (*)(int)) @0x10020090: 0x1000069c <incr>
> 
> new arm gdb:
> (gdb) p incr
> $1 = {int (int)} 0x104ff <incr>
> (gdb) p &incr
> $2 = (int (*)(int)) @0x104ff: 0x104fe <incr>

I am not familiar with ppc64.  0x1000069c is the actual code, and 
0x10020090 is the descriptor address?  Or is it the other way?

> "disassemble incr" is not changed,
> 
> (gdb) disassemble incr
> Dump of assembler code for function incr:
>    0x000000001000069c <+0>: mflr    r0
>    0x00000000100006a0 <+4>: std     r0,16(r1)
> 
> "disassemble incr+4,+4" is changed, "incr" means function address
> in old gdb, but it means function descriptor address in new gdb.
> 
> old gdb:
> (gdb) disassemble incr+4,+4
> Dump of assembler code from 0x100006a0 to 0x100006a4:
>    0x00000000100006a0 <incr+4>: std     r0,16(r1)
> 
> new gdb:
> (gdb) disassemble incr+4,+4
> Dump of assembler code from 0x10020094 to 0x10020098:
>    0x0000000010020094 <incr+4>: ps_madds0 f0,f0,f26,f0
> 
> ... so "disassemble incr+4,+4" makes no sense in new gdb.  However,
> you can use address instead.
> 
> Similarly, in old gdb, "x/i incr" is to show the first instruction at 
> function
> incr, but in new gdb, it shows the first instruction at function 
> descriptor.
> 
> (gdb) x/i incr
>    0x1000069c <incr>: mflr    r0
> (gdb) x/i incr
>    0x10020090 <incr>: .long 0x0

Ok, so it is not the best UI-wise.  Is there a way we can add 
conversions from the descriptor address to the code address in the right 
circumstances, in order to make it more useful to the user?  For 
example, does it ever make sense to apply the operator + to the 
descriptor address, or could we always convert to code address for that? 
  Same for disassembly, it probably never makes sense to disassemble the 
descriptors, so could we add an explicit conversion there?

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Ulrich Weigand]

Yao Qi wrote:

> GCC vs GDB divergence on the meaning of "incr" brings the confusion.
> We should reduce such divergence as much as we can.  However, this
> divergence was added in https://sourceware.org/ml/gdb-patches/2001-11/msg00001.html
> I agree with Jim, but I'd like use function descriptor address in value,
> which makes the whole expression evaluation look more reasonable and
> more close to compiler's behavior.

I agree that this is a problem.  However, the reasons Jim mentions in
that long comment are still valid: it is in general *not possible* to
convert an arbitrary function address back to a function descriptor ...

> In this patch, I add a new gdbarch method convert_from_func_addr, which
> converts function address back to function descriptor address or
> function pointer address.  It is the reversed operation of
> convert_from_func_ptr_addr.

... and therefore this function cannot really be implemented on ppc64
in a fully-general way.  In particular, when running stripped binaries
or code that is otherwise without symbols (e.g. JITted code etc.), this
conversion may not be possible.

B.t.w. note that there is already a similar function that attempts this
conversion (ppc-sysv-tdep.c:convert_code_addr_to_desc_addr), but it is
currently only used during inferior function calls, so it is not so
critical if it fails.  (With your change, that function may actually
no longer be necessary at that place since values should already point
to function descriptors ...)

Note that this checks for presence of an msymbol at the code address,
while your code checks for presence of a symbol.  Maybe the best way
would be to check for either.

> We convert function address to function
> descriptor address when,
> 
>  - we create value for a function,
>  - we generate ax code for a function,

Since the conversion is problematic in general, I think the best way
would be to keep descriptor addresses wherever possible, and only
convert to code addresses at the last minute when necessary.

Your code already *mostly* does that, with the exception of symbol
addresses.  I'm wondering whether we shouldn't push the change one
step further back and already store descriptor addresses in
SYMBOL_VALUE_ADDRESS.  Note that this is currently already handled
somewhat inconsistenly on ppc64: msymbols already point to the
descriptor, and so do symbols read from stabs debug info; only
symbols read from DWARF debug info actually point to the code
address.

> I don't change the meaning of return value of value_as_address, because
> it is widely used, so value_as_address still return the function
> address if type is TYPE_CODE_FUNC or TYPE_CODE_METHOD.

I'm wondering whether this is a good idea; maybe it would be better to
return descriptor addresses and update those callers that actually need
code addresses.  This would keep the principle that we should keep
descriptor addresses as long as possible.  Also, this might actually
fix more bugs; if you look at e.g. this code in value_equal:

  else if (code1 == TYPE_CODE_PTR && is_int2)
    return value_as_address (arg1) == (CORE_ADDR) value_as_long (arg2);
  else if (code2 == TYPE_CODE_PTR && is_int1)
    return (CORE_ADDR) value_as_long (arg1) == value_as_address (arg2);

this might actually cause invalid evaluation of expressions like

  incr == 0x12345678

(as compared to the native C evaluation).


> This patch brings several user visible changes, which look more
> accurate, shown by this table below,
> 
>  COMMAND           BEFORE                       AFTER
>  p main            main function address        main function descriptor
>                                                 address
>  disass main       disassembly function main    not changed
>  disass main+4,+4  disassembly 4 bytes from     disassembly 4 bytes from
>                    function main address + 4    main's function descriptor + 4
>  x/i main          show one instruction on      show one instruction on main's
>                    function main address        function descriptor
> 
> Although the latter looks inconvenient, that is consistent to the
> meaning on C language level.  Due to these changes, test cases are
> adjusted accordingly.

Those are a bit annoying.  I think the underlying problem is that operations
like "disass" or "x/i" really don't work on "values" in the original sense
but rather on "PC addresses".  Maybe it would make sense to have those
function enable a special "mode" in the expression evaluator that would
change the conversion of functions to function pointers to use the code
address instead of the descriptor address?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Maciej W. Rozycki]

On Mon, 17 Oct 2016, Ulrich Weigand wrote:

> > This patch brings several user visible changes, which look more
> > accurate, shown by this table below,
> > 
> >  COMMAND           BEFORE                       AFTER
> >  p main            main function address        main function descriptor
> >                                                 address
> >  disass main       disassembly function main    not changed
> >  disass main+4,+4  disassembly 4 bytes from     disassembly 4 bytes from
> >                    function main address + 4    main's function descriptor + 4
> >  x/i main          show one instruction on      show one instruction on main's
> >                    function main address        function descriptor

 What about `info address main'?

> > Although the latter looks inconvenient, that is consistent to the
> > meaning on C language level.  Due to these changes, test cases are
> > adjusted accordingly.
> 
> Those are a bit annoying.  I think the underlying problem is that operations
> like "disass" or "x/i" really don't work on "values" in the original sense
> but rather on "PC addresses".  Maybe it would make sense to have those
> function enable a special "mode" in the expression evaluator that would
> change the conversion of functions to function pointers to use the code
> address instead of the descriptor address?

 Agreed.  I'd keep `disass main+4,+4' and `x/i main' (or `x/x main', etc., 
for that matter) as they are now and consistent with `disass main', if 
possible.  These would indeed have to be special as we don't actually want 
to see the ISA bit set in instruction addresses in disassembly either, as 
they are interpreted as memory data rather than execution addresses there.

 For descriptor access, which may undoubtedly be useful sometimes I'd 
suggest using `disass &main+4,+4' and `x/i &main', which would be 
consistent with the interpretation of function pointers elsewhere (of 
course `p main' and `p &main' would roughly be equivalent).  Thoughts?

 Overall I like the proposal and if this goes forward I will see if we can 
adapt the MIPS backend to use this approach as well, addressing the issues 
we still have remaining, such as confusing instruction addresses and wrong 
instruction data shown with `disass /r'.  We have a little complication in 
that we have the ISA bit set in line information, so that would have to be 
stripped in DWARF record processing, but it should be much easier to do 
with a single hook in place than the complicated processing now required 
to copy ISA bit annotation from the symbol table (msymbols), the hooks to 
handle which we'll then be able to drop from our DWARF machinery.

 Thanks, Yao; also for your persistence with addressing this general issue 
in response to my previous critical comments!

  Maciej

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Yao Qi]

On Tue, Oct 18, 2016 at 3:27 AM, Maciej W. Rozycki <macro@imgtec.com> wrote:
>
>  What about `info address main'?
>

It is unchanged.  It shows the function address of main,

(gdb) info address main
Symbol "main" is a function at address 0x1000073c.
(gdb) disassemble main
Dump of assembler code for function main:
   0x000000001000073c <+0>: mflr    r0

I'll respond to the rest of your mail later, after I got some clues/ideas for
both your and Ulrich's comments.

-- 
Yao (齐尧)

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Yao Qi]

On Tue, Oct 18, 2016 at 3:27 AM, Maciej W. Rozycki <macro@imgtec.com> wrote:
>
>  Agreed.  I'd keep `disass main+4,+4' and `x/i main' (or `x/x main', etc.,
> for that matter) as they are now and consistent with `disass main', if
> possible.  These would indeed have to be special as we don't actually want
> to see the ISA bit set in instruction addresses in disassembly either, as
> they are interpreted as memory data rather than execution addresses there.
>
>  For descriptor access, which may undoubtedly be useful sometimes I'd
> suggest using `disass &main+4,+4' and `x/i &main', which would be
> consistent with the interpretation of function pointers elsewhere (of
> course `p main' and `p &main' would roughly be equivalent).  Thoughts?

Right, "main" in "x/i main" means the function address of main; "&main"
means the pointer to function main.  However, I am not sure "p main"
and "p &main" is equivalent in terms of output.

>
>  Overall I like the proposal and if this goes forward I will see if we can
> adapt the MIPS backend to use this approach as well, addressing the issues
> we still have remaining, such as confusing instruction addresses and wrong
> instruction data shown with `disass /r'.  We have a little complication in
> that we have the ISA bit set in line information, so that would have to be
> stripped in DWARF record processing, but it should be much easier to do
> with a single hook in place than the complicated processing now required
> to copy ISA bit annotation from the symbol table (msymbols), the hooks to
> handle which we'll then be able to drop from our DWARF machinery.
>

At the very beginning, I wanted to follow the MIPS approach in ARM,
but I realized some issues when writing the patch.  Then, I switched to
the approach I am proposing in this thread.  If the ISA bit plus function
address is regarded as a function descriptor, this approach should be
able to handle all of them (ppc64, arm and mips) correctly (and
cleanly, I hope).

-- 
Yao (齐尧)

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Maciej W. Rozycki]

Hi Yao,

On Fri, 28 Oct 2016, Yao Qi wrote:

> >  Overall I like the proposal and if this goes forward I will see if we can
> > adapt the MIPS backend to use this approach as well, addressing the issues
> > we still have remaining, such as confusing instruction addresses and wrong
> > instruction data shown with `disass /r'.  We have a little complication in
> > that we have the ISA bit set in line information, so that would have to be
> > stripped in DWARF record processing, but it should be much easier to do
> > with a single hook in place than the complicated processing now required
> > to copy ISA bit annotation from the symbol table (msymbols), the hooks to
> > handle which we'll then be able to drop from our DWARF machinery.
> 
> At the very beginning, I wanted to follow the MIPS approach in ARM,
> but I realized some issues when writing the patch.  Then, I switched to
> the approach I am proposing in this thread.  If the ISA bit plus function
> address is regarded as a function descriptor, this approach should be
> able to handle all of them (ppc64, arm and mips) correctly (and
> cleanly, I hope).

 Have you made any progress with your solution?  I saw Ulrich had concerns 
and it's been a while, silent, since we discussed it.  Meanwhile we have 
started discovering corner case issues with compressed code disassembly, 
which I believe your change would fix.  So we'd rather avoid creating more 
hooks (hacks) addressing these issues, only to remove them again once your 
obviously more promising solution has gone in.

  Maciej

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Yao Qi]

On Tue, Oct 3, 2017 at 7:12 PM, Maciej W. Rozycki <macro@imgtec.com> wrote:
> Hi Yao,
>
> On Fri, 28 Oct 2016, Yao Qi wrote:
>
>> >  Overall I like the proposal and if this goes forward I will see if we can
>> > adapt the MIPS backend to use this approach as well, addressing the issues
>> > we still have remaining, such as confusing instruction addresses and wrong
>> > instruction data shown with `disass /r'.  We have a little complication in
>> > that we have the ISA bit set in line information, so that would have to be
>> > stripped in DWARF record processing, but it should be much easier to do
>> > with a single hook in place than the complicated processing now required
>> > to copy ISA bit annotation from the symbol table (msymbols), the hooks to
>> > handle which we'll then be able to drop from our DWARF machinery.
>>
>> At the very beginning, I wanted to follow the MIPS approach in ARM,
>> but I realized some issues when writing the patch.  Then, I switched to
>> the approach I am proposing in this thread.  If the ISA bit plus function
>> address is regarded as a function descriptor, this approach should be
>> able to handle all of them (ppc64, arm and mips) correctly (and
>> cleanly, I hope).
>
>  Have you made any progress with your solution?  I saw Ulrich had concerns
> and it's been a while, silent, since we discussed it.  Meanwhile we have
> started discovering corner case issues with compressed code disassembly,
> which I believe your change would fix.  So we'd rather avoid creating more
> hooks (hacks) addressing these issues, only to remove them again once your
> obviously more promising solution has gone in.
>

Hi Maciej,
I had some ideas to address Ulrich's concerns/comments, IIRC.  It is still
on my TODO, but it was interrupted by GDBserver software single step
last year, and SVE/target description this year :(  I still have 3 - 4
things I need
to finish by next release.  I hope that I can pick it up again next year.

-- 
Yao (齐尧)

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Yao Qi]

Hi Ulrich,
I did some experiments these days, for all your suggestions, and
comments.  I share the results now...

On Mon, Oct 17, 2016 at 4:51 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Yao Qi wrote:
>
>> GCC vs GDB divergence on the meaning of "incr" brings the confusion.
>> We should reduce such divergence as much as we can.  However, this
>> divergence was added in https://sourceware.org/ml/gdb-patches/2001-11/msg00001.html
>> I agree with Jim, but I'd like use function descriptor address in value,
>> which makes the whole expression evaluation look more reasonable and
>> more close to compiler's behavior.
>
> I agree that this is a problem.  However, the reasons Jim mentions in
> that long comment are still valid: it is in general *not possible* to
> convert an arbitrary function address back to a function descriptor ...
>
>> In this patch, I add a new gdbarch method convert_from_func_addr, which
>> converts function address back to function descriptor address or
>> function pointer address.  It is the reversed operation of
>> convert_from_func_ptr_addr.
>
> ... and therefore this function cannot really be implemented on ppc64
> in a fully-general way.  In particular, when running stripped binaries
> or code that is otherwise without symbols (e.g. JITted code etc.), this
> conversion may not be possible.

I don't expect convert_from_func_addr working in general, due to the
reasons you mentioned.  convert_from_func_addr is only used when
symbol (from debug information) is available.  If we want to GDB
handle such complex expression evaluation correctly, debug
information is required.

>
> B.t.w. note that there is already a similar function that attempts this
> conversion (ppc-sysv-tdep.c:convert_code_addr_to_desc_addr), but it is
> currently only used during inferior function calls, so it is not so
> critical if it fails.  (With your change, that function may actually
> no longer be necessary at that place since values should already point
> to function descriptors ...)
>

Yes, convert_from_func_addr is similar to convert_code_addr_to_desc_addr.
I removed convert_code_addr_to_desc_addr, but it breaks ifunc
inferior call in my experiment, because ifunc resolver gets the function
address rather than function descriptor address, we still need
convert_code_addr_to_desc_addr to get function descriptor address.

> Note that this checks for presence of an msymbol at the code address,
> while your code checks for presence of a symbol.  Maybe the best way
> would be to check for either.
>

Now, convert_from_func_addr does whatever
convert_code_addr_to_desc_addr does.

>> We convert function address to function
>> descriptor address when,
>>
>>  - we create value for a function,
>>  - we generate ax code for a function,
>
> Since the conversion is problematic in general, I think the best way
> would be to keep descriptor addresses wherever possible, and only
> convert to code addresses at the last minute when necessary.
>

Yes, I agree.

> Your code already *mostly* does that, with the exception of symbol
> addresses.  I'm wondering whether we shouldn't push the change one
> step further back and already store descriptor addresses in
> SYMBOL_VALUE_ADDRESS.  Note that this is currently already handled
> somewhat inconsistenly on ppc64: msymbols already point to the
> descriptor, and so do symbols read from stabs debug info; only
> symbols read from DWARF debug info actually point to the code
> address.

I tried what you suggested, but failed.  SYMBOL_VALUE_ADDRESS
is used to get variable address, rather than function's address.  We
get function address from block (BLOCK_START), and get block
from symbol (SYMBOL_BLOCK_VALUE).  There is no good way to
store function descriptor address in symbol.

>
>> I don't change the meaning of return value of value_as_address, because
>> it is widely used, so value_as_address still return the function
>> address if type is TYPE_CODE_FUNC or TYPE_CODE_METHOD.
>
> I'm wondering whether this is a good idea; maybe it would be better to
> return descriptor addresses and update those callers that actually need
> code addresses.  This would keep the principle that we should keep
> descriptor addresses as long as possible.  Also, this might actually
> fix more bugs; if you look at e.g. this code in value_equal:
>
>   else if (code1 == TYPE_CODE_PTR && is_int2)
>     return value_as_address (arg1) == (CORE_ADDR) value_as_long (arg2);
>   else if (code2 == TYPE_CODE_PTR && is_int1)
>     return (CORE_ADDR) value_as_long (arg1) == value_as_address (arg2);
>
> this might actually cause invalid evaluation of expressions like
>
>   incr == 0x12345678
>
> (as compared to the native C evaluation).
>

How about doing this in a followup patch as an enhancement?  My
priority is to get this RFC acceptable, and make PPC64/ARM/MIPS
works well.  Propagate descriptor address and bug fixes can be done
later.

>
>> This patch brings several user visible changes, which look more
>> accurate, shown by this table below,
>>
>>  COMMAND           BEFORE                       AFTER
>>  p main            main function address        main function descriptor
>>                                                 address
>>  disass main       disassembly function main    not changed
>>  disass main+4,+4  disassembly 4 bytes from     disassembly 4 bytes from
>>                    function main address + 4    main's function descriptor + 4
>>  x/i main          show one instruction on      show one instruction on main's
>>                    function main address        function descriptor
>>
>> Although the latter looks inconvenient, that is consistent to the
>> meaning on C language level.  Due to these changes, test cases are
>> adjusted accordingly.
>
> Those are a bit annoying.  I think the underlying problem is that operations
> like "disass" or "x/i" really don't work on "values" in the original sense
> but rather on "PC addresses".  Maybe it would make sense to have those
> function enable a special "mode" in the expression evaluator that would
> change the conversion of functions to function pointers to use the code
> address instead of the descriptor address?
>

I think about the special "mode" in the expression evaluation, but I
suspect that there is a case in a single expression, function address
is expected in some symbol, and descriptor address is expected in
some other symbol, like,

int func (int i) { return i;}

(gdb) x/i func + func (1)

the "func" is expected to be function address and the second "func"
is expected to be function descriptor address.  I had a heuristics that
VALUE means function address if we do a pointer add with long type
(valarith.c:value_ptradd).  It works fine.

I've already managed to get all my patches work.  If you agree with
my thoughts above, I'll post my patches next week.

-- 
Yao (齐尧)

Re: [RFC 2/3] Record function descriptor address instead of function address in value

[Ulrich Weigand]

Yao Qi wrote:
> On Mon, Oct 17, 2016 at 4:51 PM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > ... and therefore this function cannot really be implemented on ppc64
> > in a fully-general way.  In particular, when running stripped binaries
> > or code that is otherwise without symbols (e.g. JITted code etc.), this
> > conversion may not be possible.
> 
> I don't expect convert_from_func_addr working in general, due to the
> reasons you mentioned.  convert_from_func_addr is only used when
> symbol (from debug information) is available.  If we want to GDB
> handle such complex expression evaluation correctly, debug
> information is required.

OK, that makes sense.  If we have a symbol from debug information,
we probably should also have the msymbol.

> > B.t.w. note that there is already a similar function that attempts this
> > conversion (ppc-sysv-tdep.c:convert_code_addr_to_desc_addr), but it is
> > currently only used during inferior function calls, so it is not so
> > critical if it fails.  (With your change, that function may actually
> > no longer be necessary at that place since values should already point
> > to function descriptors ...)
> >
> 
> Yes, convert_from_func_addr is similar to convert_code_addr_to_desc_addr.
> I removed convert_code_addr_to_desc_addr, but it breaks ifunc
> inferior call in my experiment, because ifunc resolver gets the function
> address rather than function descriptor address, we still need
> convert_code_addr_to_desc_addr to get function descriptor address.

Huh?  The ifunc resolver is supposed to return a function pointer,
which will be a descriptor address on ppc64.  Aha, it seems the
problem is in GDB's implementaton of elf_gnu_ifunc_resolve_addr,
which calls gdbarch_convert_from_func_ptr_addr -- it really
shouldn't do this, we should keep descriptors as long as possible.

> > Your code already *mostly* does that, with the exception of symbol
> > addresses.  I'm wondering whether we shouldn't push the change one
> > step further back and already store descriptor addresses in
> > SYMBOL_VALUE_ADDRESS.  Note that this is currently already handled
> > somewhat inconsistenly on ppc64: msymbols already point to the
> > descriptor, and so do symbols read from stabs debug info; only
> > symbols read from DWARF debug info actually point to the code
> > address.
> 
> I tried what you suggested, but failed.  SYMBOL_VALUE_ADDRESS
> is used to get variable address, rather than function's address.  We
> get function address from block (BLOCK_START), and get block
> from symbol (SYMBOL_BLOCK_VALUE).  There is no good way to
> store function descriptor address in symbol.

OK, I see.  Doing the conversion when looking up the symbol address
should be fine, then.

> How about doing this in a followup patch as an enhancement?  My
> priority is to get this RFC acceptable, and make PPC64/ARM/MIPS
> works well.  Propagate descriptor address and bug fixes can be done
> later.

Sure.

> > Those are a bit annoying.  I think the underlying problem is that operati=
> ons
> > like "disass" or "x/i" really don't work on "values" in the original sense
> > but rather on "PC addresses".  Maybe it would make sense to have those
> > function enable a special "mode" in the expression evaluator that would
> > change the conversion of functions to function pointers to use the code
> > address instead of the descriptor address?
> >
> 
> I think about the special "mode" in the expression evaluation, but I
> suspect that there is a case in a single expression, function address
> is expected in some symbol, and descriptor address is expected in
> some other symbol, like,
> 
> int func (int i) { return i;}
> 
> (gdb) x/i func + func (1)
> 
> the "func" is expected to be function address and the second "func"
> is expected to be function descriptor address.  I had a heuristics that
> VALUE means function address if we do a pointer add with long type
> (valarith.c:value_ptradd).  It works fine.

I see.  This is a bit odd in that evaluating "main" would then return
a descriptor while "main + 1" would return a code address, which
means the caller is never really sure which one it gets.

I think the problem you describe above could still be made to work
with the special evalution mode, you'd just have to reset the mode
to the default when recursing into certain expression nodes, like
e.g. when evaluating the function operand of a OP_FUNCALL node.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com