| Message ID | 20120718222357.117f8208@spoyarek |
|---|---|
| State | New |
| Headers | show |
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
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
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