Bug 30856 - Regression: operand size mismatch for `push'
Summary: Regression: operand size mismatch for `push'
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.41
: P2 normal
Target Milestone: ---
Assignee: Jan Beulich
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-15 00:58 UTC by Antoni Boucher
Modified: 2023-09-27 08:58 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2023-09-21 00:00:00


Attachments
Reproducer for the bug (142 bytes, text/x-csrc)
2023-09-15 00:58 UTC, Antoni Boucher
Details
ASM reproducer for the bug (410 bytes, text/plain)
2023-09-18 11:42 UTC, Antoni Boucher
Details
Working example with ATT syntax (372 bytes, text/plain)
2023-09-21 13:09 UTC, Antoni Boucher
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antoni Boucher 2023-09-15 00:58:15 UTC
Created attachment 15111 [details]
Reproducer for the bug

Hi.
When compiling the attached reproducer with gcc with the flags -fno-pie -no-pie -masm=intel, gas will show the following error:

    operand size mismatch for `push'

seemingly unable to understand the following instruction:

    push OFFSET FLAT:a

This works on version 2.40.

Thanks to fix this issue.
Comment 1 Nick Clifton 2023-09-18 10:57:34 UTC
(In reply to Antoni Boucher from comment #0)
Hi Antoni,

> Reproducer for the bug

Please could you upload the assembler output from the compiler ?

It is likely that this bug is dependent upon the version of the compiler
that is being used, so in order to reproduce and investigate the problem
we really need the assembler output, rather than the C source file.

It may also be that this problem is actually a bug in the compiler
which was not being caught by the 2.40 assembler, but which is now detected
by the 2.41 assembler.  Hence having access to the assembler source file
should allow us to find out if this is true.

Cheers
  Nick
Comment 2 Antoni Boucher 2023-09-18 11:42:27 UTC
Created attachment 15113 [details]
ASM reproducer for the bug

Here it is. You simply need to run GNU as on this file to trigger the error.
Comment 3 Jan Beulich 2023-09-21 13:01:21 UTC
I'll look into why this happens, but:

1) I'm not sure we do ourselves (and our users) much good if we allow this again. The referenced symbol is severely constrained in where in the address space it may live. Nevertheless similar symbol uses (with e.g. MOV or ADD) still work, so it is likely necessary to fix this at least for consistency reasons, and for --x32.

2) I'm not convinced the compiler should actually emit such code. It can't know where the linker script (which may be a custom one) would put the executable. When put outside of the low 2G of address space and outside of the high 2G of address space, linking would fail from what I can tell. The emitted code looks to be legitimate only when -mcmodel=kernel is in effect, or in --x32 mode.
Comment 4 Antoni Boucher 2023-09-21 13:09:18 UTC
Created attachment 15119 [details]
Working example with ATT syntax

Yes, this is useful for -mcmodel=kernel and the code continues to work when not using the Intel asm syntax, i.e. there's a difference in behavior with the ATT syntax.

I attached the ATT version (produced by gcc) that still works.
Comment 5 Jan Beulich 2023-09-21 14:38:44 UTC
(In reply to Antoni Boucher from comment #4)
> I attached the ATT version (produced by gcc) that still works.

Well, only partly: PUSHQ works, but PUSH (no suffix) doesn't according to my testing.
Comment 6 Antoni Boucher 2023-09-21 22:53:17 UTC
(In reply to Jan Beulich from comment #5)
> Well, only partly: PUSHQ works, but PUSH (no suffix) doesn't according to my
> testing.

Do you mean that gcc produces invalid asm when using the Intel syntax and should be using pushq?
I was under the impression that suffixes like q was ATT syntax, but I couldn't find any reference of the intel syntax allowed in GNU as. It does seem to allow pushq even for the Intel syntax, though.

Other assemblers allow "push qword".
Comment 7 Jan Beulich 2023-09-22 06:21:09 UTC
(In reply to Antoni Boucher from comment #6)
> Do you mean that gcc produces invalid asm when using the Intel syntax and
> should be using pushq?

No. "push offset ..." is the correct form in Intel syntax. What I'm saying is that it is wrong to emit such code without -mcmodel=kernel, for there not being any guarantee that the resulting code will actually link. (At the same time an assembly programmer may write such code, knowing the target environment.)

> I was under the impression that suffixes like q was ATT syntax, but I
> couldn't find any reference of the intel syntax allowed in GNU as. It does
> seem to allow pushq even for the Intel syntax, though.

It does, but more by mistake than deliberately.

> Other assemblers allow "push qword".

That's a different form of push, with a memory operand (here we're talking about immediate / address operands).
Comment 9 Sourceware Commits 2023-09-27 08:54:45 UTC
The master branch has been updated by Jan Beulich <jbeulich@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=fb1c10585ead9acc8d9f9d24ab093cbe5e962257

commit fb1c10585ead9acc8d9f9d24ab093cbe5e962257
Author: Jan Beulich <jbeulich@suse.com>
Date:   Wed Sep 27 10:53:38 2023 +0200

    x86-64: fix suffix-less PUSH of symbol address
    
    PR gas/30856
    
    In 5cc007751cdb ("x86: further adjust extend-to-32bit-address
    conditions") I neglected the case of PUSH, which is the only insn
    allowing (proper) symbol addresses to be used as immediates (not
    displacements, like CALL/JMP) in the absence of any register operands.
    Since it defaults to 64-bit operand size, guessing an L suffix is wrong
    there.
Comment 10 Sourceware Commits 2023-09-27 08:57:27 UTC
The binutils-2_41-branch branch has been updated by Jan Beulich <jbeulich@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7fe76f02413fff61566ae52ec80d581da1e264a2

commit 7fe76f02413fff61566ae52ec80d581da1e264a2
Author: Jan Beulich <jbeulich@suse.com>
Date:   Wed Sep 27 10:53:38 2023 +0200

    x86-64: fix suffix-less PUSH of symbol address
    
    PR gas/30856
    
    In 5cc007751cdb ("x86: further adjust extend-to-32bit-address
    conditions") I neglected the case of PUSH, which is the only insn
    allowing (proper) symbol addresses to be used as immediates (not
    displacements, like CALL/JMP) in the absence of any register operands.
    Since it defaults to 64-bit operand size, guessing an L suffix is wrong
    there.
Comment 11 Jan Beulich 2023-09-27 08:58:45 UTC
Sorted on master and 2.41.