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

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

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 26 bytes --]

Testcase cosmetic update.

[-- Attachment #2: compile-gnu-ifunc2.patch --]
[-- Type: text/plain, Size: 5770 bytes --]

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.

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"
+
+}

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

[Phil Muldoon]

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

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

[Jan Kratochvil]

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

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

[Joel Brobecker]

> 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.

-- 
Joel

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

[Jan Kratochvil]

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

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

[Joel Brobecker]

> > 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,
-- 
Joel

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

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

On Thu, 26 Feb 2015 10:34:03 +0100, Joel Brobecker wrote:
> 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)

Done, also in the 7.9 branch.

Used your PR, only now I read I should have probably used a new one.


Jan

[-- Attachment #2: Type: message/rfc822, Size: 1422 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH 1/2] Change // comment in gdb/compile/
Date: Thu, 26 Feb 2015 11:48:18 +0100

---
 gdb/compile/compile-c-support.c   | 2 +-
 gdb/compile/compile-object-load.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/compile/compile-c-support.c b/gdb/compile/compile-c-support.c
index 8f1f45d..1711cda 100644
--- a/gdb/compile/compile-c-support.c
+++ b/gdb/compile/compile-c-support.c
@@ -351,7 +351,7 @@ c_compute_program (struct compile_instance *inst,
 			" __gdb_intptr;\n",
 			buf);
 
-      // Iterate all log2 sizes in bytes supported by c_get_mode_for_size.
+      /* Iterate all log2 sizes in bytes supported by c_get_mode_for_size.  */
       for (i = 0; i < 4; ++i)
 	{
 	  const char *mode = c_get_mode_for_size (1 << i);
diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 5903f18..e187970 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -67,7 +67,7 @@ setup_sections (bfd *abfd, asection *sect, void *data_voidp)
       if ((bfd_get_section_flags (abfd, sect) & SEC_ALLOC) == 0)
 	return;
 
-      // Make the memory always readable.
+      /* Make the memory always readable.  */
       prot = GDB_MMAP_PROT_READ;
       if ((bfd_get_section_flags (abfd, sect) & SEC_READONLY) == 0)
 	prot |= GDB_MMAP_PROT_WRITE;
-- 
2.1.0

[-- Attachment #3: Type: message/rfc822, Size: 973 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH 2/2] Change // comment in gdb/compile/
Date: Thu, 26 Feb 2015 11:50:08 +0100

Missing ChangeLog in the previous commit:
	bb2b33b9395662e7562be34b47b9533620b583c6

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

	PR build/18033
	* compile/compile-c-support.c (c_compute_program): Change // comment.
	* compile/compile-object-load.c (setup_sections): Change // comment.
---
 gdb/ChangeLog | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c3d47f9..1aa6fc1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2015-02-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	PR build/18033
+	* compile/compile-c-support.c (c_compute_program): Change // comment.
+	* compile/compile-object-load.c (setup_sections): Change // comment.
+
 2015-02-26  Joel Brobecker  <brobecker@adacore.com>
 
 	PR build/18033:
-- 
2.1.0

[-- Attachment #4: Type: message/rfc822, Size: 2115 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] Change // comment in gdb/compile/
Date: Thu, 26 Feb 2015 11:52:28 +0100

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

	PR build/18033
	* compile/compile-c-support.c (c_compute_program): Change // comment.
	* compile/compile-object-load.c (setup_sections): Change // comment.
---
 gdb/ChangeLog                     | 6 ++++++
 gdb/compile/compile-c-support.c   | 2 +-
 gdb/compile/compile-object-load.c | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5f5fa51..37a69e8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2015-02-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	PR build/18033
+	* compile/compile-c-support.c (c_compute_program): Change // comment.
+	* compile/compile-object-load.c (setup_sections): Change // comment.
+
 2015-02-26  Joel Brobecker  <brobecker@adacore.com>
 
 	PR build/18033:
diff --git a/gdb/compile/compile-c-support.c b/gdb/compile/compile-c-support.c
index 8f1f45d..1711cda 100644
--- a/gdb/compile/compile-c-support.c
+++ b/gdb/compile/compile-c-support.c
@@ -351,7 +351,7 @@ c_compute_program (struct compile_instance *inst,
 			" __gdb_intptr;\n",
 			buf);
 
-      // Iterate all log2 sizes in bytes supported by c_get_mode_for_size.
+      /* Iterate all log2 sizes in bytes supported by c_get_mode_for_size.  */
       for (i = 0; i < 4; ++i)
 	{
 	  const char *mode = c_get_mode_for_size (1 << i);
diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c
index 5903f18..e187970 100644
--- a/gdb/compile/compile-object-load.c
+++ b/gdb/compile/compile-object-load.c
@@ -67,7 +67,7 @@ setup_sections (bfd *abfd, asection *sect, void *data_voidp)
       if ((bfd_get_section_flags (abfd, sect) & SEC_ALLOC) == 0)
 	return;
 
-      // Make the memory always readable.
+      /* Make the memory always readable.  */
       prot = GDB_MMAP_PROT_READ;
       if ((bfd_get_section_flags (abfd, sect) & SEC_READONLY) == 0)
 	prot |= GDB_MMAP_PROT_WRITE;
-- 
2.1.0

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

[Joel Brobecker]

> > 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)
> 
> Done, also in the 7.9 branch.
> 
> Used your PR, only now I read I should have probably used a new one.

Thank you! It's OK about the PR - I just change its subject
to match both.

-- 
Joel

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

[Pedro Alves]

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

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

[Jan Kratochvil]

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