diff mbox series

libgccjit: Add option to allow special characters in function names

Message ID 63ab69e027ce69d97dd361561675d3c323983b92.camel@zoho.com
State Accepted
Headers
Series libgccjit: Add option to allow special characters in function names |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Antoni Boucher Feb. 15, 2024, 10:08 p.m. UTC 
  Hi.
This patch adds a new option to allow special characters like . and $
in function names.
This is useful to allow for mangling using those characters.
Thanks for the review.

Comments

David Malcolm Feb. 20, 2024, 8:50 p.m. UTC | #1 
On Thu, 2024-02-15 at 17:08 -0500, Antoni Boucher wrote:
> Hi.
> This patch adds a new option to allow special characters like . and $
> in function names.
> This is useful to allow for mangling using those characters.
> Thanks for the review.

Thanks for the patch.

> diff --git a/gcc/jit/docs/topics/contexts.rst b/gcc/jit/docs/topics/contexts.rst
> index 10a0e50f9f6..4af75ea7418 100644
> --- a/gcc/jit/docs/topics/contexts.rst
> +++ b/gcc/jit/docs/topics/contexts.rst
> @@ -453,6 +453,10 @@ Boolean options
>       If true, the :type:`gcc_jit_context` will not clean up intermediate files
>       written to the filesystem, and will display their location on stderr.
>  
> +  .. macro:: GCC_JIT_BOOL_OPTION_SPECIAL_CHARS_IN_FUNC_NAMES
> +
> +     If true, allow special characters like . and $ in function names.

The documentation and the comment in libgccjit.h say:
  "allow special characters like . and $ in function names."
and on reading the implementation, the special characters are exactly
'.' and '$'.

The API seems rather arbitrary and inflexible to me; why the choice of
those characters?  Presumably those are the ones that Rust's mangling
scheme uses, but do other mangling schemes require other chars?

How about an API for setting the valid chars, something like:

extern void
gcc_jit_context_set_valid_symbol_chars (gcc_jit_context *ctxt,
                                        const char *chars);

to specify the chars that are valid in addition to underscore and
alphanumeric.

In your case you'd call:

  gcc_jit_context_set_valid_symbol_chars (ctxt, ".$");

Or is that overkill?

Dave
Iain Sandoe Feb. 20, 2024, 9:05 p.m. UTC | #2 
> On 20 Feb 2024, at 20:50, David Malcolm <dmalcolm@redhat.com> wrote:
> 
> On Thu, 2024-02-15 at 17:08 -0500, Antoni Boucher wrote:
>> Hi.
>> This patch adds a new option to allow special characters like . and $
>> in function names.
>> This is useful to allow for mangling using those characters.
>> Thanks for the review.
> 
> Thanks for the patch.
> 
>> diff --git a/gcc/jit/docs/topics/contexts.rst b/gcc/jit/docs/topics/contexts.rst
>> index 10a0e50f9f6..4af75ea7418 100644
>> --- a/gcc/jit/docs/topics/contexts.rst
>> +++ b/gcc/jit/docs/topics/contexts.rst
>> @@ -453,6 +453,10 @@ Boolean options
>>      If true, the :type:`gcc_jit_context` will not clean up intermediate files
>>      written to the filesystem, and will display their location on stderr.
>> 
>> +  .. macro:: GCC_JIT_BOOL_OPTION_SPECIAL_CHARS_IN_FUNC_NAMES
>> +
>> +     If true, allow special characters like . and $ in function names.
> 
> The documentation and the comment in libgccjit.h say:
>  "allow special characters like . and $ in function names."
> and on reading the implementation, the special characters are exactly
> '.' and '$'.
> 
> The API seems rather arbitrary and inflexible to me; why the choice of
> those characters?  Presumably those are the ones that Rust's mangling
> scheme uses, but do other mangling schemes require other chars?
> 
> How about an API for setting the valid chars, something like:
> 
> extern void
> gcc_jit_context_set_valid_symbol_chars (gcc_jit_context *ctxt,
>                                        const char *chars);
> 
> to specify the chars that are valid in addition to underscore and
> alphanumeric.
> 
> In your case you'd call:
> 
>  gcc_jit_context_set_valid_symbol_chars (ctxt, ".$");
> 
> Or is that overkill?

If we ever wanted to support objective-c (NeXT runtime) then we’d need to
be able to support +,-,[,] space and : at least.  The interesting thing there is
that most assemblers do not support that either (and the symbols then need
to be quoted into the assembler) .

So, it’s not (IMO) overkill considering at least one potential extension.

Iain

