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

["Jan Beulich" <JBeulich at suse dot com>]

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

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

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

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

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