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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

H.J. Lu-30
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.

PR gas/24821
* config/tc-i386.c (_i386_insn): Add prev.
(check_hle): Also check i.prev.name for prefix name.
(i386_md_end): New function.
(md_assemble): Apply the standalone prefix to the current
instruction.  Issue an error if a standalone prefix isn't
followed by an instruction in the same section.
(output_insn): Remember the standalone prefix and add it to the
following instruction.
* config/tc-i386.h (i386_md_end): New.
(md_end): Likewise.
* testsuite/gas/i386/i386.exp: Run prefix-2, prefix-3 and
x86-64-prefix-3.
* testsuite/gas/i386/ilp32/rex.d: Updated.
* testsuite/gas/i386/omit-lock-no.d: Likewise.
* testsuite/gas/i386/omit-lock-yes.d: Likewise.
* testsuite/gas/i386/rex.d: Likewise.
* testsuite/gas/i386/white.l: Likewise.
* testsuite/gas/i386/omit-lock.s: Replace standalone lock
with ds.
* testsuite/gas/i386/prefix-2.l: New file.
* testsuite/gas/i386/prefix-2.s: Likewise.
* testsuite/gas/i386/prefix-3.d: Likewise.
* testsuite/gas/i386/prefix-3.s: Likewise.
* testsuite/gas/i386/x86-64-prefix-3.d: Likewise.
* testsuite/gas/i386/rex.s: Add nop after REX prefixes.

--
H.J.

0001-x86-Apply-standalone-prefixes-to-the-following-instr.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Jan Beulich-2
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.

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;
}

(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.

Jan
Reply | Threaded
Open this post in threaded view
|

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

Florian Weimer-5
In reply to this post by H.J. Lu-30
* H. J. Lu:

> 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.

glibc used to do this:

| #define __arch_c_compare_and_exchange_val_8_acq(mem, newval, oldval) \
|   ({ __typeof (*mem) ret;                                                    \
|     __asm __volatile ("cmpl $0, %%fs:%P5\n\t"                                \
|                      "je 0f\n\t"                                             \
|                      "lock\n"                                                \
|                       "0:\tcmpxchgb %b2, %1"                                 \
|                       : "=a" (ret), "=m" (*mem)                              \
|                       : "q" (newval), "m" (*mem), "0" (oldval),              \
|                         "i" (offsetof (tcbhead_t, multiple_threads)));       \
|      ret; })

Will this still work?

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

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

H.J. Lu-30
In reply to this post by Jan Beulich-2
On Fri, Jul 19, 2019 at 1:46 AM Jan Beulich <[hidden email]> 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.

> 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.

--
H.J.
Reply | Threaded
Open this post in threaded view
|

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

Jan Beulich-2
On 19.07.2019 17:01,  H.J. Lu  wrote:

> On Fri, Jul 19, 2019 at 1:46 AM Jan Beulich <[hidden email]> 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.

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".

Jan
Reply | Threaded
Open this post in threaded view
|

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

H.J. Lu-30
On Fri, Jul 19, 2019 at 9:25 AM Jan Beulich <[hidden email]> wrote:

>
> On 19.07.2019 17:01,  H.J. Lu  wrote:
> > On Fri, Jul 19, 2019 at 1:46 AM Jan Beulich <[hidden email]> 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?

--
H.J.
Reply | Threaded
Open this post in threaded view
|

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

H.J. Lu-30
In reply to this post by Florian Weimer-5
On Fri, Jul 19, 2019 at 2:31 AM Florian Weimer <[hidden email]> wrote:

>
> * H. J. Lu:
>
> > 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.
>
> glibc used to do this:
>
> | #define __arch_c_compare_and_exchange_val_8_acq(mem, newval, oldval) \
> |   ({ __typeof (*mem) ret;                                                    \
> |     __asm __volatile ("cmpl $0, %%fs:%P5\n\t"                                \
> |                      "je 0f\n\t"                                             \
> |                      "lock\n"                                                \
> |                       "0:\tcmpxchgb %b2, %1"                                 \
> |                       : "=a" (ret), "=m" (*mem)                              \
> |                       : "q" (newval), "m" (*mem), "0" (oldval),              \
> |                         "i" (offsetof (tcbhead_t, multiple_threads)));       \
> |      ret; })
>
> Will this still work?
>
":" between prefixes and instruction is a special case.   Here is the updated
patch to add i386_frob_colon so that i386 can output all pending stand-alone
prefixes when seeing a colon in the same frag.

--
H.J.

0001-x86-Apply-standalone-prefixes-to-the-following-instr.patch (28K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

H.J. Lu-30
In reply to this post by H.J. Lu-30
On Fri, Jul 19, 2019 at 10:11 AM H.J. Lu <[hidden email]> wrote:

>
> On Fri, Jul 19, 2019 at 9:25 AM Jan Beulich <[hidden email]> wrote:
> >
> > On 19.07.2019 17:01,  H.J. Lu  wrote:
> > > On Fri, Jul 19, 2019 at 1:46 AM Jan Beulich <[hidden email]> 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 <[hidden email]>
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.

--
H.J.
Reply | Threaded
Open this post in threaded view
|

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

H.J. Lu-30
On Fri, Jul 19, 2019 at 10:23 AM H.J. Lu <[hidden email]> wrote:

>
> On Fri, Jul 19, 2019 at 10:11 AM H.J. Lu <[hidden email]> wrote:
> >
> > On Fri, Jul 19, 2019 at 9:25 AM Jan Beulich <[hidden email]> wrote:
> > >
> > > On 19.07.2019 17:01,  H.J. Lu  wrote:
> > > > On Fri, Jul 19, 2019 at 1:46 AM Jan Beulich <[hidden email]> 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 <[hidden email]>
> 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.
Reply | Threaded
Open this post in threaded view
|

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

Jan Beulich-2
On 19.07.2019 19:37,  H.J. Lu  wrote:
> 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.

And this is the whole point: The form without line separator
should be accepted for templates with RepPrefixOk only. The
one with line separator should (by default) not complain.
I don't mind you adding an option to this effect, but it
ought to be off by default.

As to BSF/BSR having RepPrefixOk - this is exactly what I've
been mentioning before: Recognizing the desire to have it
can't possibly happen early enough. At the point the assembler
supports such insns with the extra attribute, it'll quite
obviously also accept the real mnemonics (TZCNT/LZCNT in this
case). WBNOINVD is a pretty good recent example, as mentioned
before (and I think I did point out that in Xen we already use
"rep; wbinvd" to generate that insn).

IMO BSF/BSR should never have had the attribute added to them.

Jan