> 
> Dave
>
Antoni Boucher Feb. 29, 2024, 10:13 p.m. UTC | #3 
Hi and thanks for the review.
I thought it would be a bit weird to have an option to change which 
characters are allowed, but I can't think of a better solution.
Here's the updated patch that now allow arbitrary characters.

Le 2024-02-20 à 16 h 05, Iain Sandoe a écrit :
> 
> 
>> On 20 Feb 2024, at 20:50, David Malcolm <dmalcolm@redhat.com> wrote:
>>
>> On Thu, 2024-02-15 at 17:08 -0500, Antoni Boucher wrote:
>>> Hi.
>>> This patch adds a new option to allow special characters like . and $
>>> in function names.
>>> This is useful to allow for mangling using those characters.
>>> Thanks for the review.
>>
>> Thanks for the patch.
>>
>>> diff --git a/gcc/jit/docs/topics/contexts.rst b/gcc/jit/docs/topics/contexts.rst
>>> index 10a0e50f9f6..4af75ea7418 100644
>>> --- a/gcc/jit/docs/topics/contexts.rst
>>> +++ b/gcc/jit/docs/topics/contexts.rst
>>> @@ -453,6 +453,10 @@ Boolean options
>>>       If true, the :type:`gcc_jit_context` will not clean up intermediate files
>>>       written to the filesystem, and will display their location on stderr.
>>>
>>> +  .. macro:: GCC_JIT_BOOL_OPTION_SPECIAL_CHARS_IN_FUNC_NAMES
>>> +
>>> +     If true, allow special characters like . and $ in function names.
>>
>> The documentation and the comment in libgccjit.h say:
>>   "allow special characters like . and $ in function names."
>> and on reading the implementation, the special characters are exactly
>> '.' and '$'.
>>
>> The API seems rather arbitrary and inflexible to me; why the choice of
>> those characters?  Presumably those are the ones that Rust's mangling
>> scheme uses, but do other mangling schemes require other chars?
>>
>> How about an API for setting the valid chars, something like:
>>
>> extern void
>> gcc_jit_context_set_valid_symbol_chars (gcc_jit_context *ctxt,
>>                                         const char *chars);
>>
>> to specify the chars that are valid in addition to underscore and
>> alphanumeric.
>>
>> In your case you'd call:
>>
>>   gcc_jit_context_set_valid_symbol_chars (ctxt, ".$");
>>
>> Or is that overkill?
> 
> If we ever wanted to support objective-c (NeXT runtime) then we’d need to
> be able to support +,-,[,] space and : at least.  The interesting thing there is
> that most assemblers do not support that either (and the symbols then need
> to be quoted into the assembler) .
> 
> So, it’s not (IMO) overkill considering at least one potential extension.
> 
> Iain
> 
>>
>> Dave
>>
>
diff mbox series

Patch

From 89a92e561ca83a622dcc22a6500ca2b17551edb1 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Thu, 15 Feb 2024 17:03:22 -0500
Subject: [PATCH] libgccjit: Add option to allow special characters in function
 names

gcc/jit/ChangeLog:

	* docs/topics/contexts.rst: Add documentation for new option.
	* jit-recording.cc (recording::context::get_bool_option): New
	method.
	* jit-recording.h (get_bool_option): New method.
	* libgccjit.cc (gcc_jit_context_new_function): Allow special
	characters in function names.
	* libgccjit.h (enum gcc_jit_bool_option): New option.

gcc/testsuite/ChangeLog:

	* jit.dg/test-special-chars.c: New test.
---
 gcc/jit/docs/topics/contexts.rst          |  4 +++
 gcc/jit/jit-recording.cc                  | 15 ++++++++-
 gcc/jit/jit-recording.h                   |  3 ++
 gcc/jit/libgccjit.cc                      |  8 +++--
 gcc/jit/libgccjit.h                       |  3 ++
 gcc/testsuite/jit.dg/test-special-chars.c | 41 +++++++++++++++++++++++
 6 files changed, 71 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-special-chars.c

diff --git a/gcc/jit/docs/topics/contexts.rst b/gcc/jit/docs/topics/contexts.rst
index 10a0e50f9f6..4af75ea7418 100644
--- a/gcc/jit/docs/topics/contexts.rst
+++ b/gcc/jit/docs/topics/contexts.rst
@@ -453,6 +453,10 @@  Boolean options
      If true, the :type:`gcc_jit_context` will not clean up intermediate files
      written to the filesystem, and will display their location on stderr.
 
+  .. macro:: GCC_JIT_BOOL_OPTION_SPECIAL_CHARS_IN_FUNC_NAMES
+
+     If true, allow special characters like . and $ in function names.
+
 .. function:: void \
               gcc_jit_context_set_bool_allow_unreachable_blocks (gcc_jit_context *ctxt, \
                                                                  int bool_value)
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index 83a8b299b91..962c37ee87e 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -1511,6 +1511,18 @@  recording::context::set_bool_option (enum gcc_jit_bool_option opt,
   log_bool_option (opt);
 }
 
