Home Original page

[PATCH] x86: Disallow GOT memory as vector memory operand

H.J. Lu hjl.tools@gmail.com
Thu Feb 6 03:18:30 GMT 2025 
On Wed, Feb 5, 2025 at 4:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.02.2025 23:19, H.J. Lu wrote:
> > GOT memory is 4-byte for i386 and 8-byte for x86-64.  Disallow GOT memory
> > as vector memory operand.
>
> This is going too far and not far enough at the same time.
>
> Too far: Assembly programmers ought to be permitted to do whatever
> they like. There may be an advanced mode where we warn about things
> we deem bogus.

Assembler doesn't allow a word memory operand when instruction
requires a 128-bit memory operand.

> Not far enough: Why would non-vector operands of the wrong size be
> okay? (Correcting this can then easily go too far, though: If
> one is after a specific property of a GOT entry, TEST or CMP may,
> for example, be used on just the high part [of whatever size] of
> it.)

Smaller than GOT slot size should be OK.

> Overall: This is yet another arbitrary heuristic you're introducing.
> I did complain about you doing such already in the past. Please
> don't. Either there are firm, psABI-mandated criteria, or there
> aren't. As said above, a default-off optional check may be okay.
>
> Furthermore, why would you check the template for dword/qword?

Some vector instructions take dword/qword and vector memory.

> Shouldn't this be the actual operand that we're processing? While

It sounds good.  But it is more complex than I like.

> doing so has (afaict, without actually trying it out) the advantage
> of still permitting broadcasts of the right size, imo that needs
> dealing with separately.
>
> > PR gas/32624
> > * config/tc-i386.c (output_disp): Disallow GOT memory as vector
> > memory operand.
> > * testsuite/gas/i386/i386.exp: Run inval-got.
> > * testsuite/gas/i386/inval-got.l: New file.
> > * testsuite/gas/i386/inval-got.s: Likewise.
> > * testsuite/gas/i386/x86-64-inval-got.l: Likewise.
> > * testsuite/gas/i386/x86-64-inval-got.l: Likewise.
> > * testsuite/gas/i386/x86-64.exp: Run x86-64-inval-got.
>
> All you test is VPERMB. Imo for a code change like this, testing
> needs to cover quite a few more insns / variants. Since it was
> just the other day that you demanded that I add testcases to

Good point.  I will add some more tests.

FWIW, you provided some patches without ANY tests whatsoever.

> patches, here's my (general) request: Testcases ought to be
> providing good coverage for what needs testing. It is often not
> sufficient to merely test the one special case that was pointed
> out somewhere.
>
> Jan



-- 
H.J.
More information about the Binutils mailing list