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 <siddh...@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 <siddh...@redhat.com> > > * gcc.dg/asm-braces.c: New test case. > Thanks and keep pinging. brgds, H-P