Patchwork [RESEND-2] Allow printing of escaped curly braces in assembler directives with operands

login
register
mail settings
Submitter Siddhesh Poyarekar
Date July 18, 2012, 4:53 p.m.
Message ID <20120718222357.117f8208@spoyarek>
Download mbox | patch
Permalink /patch/171754/
State New
Headers show

Comments

Siddhesh Poyarekar - July 18, 2012, 4:53 p.m.
Hi,

Resending. I did not get any responses the last two times and I too
forgot about it. Can someone please review this?

Thanks,
Siddhesh

Begin forwarded message:

Date: Tue, 3 Apr 2012 18:46:53 +0530
From: Siddhesh Poyarekar <siddhesh@redhat.com>
To: gcc-patches@gcc.gnu.org
Subject: Fw: [PATCH] Allow printing of escaped curly braces in
assembler directives with operands


Hi,

ping?

--
Siddhesh

Begin forwarded message:

Date: Tue, 27 Mar 2012 10:51:37 +0530
From: Siddhesh Poyarekar <siddhesh@redhat.com>
To: gcc-patches@gcc.gnu.org
Subject: [PATCH] Allow printing of escaped curly braces in assembler
directives with operands


Hi,

An assembler directive with an operand is filtered through
output_asm_insn (or asm_fprintf for gcc internal asm() directives) to
expand the operand values in the assembler as well as to choose
dialects if present. This patch is concerned primarily with the
dialects, since their syntax prevent inclusion of assembler strings
with curly braces, causing them to be interpreted as dialects.

The attached patch allows printing of curly braces in assembler by
preceding them with a \\. So to print the following code into assembler:

.pushsection ".foo"; .asciz "div { width : 50%% | height=10px }"; .long
42; .popsection

The following code needs to be used with this patch:

void f()
{
  asm ( ".pushsection \".foo\"; .asciz \"div \\{ width : 50%% |
height=10px \\} \"; .long %c0; .popsection" : : "i"(42) ); }

