This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH PR gdb/22736] [aarch64] gdb crashes on a conditional breakpoint with cast return type



> On 5 Mar 2018, at 16:45, Pedro Alves <palves@redhat.com> wrote:
> 
> On 03/05/2018 03:57 PM, Alan Hayward wrote:
>> 
>> 
>>> On 2 Mar 2018, at 15:18, Joel Brobecker <brobecker@adacore.com> wrote:
>>> 
>>>>>> The cast to (int) is causing this - remove the cast and it finds the type.
>>>>>> I’m assuming that’s causing it to drop the debug info.
>>>>> 
>>>>> It sounds like a bug to me.  If this bug "TYPE_TARGET_TYPE
>>>>> (func_type) becomes NULL caused by cast" is fixed, the GDB segfault
>>>>> will go away accordingly.
>>>>> 
>>>>> To be clear, your patch here is fine to me.  My suggestion is that
>>>>> we'd better dig it deeper.
>>>> 
>>>> Agreed. I’ll raise a new bug, and have a look into it too whilst I’m
>>>> in the area.
>>> 
>>> Having read what Yao said, and looking at the example you gave,
>>> I'm now thinking that one could very well be the cause of the other;
>>> it seems like the cast might indeed be returning a value with
>>> a struct type that's missing the return type. Even a subprogram
>>> which returns nothing has a TYPE_TARGET_TYPE set to a TYPE_CODE_VOID,
>>> right?
>> 
>> Been debugging this a little more (and learnt quite a few things about gdb along the way!)
>> Not sure if this reply is the best place to discuss, or if it should go onto the bug.
>> But….
>> 
>> On x86, for strcmp the type and it's target types, as received in amd64_push_dummy_call are:
>> TYPE_CODE_FUNC -> TYPE_CODE_INT -> 0x0
>> Whereas on aarch64 in aarch64_push_dummy_call we get:
>> TYPE_CODE_FUNC -> 0x0
>> 
>> It turns out this occurs because strcmp has IFUNC vs FUNC differences:
>> 
>> X86:
>> $ readelf -s /lib/x86_64-linux-gnu/libc-2.19.so | grep strcmp
>>  2085: 0000000000087380    60 IFUNC   GLOBAL DEFAULT   12 strcmp@@GLIBC_2.2.5
>> 
>> Aarch64:
>> $ readelf -s /lib/aarch64-linux-gnu/libc-2.23.so | grep strcmp
>>  2043: 0000000000076380   164 FUNC    GLOBAL DEFAULT   12 strcmp@@GLIBC_2.17
>> 
>> 
>> I decided to test this on X86 using a FUNC….
>> 
>> $ readelf -s /lib/x86_64-linux-gnu/libc-2.19.so | grep strlen
>>   769: 0000000000088dd0   436 FUNC    GLOBAL DEFAULT   12 strlen@@GLIBC_2.2.5
>> 
>> Changed my test to do:
>> (gdb) b foo if (int)strlen(name) == 3
>> 
>> ...Now the type received by the target code is TYPE_CODE_FUNC -> 0, the same as aarch64.
>> 
>> However, x86 does not segfault. This is because amd64_push_dummy_call doesn’t bother to
>> check the function parameter.
>> 
>> 
>> Looking into the code, when reading a FUNC, readlf.c : elf_symtab_read() hits:
>> 
>> 		  if (sym->flags & BSF_GNU_INDIRECT_FUNCTION)
>> 		    ms_type = mst_text_gnu_ifunc;
>> 		  else
>> 		    ms_type = mst_text;
>> 
>> Setting the type to mst_text, which eventually causes find_minsym_type_and_address() to
>> set the type to objfile_type (objfile)->nodebug_text_symbol
>> Whereas an IFUNC gets set to objfile_type (objfile)->nodebug_text_gnu_ifunc_symbol.
>> 
>> So, I think probably either:
>> * Everything is correct
>> * mst_text needs handling differently
>> * FUNCs need a new minimal symbol type (mst_text_gnu_func?)
>> (I’m not familiar enough with this part of the code to give a definitive answer).
> 
> Funny enough, I started working on fixing ifunc-related problems early
> last week, but then got sidetracked into other high priority things. I've not
> run into a GDB crash, but just in case, maybe this branch helps:
> 
> https://github.com/palves/gdb/commits/palves/ifunc
> 
> The gnu-ifunc.exp test doesn't pass cleanly on that branch
> because I'm midway through augmenting that testcase to cover
> different scenarios and evaluating whether it's the test that
> needs fixing, or gdb.
> 

