Re: [PATCH] x86: Apply standalone prefixes to the following instruction

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Jul 19, 2019 at 10:23 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Jul 19, 2019 at 10:11 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Fri, Jul 19, 2019 at 9:25 AM Jan Beulich <JBeulich@suse.com> wrote:
> > >
> > > On 19.07.2019 17:01,  H.J. Lu  wrote:
> > > > On Fri, Jul 19, 2019 at 1:46 AM Jan Beulich <JBeulich@suse.com> wrote:
> > > >>
> > > >> On 19.07.2019 00:26,  H.J. Lu  wrote:
> > > >>> Standalone prefixes should be applied to the following instruction,
> > > >>> instead of being treated as regular instructions.  An error should be
> > > >>> issued when a standalone prefix is at the end of source or isn't
> > > >>> followed by an instruction in the same section.
> > > >>
> > > >> Commenting here, because commenting on the actual code fragments is
> > > >> not easily possible with the patch sent as attachment.
> > > >>
> > > >> For one, I don't agree that errors should be issued when switching
> > > >> sections. Clever assembly programming can easily result in the
> > > >> actual section later getting resumed, and an appropriate insn being
> > > >> there.
> > > >
> > > > Yes, you will get an error which can be easily fixed.
> > >
> > > For both this and ...
> > >
> > > >> And then I'm getting the impression that the change here is going
> > > >> to break things like
> > > >>
> > > >> static inline unsigned int find_first_set_bit(unsigned long word)
> > > >> {
> > > >>       asm ( "rep; bsf %1,%0" : "=r" (word) : "rm" (word) );
> > > >>       return (unsigned int)word;
> > > >> }
> > > >
> > > > "rep; bsf" works like "rep bsf".
> > > >
> > > >> (quoted from Xen sources), being a backwards compatible
> > > >> representation of tzcnt. Just like such have shown up in the past,
> > > >> REP prefixes could easily obtain meaning for other insns going
> > > >> forward, so tagging individual templates with RepPrefixOk is not
> > > >> going to help. WBNOINVD is a pretty recent example.
> > > >>
> > > >
> > > > Since adding REP to random instructions may lead to different instructions,
> > > > RepPrefixOk is used to prevent that.  If one really wants different
> > > > instructions,
> > > > ".byte 0xf3" can be used.
> > >
> > > ... this you realize that breaking existing code is bad? It doesn't
> > > matter how "easy" it is to fix such. Taking Xen (again) as the example,
> > > older trees are supposed to not be touched anymore except for security
> > > fixes. Now if people upgrade their underlying distros, builds of these
> > > older trees will suddenly start to fail.
> > >
> > > Furthermore, with your ".byte 0xf3" suggestion, what "protection" do
> > > you achieve when disallowing "rep", but allowing ".byte 0xf3"? Plus
> > > personally I consider the .byte variant quite a bit worse.
> >
> > It will discourage using old assembler for new instructions.  Of course,
> > one can use ".byte" for anything.
> >
> > > Finally, with a number of changes of mine (including the still pending
> > > operand size default changes which I'm slowly making progress with)
> > > you've been demanding that the Linux build not be broken. But just
> > > like Xen, Linux too uses "rep; bsf".
> > >
> >
> > "rep; bsf" works fine since bsf has RepPrefixOk.  If REP should be
> > allowed for any instructions, shouldn't RepPrefixOk be removed?
> >
>
> RepPrefixOk was added by
>
> commit 29c048b696d4e93fe9f595d59fcb6239270e5a29
> Author: Roland McGrath <roland@gnu.org>
> Date:   Fri Jun 22 16:42:08 2012 +0000
>
>     gas/
>             * config/tc-i386.c (parse_insn): Don't complain about REP prefix
>             when the template has opcode_modifier.repprefixok set.
>             * NEWS: Mention the change.
>
>     gas/testsuite/
>             * gas/i386/rep-bsf.d: New file.
>             * gas/i386/rep-bsf.s: New file.
>             * gas/i386/i386.exp: Add the new test.
>
>     opcodes/
>             * i386-opc.h (RepPrefixOk): New enum constant.
>             (i386_opcode_modifier): New bitfield 'repprefixok'.
>             * i386-gen.c (opcode_modifiers): Add RepPrefixOk.
>             * i386-opc.tbl: Add RepPrefixOk to bsf, bsr, and to all
>             instructions that have IsString.
>             * i386-tbl.h: Regenerate.
>
> We can either remove RepPrefixOk or add it to any instructions
> which can have REP.
>

Gas tried to favor "REP INSN" over "REP; INSN":

https://sourceware.org/ml/binutils/2012-06/msg00180.html
https://sourceware.org/ml/binutils/2012-06/msg00193.html

We can add RepPrefixOk if needed.

-- 
H.J.