[patchv2] compile: Fix GNU-IFUNC funcs called from injected code

Message ID 20150224200237.GA1746@host1.jankratochvil.net
State New, archived
Headers

Commit Message

Jan Kratochvil Feb. 24, 2015, 8:02 p.m. UTC
  Testcase cosmetic update.
gdb/ChangeLog
2015-02-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* compile/compile-c-symbols.c (convert_one_symbol, convert_symbol_bmsym)
	(gcc_symbol_address): Call gnu_ifunc_resolve_addr.

gdb/testsuite/ChangeLog
2015-02-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.compile/compile-ifunc.c: New file.
	* gdb.compile/compile-ifunc.exp: New file.
  

Comments

Phil Muldoon Feb. 25, 2015, 8:20 a.m. UTC | #1
On 24/02/15 20:02, Jan Kratochvil wrote:
> Testcase cosmetic update.
>
> compile-gnu-ifunc2.patch
>
> gdb/ChangeLog
> 2015-02-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>     * compile/compile-c-symbols.c (convert_one_symbol, convert_symbol_bmsym)
>     (gcc_symbol_address): Call gnu_ifunc_resolve_addr.
>
> gdb/testsuite/ChangeLog
> 2015-02-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>     * gdb.compile/compile-ifunc.c: New file.
>     * gdb.compile/compile-ifunc.exp: New file.

Thanks. This looks great other than the following nit:


>      case mst_text_gnu_ifunc:
> -      type = objfile_type (objfile)->nodebug_text_gnu_ifunc_symbol;
> +      // nodebug_text_gnu_ifunc_symbol would cause:
> +      // function return type cannot be function

Not sure if we allow // comments yet.


> +      if (TYPE_GNU_IFUNC (SYMBOL_TYPE (sym)))
> +        result = gnu_ifunc_resolve_addr (target_gdbarch (), result);

Is this guaranteed to return a value to result (or an exception)?
   
Cheers

Phil
  
Jan Kratochvil Feb. 25, 2015, 5:29 p.m. UTC | #2
On Wed, 25 Feb 2015 09:20:28 +0100, Phil Muldoon wrote:
> Not sure if we allow // comments yet.

Given the codebase already contains them and additionally it should get
converted to C++ I assume they are OK.


> > +      if (TYPE_GNU_IFUNC (SYMBOL_TYPE (sym)))
> > +        result = gnu_ifunc_resolve_addr (target_gdbarch (), result);
> 
> Is this guaranteed to return a value to result (or an exception)?

Yes.


Jan
  
Joel Brobecker Feb. 26, 2015, 8:18 a.m. UTC | #3
> On Wed, 25 Feb 2015 09:20:28 +0100, Phil Muldoon wrote:
> > Not sure if we allow // comments yet.
> 
> Given the codebase already contains them and additionally it should get
> converted to C++ I assume they are OK.

Let's please hold them off until the project is officially a C++
project. At the moment, we support the building of GDB using
a pure C compiler which may not understand those.

If other areas use them already, I consider that a bug.
  
Jan Kratochvil Feb. 26, 2015, 8:30 a.m. UTC | #4
On Thu, 26 Feb 2015 09:18:00 +0100, Joel Brobecker wrote:
> Let's please hold them off until the project is officially a C++
> project.

I will change that upon check-in.


> At the moment, we support the building of GDB using
> a pure C compiler which may not understand those.
+
> If other areas use them already, I consider that a bug.

Non-// compiler would not work anyway, last time such a patch check-in was:
	commit bb2ec1b34e7cc8d8602512235016e74b800dac3b
	Author: Tom Tromey <tromey@redhat.com>
	Date:   Wed May 14 14:35:45 2014 -0600
	    the "compile" command


Jan
  
Joel Brobecker Feb. 26, 2015, 9:34 a.m. UTC | #5
> > If other areas use them already, I consider that a bug.
> 
> Non-// compiler would not work anyway, last time such a patch check-in was:
> 	commit bb2ec1b34e7cc8d8602512235016e74b800dac3b
> 	Author: Tom Tromey <tromey@redhat.com>
> 	Date:   Wed May 14 14:35:45 2014 -0600
> 	    the "compile" command

As one of the authors of this patch, can you fix those, please?
(obvious fix, master and gdb-7.9-branch - remember that we need
a PR for fixes pushed to the branch)

I checked the sources, and outside of the checkin above, only
iq2000-tdep.c appears to have this issue at one location. I will
fix that one.

Thanks,
  
Pedro Alves Feb. 26, 2015, 3:49 p.m. UTC | #6
On 02/24/2015 08:02 PM, Jan Kratochvil wrote:
> 
> gdb/ChangeLog
> 2015-02-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* compile/compile-c-symbols.c (convert_one_symbol, convert_symbol_bmsym)
> 	(gcc_symbol_address): Call gnu_ifunc_resolve_addr.

Looks good to me too.  OK.

I think we could probably avoid the extra infcall done
within gnu_ifunc_resolve_addr if we're going to be running
the generated code anyway, but it's not worth the effort
at this point.

