Bug 25012 - pushq/popq %gs/%fs in .code64 now unsupported
Summary: pushq/popq %gs/%fs in .code64 now unsupported
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.33
: P2 normal
Target Milestone: 2.34
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-18 15:30 UTC by Christian Ehrhardt
Modified: 2019-10-08 10:52 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2019-09-18 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Ehrhardt 2019-09-18 15:30:24 UTC
Hi,
I have found that with recent binutils ipxe was no more compiling.
It broke on a section doing essentially (simplified) doing:

.code64
pushq %gs
pushq %fs
popq %gs
popq %fs

The fail I got was like:
push.S:2: Error: unsupported instruction `push'
push.S:3: Error: unsupported instruction `push'
push.S:4: Error: unsupported instruction `pop'
push.S:5: Error: unsupported instruction `pop'

There is more about that in Ubuntu bug https://bugs.launchpad.net/ubuntu/+source/binutils/+bug/1843394

But I'm reaching out to you here as I was wondering if it is a bug in gas (unlikely, but it is a change in behavior for sure).

I found that even older 2.32 are ok.
2.32-7ubuntu4 - ok
2.32.51.20190905-0ubuntu1 - fails.

I was wondering and hoping you could help me on three things:
1. was the change to no more build that assembly intentional and could one maybe link to a commit for that?
2. if the change was intentional is there a best practice how the code should be modified to work again?
3. I was checking the behavior on push/pop %fs/gs and it confused me so I wonder if the current state is correct (see attachment and below).

For #3 some details:
Summary D vs E:
- no suffix
  => works equally in both releases
  => same opcodes in all .code segments
- suffix "w"
  => works equally in both releases
  => opcodes in .code32/.code64 differ from .code16 (660f..)
  => .code16 matches the non-suffix opcodes (0f..)
- suffix "l"
  => failures in Disco, works in Eoan
  => .code16 opcodes match the non-.code16 of the "w" suffix (660f..)
  => .code32/.code64 are back to the base opcode (0f..)
  => If I remove the failing .code64 from disco then .code16/.code32 is the same as in Eoan
- suffix "q"
  => different failures in Disco and Eoan
  => in Disco .code16/.code32 fails
  => in Disco .code64 generates the basic opcode (0f..)
  => in Eoan all three .code segments fail
I wonder if all of that is correct?

P.S. I can break this up into multiple bugs if you tell me which section you want to discuss separately.
P.P.S. I have asked upstream IPXE for feedback for (a pure gut feeling) change, but there was no response yet (http://lists.ipxe.org/pipermail/ipxe-devel/2019-September/006763.html)
Comment 1 H.J. Lu 2019-09-18 18:33:29 UTC
It is caused by

commit 21df382b918888de64749e977f185c4e10a5b838
Author: Jan Beulich <jbeulich@suse.com>
Date:   Tue Jul 16 09:30:29 2019 +0200

    x86: fold SReg{2,3}
    
    They're the only exception to there generally being no mix of register
    kinds possible in an insn operand template, and there being two bits per
    operand for their representation is also quite wasteful, considering the
    low number of uses.  Fold both bits and deal with the little bit of
    fallout.
    
    Also take the liberty and drop dead code trying to set REX_B: No segment
    register has RegRex set on it.
    
    Additionally I was quite surprised that PUSH/POP with the permitted
    segment registers is not covered by the test cases.  Add the missing
    pieces.

Assembler disallows "popq %fs", but disassembler still display "popq %fs".
Comment 2 Christian Ehrhardt 2019-09-19 06:16:43 UTC
Thanks H.J. Lu,
so the change was intentional just as I thought. I'll add this to the thread [1] I have with the ipxe people then.

If there is anyone here that wants to help guiding [1] please feel free to chime in there or let me know here and I can pass the info.

[1]: http://lists.ipxe.org/pipermail/ipxe-devel/2019-September/006763.html
Comment 3 Jan Beulich 2019-09-19 10:48:13 UTC
Hmm, yes - No_qSuf gets in the way here. Simply removing the attribute won't work though, I'm afraid. I'll try to find time soon to look into this. (And I'm surprised there's nothing in the testsuite that would have allowed me to notice this before even submitting the patch.)
Comment 4 H.J. Lu 2019-10-04 15:08:17 UTC
Fixed for 2.34 by

commit 3f9aad111cea2f25877d0a6b404956769c14faee
Author: Jan Beulich <jbeulich@suse.com>
Date:   Fri Sep 20 10:18:15 2019 +0200

    x86-64: fix handling of PUSH/POP of segment register
    
    Commit 21df382b91 ("x86: fold SReg{2,3}") went too far: Folding 64-bit
    PUSH/POP templates into non-64-bit ones isn't correct, due to the
    different operand widths, and hence suffixes permitted. Restore the
    separate templates.
    
    Add tests of PUSH/POP with q suffix and %fs/%gs operands to the
    testsuite. While doing so also add PUSHF/POPF ones _without_ suffix.

and on 2.33 branch by

commit 20057c8c5e67ffdfb1a7b6a4ef3d337ea27663d1
Author: Jan Beulich <jbeulich@suse.com>
Date:   Fri Sep 20 10:18:15 2019 +0200

    x86-64: fix handling of PUSH/POP of segment register
    
    Commit 21df382b91 ("x86: fold SReg{2,3}") went too far: Folding 64-bit
    PUSH/POP templates into non-64-bit ones isn't correct, due to the
    different operand widths, and hence suffixes permitted. Restore the
    separate templates.
    
    Add tests of PUSH/POP with q suffix and %fs/%gs operands to the
    testsuite. While doing so also add PUSHF/POPF ones _without_ suffix.
    
    (cherry picked from commit 3f9aad111cea2f25877d0a6b404956769c14faee)
Comment 5 Christian Ehrhardt 2019-10-08 10:52:53 UTC
Thank you Jan!