From patchwork Fri Jun 11 18:00:13 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Disallow side-effects in inline asm operands unless (a new) _ constraint modifier is used (PR middle-end/44492) Date: Fri, 11 Jun 2010 08:00:13 -0000 From: Jakub Jelinek X-Patchwork-Id: 55515 Message-Id: <20100611180013.GC7811@tyan-ft48-01.lab.bos.redhat.com> To: gcc-patches@gcc.gnu.org Hi! As discussed in the PR, currently auto-inc-dec.c happily propagates {PRE,POST}_{INC,DEC_MODIFY} side-effects into inline asm memory operands when constraints like "g" or "m" are used. Unfortunately that means a lot of inline asms out there work by purse accident - if a mem operand can have a side-effect, special case needs to be done on the inline asm pattern side. In particular, it has to be used exactly once in an instruction that is capable of handling side-effects and on many targets needs to be accompanied by special modifier too (e.g. on ppc/ppc64 it needs to be used in insn like lwz%U1 %0,%1, if the %U1 part is omitted, code is silently miscompiled). A lot of inline asms I've grepped for don't use "m" or "=m" operands anywhere, many look e.g. like asm ("... %2 ... " : "=m" (*p) : "m" (*p), "r" (p)); and the "m" and "=m" operands are there just to tell GCC that the insn (might) reads or writes that memory. If the memory operand with side-effects isn't used at all, the side-effects don't happen, if it is used more than once, they happen multiple times. The following patch disallows side-effects in inline asm operands unless specifically allowed in source code ( _ constraint modifier says this operand obeys all the rules that it can contain side-effects). Bootstrapped/regtested on x86_64-linux and i686-linux (yeah, I know, they aren't AUTO_INC_DEC) and tested on powerpc64-linux cross. Ok for trunk? 2010-06-10 Jakub Jelinek PR middle-end/44492 * recog.h (struct recog_data): Add is_asm field. * recog.c (asm_operand_ok, constrain_operands): Handle _ constraint modifier. If it is not present and operand contains {PRE,POST}_{INC,DEC,MODIFY}, return 0. (extract_insn): Initialize recog_data.is_asm. (preprocess_constraints): Handle _ constraint modifier. * ira-costs.c (record_reg_classes): Likewise. * ira-lives.c (single_reg_class): Likewise. * stmt.c (parse_output_constraint, parse_input_constraint): Likewise. * postreload.c (reload_cse_simplify_operands): Likewise. * reload.c (find_reloads): Likewise. * reload1.c (maybe_fix_stack_asms): Likewise. * doc/md.texi (Modifiers): Document _ constraint modifier. * g++.dg/torture/pr44492.C: New test. Jakub --- gcc/reload1.c.jj 2010-06-10 19:32:01.000000000 +0200 +++ gcc/reload1.c 2010-06-11 10:00:12.000000000 +0200 @@ -1414,6 +1414,7 @@ maybe_fix_stack_asms (void) case '>': case 'V': case 'o': case '&': case 'E': case 'F': case 's': case 'i': case 'n': case 'X': case 'I': case 'J': case 'K': case 'L': case 'M': case 'N': case 'O': case 'P': + case '_': case TARGET_MEM_CONSTRAINT: break; --- gcc/recog.h.jj 2010-05-13 12:21:21.000000000 +0200 +++ gcc/recog.h 2010-06-11 10:05:09.000000000 +0200 @@ -230,6 +230,9 @@ struct recog_data /* The number of alternatives in the constraints for the insn. */ char n_alternatives; + /* True if insn is ASM_OPERANDS. */ + bool is_asm; + /* Specifies whether an insn alternative is enabled using the `enabled' attribute in the insn pattern definition. For back ends not using the `enabled' attribute the array fields are --- gcc/postreload.c.jj 2010-06-10 19:32:00.000000000 +0200 +++ gcc/postreload.c 2010-06-11 10:00:11.000000000 +0200 @@ -537,7 +537,7 @@ reload_cse_simplify_operands (rtx insn, { case '=': case '+': case '?': case '#': case '&': case '!': - case '*': case '%': + case '*': case '%': case '_': case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': case '<': case '>': case 'V': case 'o': --- gcc/recog.c.jj 2010-06-10 19:32:00.000000000 +0200 +++ gcc/recog.c 2010-06-11 10:14:55.000000000 +0200 @@ -1601,6 +1601,9 @@ int asm_operand_ok (rtx op, const char *constraint, const char **constraints) { int result = 0; +#ifdef AUTO_INC_DEC + bool incdec_ok = false; +#endif /* Use constrain_operands after reload. */ gcc_assert (!reload_completed); @@ -1608,7 +1611,7 @@ asm_operand_ok (rtx op, const char *cons /* Empty constraint string is the same as "X,...,X", i.e. X for as many alternatives as required to match the other operands. */ if (*constraint == '\0') - return 1; + result = 1; while (*constraint) { @@ -1629,6 +1632,12 @@ asm_operand_ok (rtx op, const char *cons case '?': break; + case '_': +#ifdef AUTO_INC_DEC + incdec_ok = true; +#endif + break; + case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': /* If caller provided constraints pointer, look up @@ -1814,6 +1823,23 @@ asm_operand_ok (rtx op, const char *cons return 0; } +#ifdef AUTO_INC_DEC + /* For operands without _ constraint modifier reject side-effects. */ + if (!incdec_ok && result && MEM_P (op)) + switch (GET_CODE (XEXP (op, 0))) + { + case PRE_INC: + case POST_INC: + case PRE_DEC: + case POST_DEC: + case PRE_MODIFY: + case POST_MODIFY: + return 0; + default: + break; + } +#endif + return result; } @@ -2039,6 +2065,7 @@ extract_insn (rtx insn) recog_data.n_operands = 0; recog_data.n_alternatives = 0; recog_data.n_dups = 0; + recog_data.is_asm = false; switch (GET_CODE (body)) { @@ -2085,6 +2112,7 @@ extract_insn (rtx insn) while (*p) recog_data.n_alternatives += (*p++ == ','); } + recog_data.is_asm = true; break; } fatal_insn_not_found (insn); @@ -2198,6 +2226,7 @@ preprocess_constraints (void) case 's': case 'i': case 'n': case 'I': case 'J': case 'K': case 'L': case 'M': case 'N': case 'O': case 'P': + case '_': /* These don't say anything we care about. */ break; @@ -2396,7 +2425,7 @@ constrain_operands (int strict) break; case '?': case '!': case '*': case '%': - case '=': case '+': + case '=': case '+': case '_': break; case '#': @@ -2699,6 +2728,29 @@ constrain_operands (int strict) = recog_data.operand[funny_match[funny_match_index].this_op]; } +#ifdef AUTO_INC_DEC + /* For operands without _ constraint modifier reject + side-effects. */ + if (recog_data.is_asm) + { + for (opno = 0; opno < recog_data.n_operands; opno++) + if (MEM_P (recog_data.operand[opno])) + switch (GET_CODE (XEXP (recog_data.operand[opno], 0))) + { + case PRE_INC: + case POST_INC: + case PRE_DEC: + case POST_DEC: + case PRE_MODIFY: + case POST_MODIFY: + if (strchr (recog_data.constraints[opno], '_') == 0) + return 0; + break; + default: + break; + } + } +#endif return 1; } } --- gcc/stmt.c.jj 2010-06-10 19:32:01.000000000 +0200 +++ gcc/stmt.c 2010-06-11 10:00:11.000000000 +0200 @@ -369,7 +369,7 @@ parse_output_constraint (const char **co case 'E': case 'F': case 'G': case 'H': case 's': case 'i': case 'n': case 'I': case 'J': case 'K': case 'L': case 'M': - case 'N': case 'O': case 'P': case ',': + case 'N': case 'O': case 'P': case ',': case '_': break; case '0': case '1': case '2': case '3': case '4': @@ -465,7 +465,7 @@ parse_input_constraint (const char **con break; case '<': case '>': - case '?': case '!': case '*': case '#': + case '?': case '!': case '*': case '#': case '_': case 'E': case 'F': case 'G': case 'H': case 's': case 'i': case 'n': case 'I': case 'J': case 'K': case 'L': case 'M': --- gcc/ira-costs.c.jj 2010-06-10 19:32:01.000000000 +0200 +++ gcc/ira-costs.c 2010-06-11 10:00:11.000000000 +0200 @@ -398,7 +398,7 @@ record_reg_classes (int n_alts, int n_op case '?': alt_cost += 2; - case '!': case '#': case '&': + case '!': case '#': case '&': case '_': case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': break; --- gcc/reload.c.jj 2010-06-10 19:32:00.000000000 +0200 +++ gcc/reload.c 2010-06-11 10:00:11.000000000 +0200 @@ -3106,7 +3106,7 @@ find_reloads (rtx insn, int replace, int c = '\0'; break; - case '=': case '+': case '*': + case '=': case '+': case '*': case '_': break; case '%': --- gcc/doc/md.texi.jj 2010-06-11 09:38:04.000000000 +0200 +++ gcc/doc/md.texi 2010-06-11 14:37:13.000000000 +0200 @@ -1595,6 +1595,21 @@ Says that the following character should register preferences. @samp{*} has no effect on the meaning of the constraint as a constraint, and no effect on reloading. +@cindex @samp{_} in constraint +@item _ +Means that this operand in inline asm can contain side-effects. +By default side-effects in operands of inline asm are disallowed, +because there is no guarantee that the operands are used exactly once +for the inline asm in an instruction that can update the side-effects +(pre- or post-increment or decrement of a register used in memory +operand addressing). +When @samp{_} is added, the operand must be used exactly once in an +instruction that will do the update and any target specific means +of ensuring that the side-effects happen in an instruction are present. +E.g. on PowerPC that means using the operand with @samp{_} constraint +modifier only in load or store instructions together with @samp{%U}@var{N} +suffix for the instruction, etc. + @ifset INTERNALS Here is an example: the 68000 has an instruction to sign-extend a halfword in a data register, and can also sign-extend a value by --- gcc/ira-lives.c.jj 2010-06-10 19:32:00.000000000 +0200 +++ gcc/ira-lives.c 2010-06-11 10:00:11.000000000 +0200 @@ -640,6 +640,7 @@ single_reg_class (const char *constraint case '%': case '!': case '?': + case '_': break; case 'i': if (CONSTANT_P (op) --- gcc/testsuite/g++.dg/torture/pr44492.C.jj 2010-06-11 10:00:12.000000000 +0200 +++ gcc/testsuite/g++.dg/torture/pr44492.C 2010-06-11 10:00:12.000000000 +0200 @@ -0,0 +1,31 @@ +// PR middle-end/44492 +// { dg-do run } + +struct T { unsigned long p; }; +struct S { T a, b, c; unsigned d; }; + +__attribute__((noinline)) +void +bar (const T &x, const T &y) +{ + if (x.p != 0x2348 || y.p != 0x2346) + __builtin_abort (); +} + +__attribute__((noinline)) +void +foo (S &s, T e) +{ + unsigned long a = e.p; + unsigned long b = s.b.p; + __asm__ volatile ("" : : "rm" (a), "rm" (b)); + bar (e, s.b); +} + +int +main () +{ + S s = { { 0x2345 }, { 0x2346 }, { 0x2347 }, 6 }; + T t = { 0x2348 }; + foo (s, t); +}