> +if { [prepare_for_testing ${testfile}.exp "$testfile-debug" $srcfile] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +

Could you move these inside the prefix too, so that if they
fail for some reason, we get distinct test messages from
the nodebug case?

> +with_test_prefix "debug" {

Thanks,
Pedro Alves
  
Jan Kratochvil Feb. 26, 2015, 4:43 p.m. UTC | #7
On Thu, 26 Feb 2015 16:49:48 +0100, Pedro Alves wrote:
> I think we could probably avoid the extra infcall done
> within gnu_ifunc_resolve_addr if we're going to be running
> the generated code anyway, but it's not worth the effort
> at this point.

Yes; it would also need hacking in the GCC part.


> Could you move these inside the prefix too, so that if they
> fail for some reason, we get distinct test messages from
> the nodebug case?

Done.


Checked in:
	081a1c2cede38dfb837e3d89539416fd836be4fe


Thanks,
Jan
  

Patch

diff --git a/gdb/compile/compile-c-symbols.c b/gdb/compile/compile-c-symbols.c
index 6562f05..ba6a229 100644
--- a/gdb/compile/compile-c-symbols.c
+++ b/gdb/compile/compile-c-symbols.c
@@ -187,6 +187,8 @@  convert_one_symbol (struct compile_c_instance *context,
 	case LOC_BLOCK:
 	  kind = GCC_C_SYMBOL_FUNCTION;
 	  addr = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+	  if (is_global && TYPE_GNU_IFUNC (SYMBOL_TYPE (sym)))
+	    addr = gnu_ifunc_resolve_addr (target_gdbarch (), addr);
 	  break;
 
 	case LOC_CONST:
@@ -365,6 +367,8 @@  convert_symbol_bmsym (struct compile_c_instance *context,
   gcc_decl decl;
   CORE_ADDR addr;
 
+  addr = MSYMBOL_VALUE_ADDRESS (objfile, msym);
+
   /* Conversion copied from write_exp_msymbol.  */
   switch (MSYMBOL_TYPE (msym))
     {
@@ -376,8 +380,11 @@  convert_symbol_bmsym (struct compile_c_instance *context,
       break;
 
     case mst_text_gnu_ifunc:
-      type = objfile_type (objfile)->nodebug_text_gnu_ifunc_symbol;
+      // nodebug_text_gnu_ifunc_symbol would cause:
+      // function return type cannot be function
+      type = objfile_type (objfile)->nodebug_text_symbol;
       kind = GCC_C_SYMBOL_FUNCTION;
+      addr = gnu_ifunc_resolve_addr (target_gdbarch (), addr);
       break;
 
     case mst_data:
@@ -400,7 +407,6 @@  convert_symbol_bmsym (struct compile_c_instance *context,
     }
 
   sym_type = convert_type (context, type);
-  addr = MSYMBOL_VALUE_ADDRESS (objfile, msym);
   decl = C_CTX (context)->c_ops->build_decl (C_CTX (context),
 					     MSYMBOL_NATURAL_NAME (msym),
 					     kind, sym_type, NULL, addr,
@@ -497,6 +503,8 @@  gcc_symbol_address (void *datum, struct gcc_c_context *gcc_context,
 				"gcc_symbol_address \"%s\": full symbol\n",
 				identifier);
 	  result = BLOCK_START (SYMBOL_BLOCK_VALUE (sym));
+	  if (TYPE_GNU_IFUNC (SYMBOL_TYPE (sym)))
+	    result = gnu_ifunc_resolve_addr (target_gdbarch (), result);
 	  found = 1;
 	}
       else
@@ -512,6 +520,8 @@  gcc_symbol_address (void *datum, struct gcc_c_context *gcc_context,
 				    "symbol\n",
 				    identifier);
 	      result = BMSYMBOL_VALUE_ADDRESS (msym);
+	      if (MSYMBOL_TYPE (msym.minsym) == mst_text_gnu_ifunc)
+		result = gnu_ifunc_resolve_addr (target_gdbarch (), result);
 	      found = 1;
 	    }
 	}
diff --git a/gdb/testsuite/gdb.compile/compile-ifunc.c b/gdb/testsuite/gdb.compile/compile-ifunc.c
new file mode 100644
index 0000000..e5cee77
--- /dev/null
+++ b/gdb/testsuite/gdb.compile/compile-ifunc.c
@@ -0,0 +1,46 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+typedef int (*final_t) (int arg);
+
+int
+final (int arg)
+{
+  return arg + 1;
+}
+
+asm (".type gnu_ifunc, %gnu_indirect_function");
+
+final_t
+gnu_ifunc (void)
+{
+  return final;
+}
+
+extern int gnu_ifunc_alias (int arg) __attribute__ ((alias ("gnu_ifunc")));
+
+static int resultvar;
+
+int
+main (void)
+{
+  if (gnu_ifunc_alias (10) != 11)
+    abort ();
+  return resultvar;
+}
diff --git a/gdb/testsuite/gdb.compile/compile-ifunc.exp b/gdb/testsuite/gdb.compile/compile-ifunc.exp
new file mode 100644
index 0000000..9e83c46
--- /dev/null
+++ b/gdb/testsuite/gdb.compile/compile-ifunc.exp
@@ -0,0 +1,54 @@ 
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if { [prepare_for_testing ${testfile}.exp "$testfile-nodebug" $srcfile {}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+if {[skip_compile_feature_tests]} {
+    untested "compile command not supported (could not find libcc1 shared library?)"
+    return -1
+}
+
+with_test_prefix "nodebug" {
+
+    gdb_test_no_output "compile code resultvar = gnu_ifunc (10);"
+
+    gdb_test "p resultvar" " = 11"
+
+}
+
+if { [prepare_for_testing ${testfile}.exp "$testfile-debug" $srcfile] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+with_test_prefix "debug" {
+
+    # gnu_ifunc (10): error: too many arguments to function 'gnu_ifunc'
+    gdb_test_no_output "compile code resultvar = gnu_ifunc_alias (10);"
+
+    gdb_test "p resultvar" " = 11"
+
+}