[PATCH] i386: Add .code16gcc fldenv tests

[H.J. Lu]

On Thu, Oct 31, 2019 at 10:26 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Oct 31, 2019 at 2:24 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 31.10.2019 00:57, H.J. Lu wrote:
> > > On Wed, Oct 30, 2019 at 12:59 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>
> > >> On 29.10.2019 18:55,  H.J. Lu  wrote:
> > >>> On Mon, Oct 28, 2019 at 1:05 AM Jan Beulich <jbeulich@suse.com> wrote:
> > >>>>
> > >>>> Commit b76bc5d54e ("x86: don't default variable shift count insns to
> > >>>> 8-bit operand size") pointed out a very bad case, but the underlying
> > >>>> problem is, as mentioned on various occasions, much larger: Silently
> > >>>> selecting a (nowhere documented afaict) certain default operand size
> > >>>> when there's no "sizing" suffix and no suitable register operand(s) is
> > >>>> simply dangerous (for the programmer to make mistakes).
> > >>>>
> > >>>> While in Intel syntax mode such mistakes already lead to an error (which
> > >>>> is going to remain that way), AT&T syntax mode now gains warnings in
> > >>>> such cases by default, which can be suppressed or promoted to an error
> > >>>> if so desired by the programmer. Furthermore at least general purpose
> > >>>> insns now consistently have a default applied (alongside the warning
> > >>>> emission), rather than accepting some and refusing others.
> > >>>>
> > >>>> No warnings are (as before) to be generated for "DefaultSize" insns as
> > >>>> well as ones acting on selector and other fixed-width values. The set of
> > >>>> "DefaultSize" ones gets slightly widened for the purposes here.
> > >>>
> > >>> What is the advantage to add DefaultSize vs the alternative?
> > >>
> > >> I don't know what alternative you refer to; if you mean some
> > >> hypothetical one, then the advantage of simply adding
> > >> DefaultSize as done here is likely that it allows to not add or
> > >> further complicate logic in tc-i386*.c. Furthermore the ones which
> > >> get the attribute added should have had it already before, if the
> > >> comment "default insn size depends on mode" is to be trusted.
> > >>
> > >
> > > DefaultSize is added to some instructions and then they are excluded:
> > >
> > > +          /* exclude jmp/ljmp */
> > > +          && strcmp (i.tm.name, "jmp") && strcmp (i.tm.name, "ljmp")
> > > +          /* exclude byte-displacement jumps */
> > > +          && !i.tm.opcode_modifier.jumpbyte
> > > +          /* exclude lgdt/lidt/sgdt/sidt */
> > > +          && (i.tm.base_opcode != 0x0f01 || i.tm.extension_opcode > 3)
> > >            /* exclude fldenv/frstor/fsave/fstenv */
> > >            && i.tm.opcode_modifier.no_ssuf)
> > >
> > > It looks odd.
> >
> > But this isn't the only place where defaultsize gets evaluated.
> > See how lgdt/lidt/sgdt/sidt already have the attribute in the
> > opcode table, but need exclusion here now too. The alternative
> > would be two independent attributes - one to be evaluated here,
> > and the other to be evaluated further down in the function. Yet
> > again - this dual use has been there before, and just needs
> > suitable extending of the logic now.
> >
>
> Normally instructions with DefaultSize have i.suffix unset.  Except with
> .code16gcc, which is used to support 16-bit mode with 32-bit address,
> i.suffix is set to 'l' for 32-bit address.   However,
> iret/fldenv/frstor/fsave/fstenv
> are exceptions since they need 16-bit variants.  So we need 2 different
> DefaultSize behaviors for .code16gcc, one uses LONG_MNEM_SUFFIX
> and the other uses WORD_MNEM_SUFFIX.  We should update
> DefaultSize to properly encode iret/fldenv/frstor/fsave/fstenv for
> .code16gcc, instead of checking i.tm.opcode_modifier.no_ssuf.
> Something like this.
>

I checked in this patch to add .code16gcc fldenv tests.


-- 
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-i386-Add-.code16gcc-fldenv-tests.patch
Type: application/x-patch
Size: 2163 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20191031/09504dbf/attachment.bin>