Message ID | 34941134-4f9f-29f5-18f1-af218824da24@redhat.com |
---|---|
State | New |
Headers | show |
Series | [committed,PR99680] Check empty constraint before using CONSTRAINT_LEN. | expand |
Hi!
On Sat, Mar 20, 2021 at 10:53:32AM -0400, Vladimir Makarov via Gcc-patches wrote:
> It seems CONSTRAINT_LEN treats constraint '\0' as one having length 1.
Maybe we should fix that? See attached patch.
Segher
diff --git a/gcc/genpreds.c b/gcc/genpreds.c
index 8499a2a2383b..b83a030d6a5d 100644
--- a/gcc/genpreds.c
+++ b/gcc/genpreds.c
@@ -1141,6 +1141,9 @@ write_insn_constraint_len (void)
" switch (fc)\n"
" {");
+ /* Empty constraints have length 0. */
+ printf (" case 0: return 0;\n");
+
for (i = 0; i < ARRAY_SIZE (constraints_by_letter_table); i++)
{
class constraint_data *c = constraints_by_letter_table[i];
@@ -1470,14 +1473,9 @@ write_tm_preds_h (void)
address_start, address_end);
write_allows_reg_mem_function ();
- if (constraint_max_namelen > 1)
- {
- write_insn_constraint_len ();
- puts ("#define CONSTRAINT_LEN(c_,s_) "
- "insn_constraint_len (c_,s_)\n");
- }
- else
- puts ("#define CONSTRAINT_LEN(c_,s_) 1\n");
+ write_insn_constraint_len ();
+ puts ("#define CONSTRAINT_LEN(c_,s_) insn_constraint_len (c_,s_)\n");
+
if (have_register_constraints)
puts ("extern enum reg_class reg_class_for_constraint_1 "
"(enum constraint_num);\n"
Segher Boessenkool <segher@kernel.crashing.org> writes: > Hi! > > On Sat, Mar 20, 2021 at 10:53:32AM -0400, Vladimir Makarov via Gcc-patches wrote: >> It seems CONSTRAINT_LEN treats constraint '\0' as one having length 1. > > Maybe we should fix that? See attached patch. I don't strongly object, but CONSTRAINT_LEN (0) doesn't feel to me like a question we should be asking. IMO it would be confusing to have, say: p += CONSTRAINT_LEN (*p); “stick” at a null terminator even though the code is notionally an increment. '\0' is just a normal string null terminator and so I don't think we should be processing it as if it were a constraint character. How about having a gcc_unreachable on zero instead? Thanks, Richard > Segher > > > diff --git a/gcc/genpreds.c b/gcc/genpreds.c > index 8499a2a2383b..b83a030d6a5d 100644 > --- a/gcc/genpreds.c > +++ b/gcc/genpreds.c > @@ -1141,6 +1141,9 @@ write_insn_constraint_len (void) > " switch (fc)\n" > " {"); > > + /* Empty constraints have length 0. */ > + printf (" case 0: return 0;\n"); > + > for (i = 0; i < ARRAY_SIZE (constraints_by_letter_table); i++) > { > class constraint_data *c = constraints_by_letter_table[i]; > @@ -1470,14 +1473,9 @@ write_tm_preds_h (void) > address_start, address_end); > write_allows_reg_mem_function (); > > - if (constraint_max_namelen > 1) > - { > - write_insn_constraint_len (); > - puts ("#define CONSTRAINT_LEN(c_,s_) " > - "insn_constraint_len (c_,s_)\n"); > - } > - else > - puts ("#define CONSTRAINT_LEN(c_,s_) 1\n"); > + write_insn_constraint_len (); > + puts ("#define CONSTRAINT_LEN(c_,s_) insn_constraint_len (c_,s_)\n"); > + > if (have_register_constraints) > puts ("extern enum reg_class reg_class_for_constraint_1 " > "(enum constraint_num);\n"
On 2021-03-22 6:22 a.m., Richard Sandiford wrote: > '\0' is just a normal string null terminator and so I don't think we > should be processing it as if it were a constraint character. > How about having a gcc_unreachable on zero instead? I would be nice to use gcc_unreachable but it requires to rewrite some loops working on constraints. All the loops check 0 explicitly to stop cycling, some of them still use CONSTRAIN_LEN for 0, e.g. code taken from reload.c: switch ((c = *p, len = CONSTRAINT_LEN (c, p)), c) { case '\0': len = 0; break; ... I think we could wait stage 1 to do this.
On Mon, Mar 22, 2021 at 10:22:55AM +0000, Richard Sandiford wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Sat, Mar 20, 2021 at 10:53:32AM -0400, Vladimir Makarov via Gcc-patches wrote: > >> It seems CONSTRAINT_LEN treats constraint '\0' as one having length 1. > > > > Maybe we should fix that? See attached patch. > > I don't strongly object, but CONSTRAINT_LEN (0) doesn't feel to me like a > question we should be asking. IMO it would be confusing to have, say: > > p += CONSTRAINT_LEN (*p); The CONSTRAINT_LEN macro has two arguments: the first character of the constraint string, and a pointer to it. That first character is just a premature optimisation if you ask me (and it shouldn't be a macro at all, it should use a function), but the important point is that the pointer to the string is the important one. So your example reads as p += CONSTRAINT_LEN (*p, p); > “stick” at a null terminator even though the code is notionally > an increment. Then don't write code like that? Errors should be handled *some* way, instead of nastiness like len = CONSTRAINT_LEN (c, constraint); do constraint++; while (--len && *constraint && *constraint != ','); (from recog.c). But since CONSTRAINT_LEN (0, "") should return *something*, returning length 0 makes a lot of sense. Many things do not need to treat it different from how real constraints are handled, but everything that wants can do so. > '\0' is just a normal string null terminator and so I don't think we > should be processing it as if it were a constraint character. But we don't. We process it as if it is the first character of the remaining constraint string. Which it is :-) > How about having a gcc_unreachable on zero instead? a) Then it should *definitely* not be a macro. b) We can do better error handling than that. Segher
On Mon, Mar 22, 2021 at 10:28:48AM -0400, Vladimir Makarov wrote: > On 2021-03-22 6:22 a.m., Richard Sandiford wrote: > >'\0' is just a normal string null terminator and so I don't think we > >should be processing it as if it were a constraint character. > >How about having a gcc_unreachable on zero instead? > > I would be nice to use gcc_unreachable but it requires to rewrite some > loops working on constraints. All the loops check 0 explicitly to stop > cycling, some of them still use CONSTRAIN_LEN for 0, e.g. code taken > from reload.c: > > switch ((c = *p, len = CONSTRAINT_LEN (c, p)), c) > { > case '\0': > len = 0; > break; > > ... > > I think we could wait stage 1 to do this. Yes. And such gcc_unreachable should be in as high-level code as possible, not deep in some macro. Segher
commit c1ab0c0336d85f5e97739060ecf77fd05ac86d2a Author: Vladimir N. Makarov <vmakarov@redhat.com> Date: Sat Mar 20 10:50:03 2021 -0400 [PR99680] Check empty constraint before using CONSTRAINT_LEN. It seems CONSTRAINT_LEN treats constraint '\0' as one having length 1. Therefore we read after the constraint string. The patch fixes it. gcc/ChangeLog: PR rtl-optimization/99680 * lra-constraints.c (skip_contraint_modifiers): Rename to skip_constraint_modifiers. (process_address_1): Check empty constraint before using CONSTRAINT_LEN. diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index 698d8d04a1e..fdfe953bcf5 100644 --- a/gcc/lra-constraints.c +++ b/gcc/lra-constraints.c @@ -3395,12 +3395,12 @@ equiv_address_substitution (struct address_info *ad) /* Skip all modifiers and whitespaces in constraint STR and return the result. */ static const char * -skip_contraint_modifiers (const char *str) +skip_constraint_modifiers (const char *str) { for (;;str++) switch (*str) { - case '+' : case '&' : case '=': case '*': case ' ': case '\t': + case '+': case '&' : case '=': case '*': case ' ': case '\t': case '$': case '^' : case '%': case '?': case '!': break; default: return str; @@ -3451,13 +3451,13 @@ process_address_1 (int nop, bool check_only_p, return false; constraint - = skip_contraint_modifiers (curr_static_id->operand[nop].constraint); + = skip_constraint_modifiers (curr_static_id->operand[nop].constraint); if (IN_RANGE (constraint[0], '0', '9')) { char *end; unsigned long dup = strtoul (constraint, &end, 10); constraint - = skip_contraint_modifiers (curr_static_id->operand[dup].constraint); + = skip_constraint_modifiers (curr_static_id->operand[dup].constraint); } cn = lookup_constraint (*constraint == '\0' ? "X" : constraint); /* If we have several alternatives or/and several constraints in an @@ -3465,10 +3465,10 @@ process_address_1 (int nop, bool check_only_p, use unknown constraint. The exception is an address constraint. If operand has one address constraint, probably all others constraints are address ones. */ - if (get_constraint_type (cn) != CT_ADDRESS - && *skip_contraint_modifiers (constraint - + CONSTRAINT_LEN (constraint[0], - constraint)) != '\0') + if (constraint[0] != '\0' && get_constraint_type (cn) != CT_ADDRESS + && *skip_constraint_modifiers (constraint + + CONSTRAINT_LEN (constraint[0], + constraint)) != '\0') cn = CONSTRAINT__UNKNOWN; if (insn_extra_address_constraint (cn) /* When we find an asm operand with an address constraint that