The other option to \\ (since it doesn't look as clean) was to use %
as an escape character, but I was not sure if that is a better looking
option or a worse looking one. I don't mind resubmitting the patch to
use %{ and %} to print curly braces if that is acceptable.

It is still possible to print curly braces in asm string literals
without operands since they do not undergo any transformation.

The patch does not introduce any regressions. I have tested this with
x86_64 and i686 and it works well with both of them.

Regards,
Siddhesh


gcc/ChangeLog:

2012-03-27  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* final.c (output_asm_insn, asm_fprintf): Print curly braces if
	preceded by an escaped backslash (\\).

testsuite/ChangeLog:

2012-03-27  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* gcc.dg/asm-braces.c: New test case.
Hans-Peter Nilsson - July 21, 2012, 2:24 a.m.
On Wed, 18 Jul 2012, Siddhesh Poyarekar wrote:

> Hi,
>
> Resending. I did not get any responses the last two times and I too
> forgot about it. Can someone please review this?

This is *not* an approver-review.

> An assembler directive with an operand is filtered through
> output_asm_insn (or asm_fprintf for gcc internal asm() directives) to
> expand the operand values in the assembler as well as to choose
> dialects if present. This patch is concerned primarily with the
> dialects, since their syntax prevent inclusion of assembler strings
> with curly braces, causing them to be interpreted as dialects.
>
> The attached patch allows printing of curly braces in assembler by
> preceding them with a \\.

(the single character \ to wit.  The \\ is the sequence you have
to write in C.)

Sounds like a good idea, but I think you shouldn't limit that to
"{}" but rather introduce \ to escape and cause any next
character to be emitted as-is, in particular "|".

> So to print the following code into assembler:
>
> .pushsection ".foo"; .asciz "div { width : 50%% | height=10px }"; .long
> 42; .popsection
>
> The following code needs to be used with this patch:
>
> void f()
> {
>   asm ( ".pushsection \".foo\"; .asciz \"div \\{ width : 50%% |
> height=10px \\} \"; .long %c0; .popsection" : : "i"(42) ); }
>
> The other option to \\ (since it doesn't look as clean) was to use %
> as an escape character, but I was not sure if that is a better looking
> option or a worse looking one. I don't mind resubmitting the patch to
> use %{ and %} to print curly braces if that is acceptable.

I'm on the fence regarding this, but leaning towards maybe
that's a better way forward, as it can't collide with any
current use of \\ (seeing as "%{" "%}" and "%|" currently fall
down into a gcc_unreachable).  If you change your patch to this,
make sure you include "%|" to emit "|".

(Hm, why is there a gcc_unreachable?  It should be an error as
"user" asm statements can reach it!)

> It is still possible to print curly braces in asm string literals
> without operands since they do not undergo any transformation.
>
> The patch does not introduce any regressions. I have tested this with
> x86_64 and i686 and it works well with both of them.

That's a target (yes, counting it as one target) that defines
ASSEMBLER_DIALECT wherein "{}|" has the predefined meaning you
try to escape.  I suggest testing a non-ASSEMBLER_DIALECT target
as well.  And check that no target uses \\ for something else
through ASM_FPRINTF_EXTENSIONS... ...so it seems like %{ etc.
would be safer.

> gcc/ChangeLog:
>
> 2012-03-27  Siddhesh Poyarekar  <siddhesh@redhat.com>
>
> 	* final.c (output_asm_insn, asm_fprintf): Print curly braces if
> 	preceded by an escaped backslash (\\).


>
> testsuite/ChangeLog:
>
> 2012-03-27  Siddhesh Poyarekar  <siddhesh@redhat.com>
>
> 	* gcc.dg/asm-braces.c: New test case.
>

Thanks and keep pinging.

brgds, H-P
Siddhesh Poyarekar - July 21, 2012, 2:57 p.m.
On Fri, 20 Jul 2012 22:24:57 -0400 (EDT), Hans-Peter wrote:
> Sounds like a good idea, but I think you shouldn't limit that to
> "{}" but rather introduce \ to escape and cause any next
> character to be emitted as-is, in particular "|".

I had dropped the change to escape "|" thinking it wasn't needed -- it
gets interpreted as a special character only if it is within a {}. It
makes sense to add it though, since there could be a use case to have
a '|' within the assembler dialect options.

> That's a target (yes, counting it as one target) that defines
> ASSEMBLER_DIALECT wherein "{}|" has the predefined meaning you
> try to escape.  I suggest testing a non-ASSEMBLER_DIALECT target
> as well.  And check that no target uses \\ for something else
> through ASM_FPRINTF_EXTENSIONS... ...so it seems like %{ etc.
> would be safer.

OK, I'll do this and post a modified patch. Thanks for the review!

Regards,
Siddhesh

Patch

diff --git a/gcc/final.c b/gcc/final.c
index 718caf1..2393c0f 100644
--- a/gcc/final.c
+++ b/gcc/final.c
@@ -3444,6 +3444,12 @@  output_asm_insn (const char *templ, rtx *operands)
 	  output_operand_lossage ("invalid %%-code");
 	break;
 
+      /* Escaped braces. Print them as is. */
+      case '\\':
+        if (*p == '{' || *p == '}')
+          c = *p++;
+        /* FALLTHROUGH */
+
       default:
 	putc (c, asm_out_file);
       }
@@ -3955,6 +3961,12 @@  asm_fprintf (FILE *file, const char *p, ...)
 	  }
 	break;
 
+      /* Escaped braces. Print them as is. */
+      case '\\':
+        if (*p == '{' || *p == '}')
+          c = *p++;
+        /* FALLTHROUGH */
+
       default:
 	putc (c, file);
       }
diff --git a/gcc/testsuite/gcc.dg/asm-braces.c b/gcc/testsuite/gcc.dg/asm-braces.c
new file mode 100644
index 0000000..4f428c8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asm-braces.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+void f()
+{
+  asm ( ".pushsection \".foo\"; .asciz \"div \\{ width : 50%% | height = 10px \\} \"; .long %c0; .popsection" : : "i"(42) );
+}
+
+/* { dg-final { scan-assembler "div { width : 50%% | height = 10px }" } } */
-- 
1.7.7.6