+int
+recording::context::get_bool_option (enum gcc_jit_bool_option opt)
+{
+  if (opt < 0 || opt >= GCC_JIT_NUM_BOOL_OPTIONS)
+    {
+      add_error (NULL,
+		 "unrecognized (enum gcc_jit_bool_option) value: %i", opt);
+      return 0;
+    }
+  return m_bool_options[opt];
+}
+
 void
 recording::context::set_inner_bool_option (enum inner_bool_option inner_opt,
 					   int value)
@@ -1863,7 +1875,8 @@  static const char * const
   "GCC_JIT_BOOL_OPTION_DUMP_SUMMARY",
   "GCC_JIT_BOOL_OPTION_DUMP_EVERYTHING",
   "GCC_JIT_BOOL_OPTION_SELFCHECK_GC",
-  "GCC_JIT_BOOL_OPTION_KEEP_INTERMEDIATES"
+  "GCC_JIT_BOOL_OPTION_KEEP_INTERMEDIATES",
+  "GCC_JIT_BOOL_OPTION_SPECIAL_CHARS_IN_FUNC_NAMES",
 };
 
 static const char * const
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 4833b2d3f52..377b60c662c 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -267,6 +267,9 @@  public:
   set_bool_option (enum gcc_jit_bool_option opt,
 		   int value);
 
+  int
+  get_bool_option (enum gcc_jit_bool_option opt);
+
   void
   set_inner_bool_option (enum inner_bool_option inner_opt,
 			 int value);
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 5a1308b2b8c..3c5ff5a2a59 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -1222,10 +1222,13 @@  gcc_jit_context_new_function (gcc_jit_context *ctxt,
      Eventually we'll need some way to interact with e.g. C++ name
      mangling.  */
   {
+    int special_chars_allowed
+      = ctxt->get_bool_option (GCC_JIT_BOOL_OPTION_SPECIAL_CHARS_IN_FUNC_NAMES);
     /* Leading char: */
     char ch = *name;
     RETURN_NULL_IF_FAIL_PRINTF2 (
-	ISALPHA (ch) || ch == '_',
+	ISALPHA (ch) || ch == '_' || (special_chars_allowed
+	  && (ch == '.' || ch == '$')),
 	ctxt, loc,
 	"name \"%s\" contains invalid character: '%c'",
 	name, ch);
@@ -1233,7 +1236,8 @@  gcc_jit_context_new_function (gcc_jit_context *ctxt,
     for (const char *ptr = name + 1; (ch = *ptr); ptr++)
       {
 	RETURN_NULL_IF_FAIL_PRINTF2 (
-	  ISALNUM (ch) || ch == '_',
+	  ISALNUM (ch) || ch == '_' || (special_chars_allowed
+	    && (ch == '.' || ch == '$')),
 	  ctxt, loc,
 	  "name \"%s\" contains invalid character: '%c'",
 	  name, ch);
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index d45b767c262..694f45f34e5 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -244,6 +244,9 @@  enum gcc_jit_bool_option
      their location on stderr.  */
   GCC_JIT_BOOL_OPTION_KEEP_INTERMEDIATES,
 
+  /* If true, allow special characters like . and $ in function names.  */
+  GCC_JIT_BOOL_OPTION_SPECIAL_CHARS_IN_FUNC_NAMES,
+
   GCC_JIT_NUM_BOOL_OPTIONS
 };
 
diff --git a/gcc/testsuite/jit.dg/test-special-chars.c b/gcc/testsuite/jit.dg/test-special-chars.c
new file mode 100644
index 00000000000..1bca71164d8
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-special-chars.c
@@ -0,0 +1,41 @@ 
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  gcc_jit_context_set_bool_option (ctxt,
+    GCC_JIT_BOOL_OPTION_SPECIAL_CHARS_IN_FUNC_NAMES, 1);
+
+  /* Let's try to inject the equivalent of:
+       void
+       name$with.special_chars (void)
+       {
+       }
+   */
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+
+  /* Build the test_fn.  */
+  gcc_jit_function *test_fn =
+    gcc_jit_context_new_function (ctxt, NULL,
+				  GCC_JIT_FUNCTION_EXPORTED,
+				  void_type,
+				  "name$with.special_chars",
+				  0, NULL,
+				  0);
+
+  gcc_jit_block *block = gcc_jit_function_new_block (test_fn, NULL);
+  gcc_jit_block_end_with_void_return (
+    block, NULL);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  CHECK_NON_NULL (result);
+}
-- 
2.43.0