Thanks for the branch, gave that it a try, but it has the same error.

Digging a little more into this….

The target type for the *function is set to the type of the function pointer, not the return type.
So, for IFUNC, TYPE_CODE_FUNC target type is TYPE_CODE_INT which gives the type
of the function pointer. For the FUNC, there is no pointer, so the target type of TYPE_CODE_FUNC
is set to null. That makes sense to me, but isn’t immediately obvious.

In call_function_by_hand_dummy() there already exists code to extract the type of the function
return, which is set as target_values_type. Patch below simply passes this through to the
gdbarch_push_dummy_call  method. Updated aarch64 and arm targets to use this.

If this patch is ok for amd64/aarch64/arm, then I will post again with _push_dummy_call()
fixed on every target. (Not done that yet because it’s a lot of tedious copy pasting). Most targets
don’t even look at the return types, so I’m not expecting many issues when I look at them.

Tested on aarch64 only with make check unitest and native_gdbserver.


2018-03-06  Alan Hayward  <alan.hayward@arm.com>

	PR gdb/22736
	* aarch64-tdep.c (aarch64_push_dummy_call): Use passed in return type.
	* amd64-tdep.c (amd64_push_dummy_call): Add new arg.
	* arm-tdep.c (arm_push_dummy_call): Use passed in return type.
	* gdbarch.sh (gdbarch_push_dummy_call): Add return type.
	* gdbarch.h: Regenerate.
	* gdbarch.sh: Likewise.
	* infcall.c (call_function_by_hand_dummy): Pass down return type.


diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index f08945ea07101e1cd7906ca640c023ac7d189dd9..bf9ae4475f80a8200eeb56a8edf4a9f7ee3daa37 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1376,13 +1376,12 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 			 struct regcache *regcache, CORE_ADDR bp_addr,
 			 int nargs,
 			 struct value **args, CORE_ADDR sp, int struct_return,
-			 CORE_ADDR struct_addr)
+			 CORE_ADDR struct_addr, struct type *return_type)
 {
   int argnum;
   struct aarch64_call_info info;
   struct type *func_type;
-  struct type *return_type;
-  int lang_struct_return;
+  int lang_struct_return = 0;

   memset (&info, 0, sizeof (info));

@@ -1411,20 +1410,11 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
      Rather that change the target interface we call the language code
      directly ourselves.  */

-  func_type = check_typedef (value_type (function));
-
-  /* Dereference function pointer types.  */
-  if (TYPE_CODE (func_type) == TYPE_CODE_PTR)
-    func_type = TYPE_TARGET_TYPE (func_type);
-
-  gdb_assert (TYPE_CODE (func_type) == TYPE_CODE_FUNC
-	      || TYPE_CODE (func_type) == TYPE_CODE_METHOD);
-
   /* If language_pass_by_reference () returned true we will have been
      given an additional initial argument, a hidden pointer to the
      return slot in memory.  */
-  return_type = TYPE_TARGET_TYPE (func_type);
-  lang_struct_return = language_pass_by_reference (return_type);
+  if (return_type != nullptr)
+    lang_struct_return = language_pass_by_reference (return_type);

   /* Set the return address.  For the AArch64, the return breakpoint
      is always at BP_ADDR.  */
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 6b92c9244c627af5fea78fdfd97b41a887fb679a..c50f611846c32364f31c33e0b4092af881fa6248 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -990,7 +990,8 @@ static CORE_ADDR
 amd64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		       struct regcache *regcache, CORE_ADDR bp_addr,
 		       int nargs, struct value **args,	CORE_ADDR sp,
-		       int struct_return, CORE_ADDR struct_addr)
+		       int struct_return, CORE_ADDR struct_addr,
+		       struct type *return_type)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   gdb_byte buf[8];
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index ef7e66b36afe34aed3880b86d16b466984481131..78240d21ef1082bfdaaafa6be6c2f5a73ac819a6 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3689,7 +3689,7 @@ static CORE_ADDR
 arm_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		     struct regcache *regcache, CORE_ADDR bp_addr, int nargs,
 		     struct value **args, CORE_ADDR sp, int struct_return,
-		     CORE_ADDR struct_addr)
+		     CORE_ADDR struct_addr, struct type *return_type)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int argnum;
@@ -3700,12 +3700,8 @@ arm_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   struct type *ftype;
   unsigned vfp_regs_free = (1 << 16) - 1;

