Re: V2: [PATCH] x32: Generate 0x67 prefix for VSIB address without base

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

On Tue, Feb 26, 2019 at 6:45 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.02.19 at 14:23, <hjl.tools@gmail.com> wrote:
> > On Tue, Feb 26, 2019 at 3:41 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >> >>> On 26.02.19 at 05:35, <hjl.tools@gmail.com> wrote:
> >> > In x32, add ADDR_PREFIX_OPCODE prefix for VSIB address without base
> >> > register so that vector index register will be zero-extended to 64 bits.
> >> >
> >> > We can't have ADDR_PREFIX_OPCODE prefix with 32-bit address if there is
> >> > segment override since address will be segment base + zero-extended to 64
> >> > bits of (base + index * scale + disp).  But GCC:
> >> >
> >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89502
> >>
> >> Neither above nor in the bug you explain what's wrong with the
> >> segment override plus address size override in x32 mode. Since you
> >
> > X32 relies on 0x67 prefix to zero-extend address to 64 bits:
> >
> > zero-extended (base + index * scale + disp)
> >
> > With segment override, we got
> >
> > segment base + zero-extended (base + index * scale + disp)
> >
> > instead of
> >
> > zero-extended (segment base + base + index * scale + disp)
> >
> > When base + index * scale + disp is negative, we get the wrong
> > address.
> >
> > VSIB address in vgatherdps is
> >
> > base + sign-extend(index) * scale + disp
> >
> > With segment override, we got
> >
> > segment base + zero-extended (base + sign-extend(index) * scale + disp)
>
> Right. But whether that's what the programmer wanted we don't
> know. Also please consider the qword index forms as well, plus
> the dword index forms with scaling factor 2, 4, or 8 (allowing for
> effective indexes up to 35 bits wide).
>
> All of this would be acceptable if address space was limited to 4Gb
> for x32, but that's not the case according to my reading of the
> chapter in the psABI.

10.4 Kernel Support
Kernel should limit stack and addresses returned from system calls
bewteen 0x00000000
to 0xf f f f f f f f .

> > 175.vpr in SPEC CPU 2000:
> >[...]
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x004158fd in try_place.isra ()
> > (gdb) disass 0x004158fd,+32
> > Dump of assembler code from 0x4158fd to 0x41591d:
> > => 0x004158fd <try_place.isra.5+7517>: vgatherdps %ymm2,0xc(,%ymm15,1),%ymm12
>
> Okay, this is the special case of the index register actually holding
> addresses. What about the case where the displacement is the base
> address, and the index register holds indeed indexes?

I will fix it.

> >> keep using the same wording with just slight alterations, it must be
> >> something very obvious to you, but entirely un-obvious to me. Is
> >> this related to the desire of using both negative and positive
> >> offsets into TLS, where (obviously I would say) there's not going
> >> to be any wrapping at the 4Gb boundary? If so, I'd say the TLS
> >
> > It won't wrap for x32.
> >
> >> usage model is broken, but it's not the assembler that should
> >
> > No, it is not.  Please read "ILP32 Programming Model" in x86-64 psABI.
>
> I trust you that you follow what is written there. The question
> though is whether it wasn't a mistake to permit negative offsets in
> the first place.

Negative offset is by design.

> >> prevent use of otherwise valid constructs. Whether full 64-bit
> >
> > Assembly is correct for 64-bit mode.  Since it doesn't work for
> > x32 when offset is negative, we should at least give a warning.
>
> Well, yes, since the ABI can't reasonably be changed, emitting a
> warning looks like the only option now. But as said, please don't tie
> this to that pre-existing one, not the least because that's also what

Existing code will get a warning.

> is going to control the lack-of-disambiguating-suffix diagnostic in
> AT&T mode the change for I hope to submit at some point over the
> next several months (now that I've mostly completed the prereqs
> you had set for this).
>
> >> addresses (and hence full non-zero %fs/%gs bases with no
> >> wrapping at the 4Gb boundary) is intended is the programmer's
> >> choice, not something the assembler should enforce unconditionally.
> >> Optionally emitting a warning is acceptable, but then this shouldn't
> >> be tied to any other, more generically applicable warnings.
> >
> > Binutils, including linker, does a few things special for x32 to deal
> > with address limitation.  This is just one of them.
>
> But are there pre-existing cases where in order to make one
> thing work a different thing got deliberately broken?

It works only if offset isn't negative.

> >> In any event, if this is to stay, then at least the code comment
> >> needs to be quite a bit more clear - "we can't have" is not enough
> >> without explicitly saying why that is.
> >>
> >> > --- a/gas/config/tc-i386.c
> >> > +++ b/gas/config/tc-i386.c
> >> > @@ -8141,6 +8141,36 @@ output_insn (void)
> >> >         i.prefix[LOCK_PREFIX] = 0;
> >> >       }
> >> >
> >> > +#if defined (OBJ_MAYBE_ELF) || defined (OBJ_ELF)
> >> > +      if (flag_code == CODE_64BIT && x86_elf_abi == X86_64_X32_ABI)
> >> > +     {
> >> > +       /* In x32, add ADDR_PREFIX_OPCODE prefix for VSIB address
> >> > +          without base register so that vector index register will
> >> > +          be zero-extended to 64 bits.  */
> >> > +       if (!i.base_reg && i.tm.opcode_modifier.vecsib)
> >> > +         add_prefix (ADDR_PREFIX_OPCODE);
> >>
> >> Just to re-state: There needs to be a way to override this behavior.
> >> And this is already leaving aside that making this the default from
> >> now on has a fair risk of breaking currently working code. (Note
> >> that this is not to say that I can't see that the change will also
> >> help currently broken code.)
> >
> > Please see above.  If VSIB index is below 2G, my fix doesn't change
> > anything.  If VSIB index is above 2G, the program crashes before my fix.
>
> Right, and I didn't put under question that you indeed fix one
> specific case. I just can't help thinking that you do so by breaking
> other cases, as per above. And I am of the opinion that it ought
> to be the compiler (or assembly programmer) who ought to
> explicitly request 32-bit addressing (e.g. by way of using the
> addr32 prefix) in this specific example of yours.
>
> >> > +       /* In x32, we can't have ADDR_PREFIX_OPCODE prefix with
> >> > +          segment override since final address will be segment
> >> > +          base + zero-extended (base + index * scale + disp).  */
> >> > +       if (operand_check != check_none
> >> > +           && i.prefix[ADDR_PREFIX]
> >> > +           && i.prefix[SEG_PREFIX])
> >> > +         {
> >> > +           const seg_entry *seg;
> >> > +           if (i.seg[0])
> >> > +             seg = i.seg[0];
> >> > +           else
> >> > +             seg = i.seg[1];
> >> > +           if (operand_check == check_error)
> >> > +             as_bad (_("can't encode segment `%s%s' with 32-bit address"),
> >
> > How about just
> >
> > segment `%s%s' override with 32-bit address
>
> That's slightly better text indeed.
>
> Jan
>


-- 
H.J.