On Fri, Jan 17, 2025 at 10:47:09AM +0100, Jakub Jelinek wrote: > Hi! > > r15-2002 s390: Fully exploit vgm, vgbm, vrepi change added > some code to print_operand and added gcc_checking_asserts in there. > But print_operand ideally should have no assertions in it, as most > of the assumptions can be easily violated by people using it in > inline asm. > This issue in particular was seen by failure to compile s390-tools, > which had in its extended inline asm uses of %r1 and %r2. > I really don't know if they meant %%r1 and %%r2 or %1 and %2 and > will leave that decision to the maintainers, but the thing is that
https://github.com/ibm-s390-linux/s390-tools/pull/180 > %r1 and %r2 used to expand like %1 and %2 in GCC 14 and earlier, > now in checking build it ICEs and in --enable-checking=release build > fails to assemble (the checking assert is ignored and the compiler just uses > some uninitialized variables to emit something arbitrary). > > With the following patch it is diagnosed as error consistently > regardless if it is release checking or no checking or checking compiler. > > Bootstrapped/regtested on s390x-linux, ok for trunk? Ok. Thanks for fixing and pointing this out. Much appreciated. > > Note, I see also > else if (GET_CODE (x) == UNSPEC && XINT (x, 1) == UNSPEC_TLSLDM) > { > fprintf (file, "%s", ":tls_ldcall:"); > const char *name = get_some_local_dynamic_name (); > gcc_assert (name); > assemble_name (file, name); > } > in print_operand, maybe that isn't a big deal because it might be > impossible to construct inline asm argument which is UNSPEC_TLSLDM. > And then there is > case 'e': case 'f': > case 's': case 't': > { > int start, end; > int len; > bool ok; > > len = (code == 's' || code == 'e' ? 64 : 32); > ok = s390_contiguous_bitmask_p (ival, true, len, &start, &end); > gcc_assert (ok); > if (code == 's' || code == 't') > ival = start; > else > ival = end; > } > break; > which likely should be also output_operand_lossage but I haven't tried > to reproduce that. AFAICS those shouldn't be constructable via inline asm arguments. However, I agree that asserts are not ideal here. For example, in *insv_z10_noshift_cc the second alternative uses %e for an operand which isn't guarded by predicate contiguous_bitmask_operand. Though, I haven't come up with a reproducer either. Cheers, Stefan > > 2025-01-17 Jakub Jelinek <ja...@redhat.com> > > PR target/118511 > * config/s390/s390.cc (print_operand) <case 'p'>: Use > output_operand_lossage instead of gcc_checking_assert. > (print_operand) <case 'q'>: Likewise. > (print_operand) <case 'r'>: Likewise. > > * gcc.target/s390/pr118511.c: New test. > > --- gcc/config/s390/s390.cc.jj 2025-01-10 16:09:37.280043199 +0100 > +++ gcc/config/s390/s390.cc 2025-01-16 10:43:58.533809724 +0100 > @@ -8688,8 +8688,12 @@ print_operand (FILE *file, rtx x, int co > { > machine_mode mode; > short imm; > - bool b = s390_constant_via_vrepi_p (x, &mode, &imm); > - gcc_checking_assert (b); > + if (!s390_constant_via_vrepi_p (x, &mode, &imm)) > + { > + output_operand_lossage ("invalid constant for output " > + "modifier '%c'", code); > + return; > + } > switch (mode) > { > case QImode: > @@ -8714,8 +8718,12 @@ print_operand (FILE *file, rtx x, int co > { > machine_mode mode; > int start, end; > - bool b = s390_constant_via_vgm_p (x, &mode, &start, &end); > - gcc_checking_assert (b); > + if (!s390_constant_via_vgm_p (x, &mode, &start, &end)) > + { > + output_operand_lossage ("invalid constant for output " > + "modifier '%c'", code); > + return; > + } > switch (mode) > { > case QImode: > @@ -8739,8 +8747,12 @@ print_operand (FILE *file, rtx x, int co > case 'r': > { > unsigned mask; > - bool b = s390_constant_via_vgbm_p (x, &mask); > - gcc_checking_assert (b); > + if (!s390_constant_via_vgbm_p (x, &mask)) > + { > + output_operand_lossage ("invalid constant for output " > + "modifier '%c'", code); > + return; > + } > fprintf (file, "%u", mask); > } > return; > --- gcc/testsuite/gcc.target/s390/pr118511.c.jj 2025-01-16 > 10:51:14.025721964 +0100 > +++ gcc/testsuite/gcc.target/s390/pr118511.c 2025-01-16 10:50:48.009085651 > +0100 > @@ -0,0 +1,11 @@ > +/* PR target/118511 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +void > +foo (void) > +{ > + asm volatile ("# %p0" : : "d" (1)); /* { dg-error "invalid 'asm': > invalid constant for output modifier 'p'" } */ > + asm volatile ("# %q0" : : "d" (1)); /* { dg-error "invalid 'asm': > invalid constant for output modifier 'q'" } */ > + asm volatile ("# %r0" : : "d" (1)); /* { dg-error "invalid 'asm': > invalid constant for output modifier 'r'" } */ > +} > > Jakub >