-  /* Determine the type of this function and whether the VFP ABI
-     applies.  */
-  ftype = check_typedef (value_type (function));
-  if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
-    ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
-  use_vfp_abi = arm_vfp_abi_for_function (gdbarch, ftype);
+  /* Determine whether the VFP ABI applies.  */
+  use_vfp_abi = arm_vfp_abi_for_function (gdbarch, return_type);

   /* Set the return address.  For the ARM, the return breakpoint is
      always at BP_ADDR.  */
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 5cb131de1d27209107b5b83773ea7560ef0da6ac..086a230ffe6d295e1d1d1c3b5e3391ec979444eb 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -397,8 +397,8 @@ extern void set_gdbarch_deprecated_fp_regnum (struct gdbarch *gdbarch, int depre

 extern int gdbarch_push_dummy_call_p (struct gdbarch *gdbarch);

-typedef CORE_ADDR (gdbarch_push_dummy_call_ftype) (struct gdbarch *gdbarch, struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr);
-extern CORE_ADDR gdbarch_push_dummy_call (struct gdbarch *gdbarch, struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr);
+typedef CORE_ADDR (gdbarch_push_dummy_call_ftype) (struct gdbarch *gdbarch, struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr, struct type *return_type);
+extern CORE_ADDR gdbarch_push_dummy_call (struct gdbarch *gdbarch, struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr, struct type *return_type);
 extern void set_gdbarch_push_dummy_call (struct gdbarch *gdbarch, gdbarch_push_dummy_call_ftype *push_dummy_call);

 extern int gdbarch_call_dummy_location (struct gdbarch *gdbarch);
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index b8703e5a556e310e023cadf57037918d1bdd2850..50dd72db5d5a06a841391290c1afac345af49eb9 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -2368,13 +2368,13 @@ gdbarch_push_dummy_call_p (struct gdbarch *gdbarch)
 }

 CORE_ADDR
-gdbarch_push_dummy_call (struct gdbarch *gdbarch, struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr)
+gdbarch_push_dummy_call (struct gdbarch *gdbarch, struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr, struct type *return_type)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->push_dummy_call != NULL);
   if (gdbarch_debug >= 2)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_push_dummy_call called\n");
-  return gdbarch->push_dummy_call (gdbarch, function, regcache, bp_addr, nargs, args, sp, struct_return, struct_addr);
+  return gdbarch->push_dummy_call (gdbarch, function, regcache, bp_addr, nargs, args, sp, struct_return, struct_addr, return_type);
 }

 void
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 33dfa6b349dee778f1a82a511a6cf57960d48f89..a1472069aa3d3a3bd355a772a4f9527f413330ca 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -488,7 +488,7 @@ M;struct frame_id;dummy_id;struct frame_info *this_frame;this_frame
 # deprecated_fp_regnum.
 v;int;deprecated_fp_regnum;;;-1;-1;;0

-M;CORE_ADDR;push_dummy_call;struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr;function, regcache, bp_addr, nargs, args, sp, struct_return, struct_addr
+M;CORE_ADDR;push_dummy_call;struct value *function, struct regcache *regcache, CORE_ADDR bp_addr, int nargs, struct value **args, CORE_ADDR sp, int struct_return, CORE_ADDR struct_addr, struct type *return_type;function, regcache, bp_addr, nargs, args, sp, struct_return, struct_addr, return_type
 v;int;call_dummy_location;;;;AT_ENTRY_POINT;;0
 M;CORE_ADDR;push_dummy_code;CORE_ADDR sp, CORE_ADDR funaddr, struct value **args, int nargs, struct type *value_type, CORE_ADDR *real_pc, CORE_ADDR *bp_addr, struct regcache *regcache;sp, funaddr, args, nargs, value_type, real_pc, bp_addr, regcache

diff --git a/gdb/infcall.c b/gdb/infcall.c
index b7f4a176db581c15c4fdd8c5299aab35d0fe4a68..773f43e884b3d831e0bd8254998cbb7ecb8daf33 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -1070,7 +1070,8 @@ call_function_by_hand_dummy (struct value *function,
      return address should be pointed.  */
   sp = gdbarch_push_dummy_call (gdbarch, function, get_current_regcache (),
 				bp_addr, nargs, args,
-				sp, struct_return, struct_addr);
+				sp, struct_return, struct_addr,
+				target_values_type);

   /* Set up a frame ID for the dummy frame so we can pass it to
      set_momentary_breakpoint.  We need to give the breakpoint a frame





Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]