[PATCH v2 31/65] MIPS: use is_whitespace()

[Maciej W. Rozycki]

On Mon, 3 Feb 2025, Jan Beulich wrote:

> >> --- a/gas/config/tc-mips.c
> >> +++ b/gas/config/tc-mips.c
> >> @@ -14388,7 +14388,7 @@ mips16_ip (char *str, struct mips_cl_ins
> >>    struct mips_operand_token *tokens;
> >>    unsigned int l;
> >>  
> >> -  for (s = str; *s != '\0' && *s != '.' && *s != ' '; ++s)
> >> +  for (s = str; *s != '\0' && *s != '.' && !is_whitespace (*s); ++s)
> >                    ^^^^^^^^^^
> >  Shouldn't this also be `!is_end_of_stmt (*s)'?
> 
> This is the kind of question you as the target maintainer really
> will want to answer. All I could do is try to guess where
> conversion might be on order.

 All I can say if `*s' is '\0', then we're at the end of an assembly 
instruction mnemonic that has no operands following, so my understanding 
is it is indeed the case for `!is_end_of_stmt (*s)'.  However since you 
made such changes elsewhere but chose not to make one on this occasion, 
I've asked whether this has been intentional or just a missed case.

> >  I think this only causes obfuscation to this already messed up statement.  
> > Since there are only two cases here really ('\0' does nothing and is the 
> > only remaining possibility here, guaranteed by the loop right above) can 
> > you please rewrite this as:
> > 
> >   if (c == '.')
> >     {
> >       ...
> >     }
> >   else if (is_whitespace (c))
> >     s++;
> > 
> > or suchlike?
> 
> Possibly. On a similar question from Richard on aarch64 I indicated that
> from other projects I'm working on I'm used to using switch() in such
> cases, even if at a certain point there may be just a single case label.
> This is to ease future addition of new further labels.

 This is generic MIPS assembly language syntax, which is unlikely to ever 
change, and then for the MIPS16 intruction set, which has been effectively 
a dead end for decades now, even the MIPS16e2 extension ~10 years ago was 
a huge surprise and a one-off effort due to a specific customer request, 
so we can safely assume nothing else will ever happen again here.  So I 
think we need to optimise for code clarity rather than minimising highly 
unlikely future changes.  Yes, you need the backend maintainer's knowledge 
to decide here.

 For the record we're handling explicit instruction size override suffixes 
on MIPS16 mnemonics here, so the three cases the current switch statement 
handles is:

- '\0': end of mnemonic, instruction ends w/o operands, no size override,

- ' ':  end of mnemonic, operands follow, no size override,

- '.':  end of mnemonic, a size override suffix follows (then either the 
        instruction ends or operands follow).

There's simply no room for expansion here, and as I say the instruction 
set is a dead end and therefore in the maintenance mode.

 NB this stuff is extensively covered and should therefore be safe to 
apply cleanups to with little concern as to possible breakage, cf.:

$ grep '\.[et]\b' gas/testsuite/gas/mips/*.s

and I put significant effort to get this stuff right, as it used to be 
broken.  See commit 7fd539200562 ("MIPS16: Switch to 32-bit opcode table 
interpretation"), commit 3fb49709438e ("MIPS16/GAS: Fix forced size 
suffixes with argumentless instructions"), and commit 25499ac7ee92 
("MIPS16e2: Add MIPS16e2 ASE support") for the most relevant changes.

> >> @@ -14417,7 +14418,7 @@ mips16_ip (char *str, struct mips_cl_ins
> >>  	}
> >>        if (*s == '\0')
> > 
> >  And `is_end_of_stmt (*s)' here presumably too?
> 
> See above. It has been a lot of targets all doing things (often just
> slightly) differently, so I can only guess that I may have got the
> impression that somewhere up the callstack something nul-terminates the
> string.

 It is an analogous situation once the override suffix has been swallowed.

  Maciej