The x86-64 disassembler sometimes prints the REX prefix as BYTE #X48

Bug #1085729 reported by Lutz Euler
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
SBCL
Fix Released
Low
Unassigned

Bug Description

Executing the following code

(let ((sb-c::*compiler-trace-output* *standard-output*))
  (compile nil `(lambda (x)
                   (declare (type word x) (optimize speed))
                   (values (floor x -8)))))

outputs as disassembly

; BD: 48892B MOV [RBX], RBP
; C0: 48 BYTE #X48
; C1: 8BEB MOV RBP, RBX

where one would expect

; BD: 48892B MOV [RBX], RBP
; C0: 488BEB MOV RBP, RBX

Revision history for this message
Lutz Euler (lutz-euler) wrote :

Bisected to

65bdee4ba534e82c352cff3eec16473daaf285dd is the first bad commit
commit 65bdee4ba534e82c352cff3eec16473daaf285dd
Author: Lutz Euler <email address hidden>
Date: Wed Dec 14 18:11:53 2011 +0100

    Improve handling of x86[-64] prefix instructions in the disassembler.

Revision history for this message
Lutz Euler (lutz-euler) wrote :

With a function definition

(defun f (x)
  (declare (type word x) (optimize speed))
  (values (floor x -8)))

both (DISASSEMBLE 'F) or (SB-DISASSEM::DISASSEMBLE-CODE-COMPONENT #'F) show the correct disassembly.
Replace F here with the (COMPILE NIL ...) expression from the bug description and both calls again show the correct disassembly.

So it may be that the bug occurs only when using *COMPILER-TRACE-OUTPUT*.

Lutz Euler (lutz-euler)
Changed in sbcl:
status: New → In Progress
assignee: nobody → Lutz Euler (lutz-euler)
importance: Undecided → Low
Revision history for this message
Lutz Euler (lutz-euler) wrote :

I attach a patch that fixes the issue.

I currently don't want to commit it because I am not yet decided whether the disassembler code could not be cleaned up in this area as described below which would allow for a simpler fix, too.

The cleanup would be to simplify disassemble-assem-segment, assem-segment-to-disassem-segments and add-block-segments by simply copying the complete assem-segment into one continuous vector and disassembling that. This would allow to drop all the machinery that now is in place to find instruction boundaries because add-block-segments seems or maybe even needs to assume that fillers could be anywhere inside instructions.

I don't currently understand whether this simplification would break something or might be undesirable.

Changed in sbcl:
status: In Progress → Confirmed
assignee: Lutz Euler (lutz-euler) → nobody
Revision history for this message
Lutz Euler (lutz-euler) wrote :

The attached patch implements the version with the simplified
disassembler as hinted at in comment #3.

I don't want to commit that currently, either, as there is an old
KLUDGE comment at ON-SEGMENTS-CONTENTS-VECTORLY
saying that it might be possible to move the segment compaction
to FINALIZE-SEGMENT.

I want to look into that first as it would supersede this patch.

Lutz Euler (lutz-euler)
Changed in sbcl:
assignee: nobody → Lutz Euler (lutz-euler)
status: Confirmed → In Progress
Revision history for this message
Lutz Euler (lutz-euler) wrote :

The attached patch implements a version that fixes the KLUDGE mentioned in comment #4. The segment compaction is now done in FINALIZE-SEGMENT. This allows for the same simplifications in the disassembler as done in the patch in comment #4.

Revision history for this message
Lutz Euler (lutz-euler) wrote :

I just committed the patch from #5 as 51bc001b7a98af096af782a672389e51004af068.

Changed in sbcl:
status: In Progress → Fix Committed
assignee: Lutz Euler (lutz-euler) → nobody
Changed in sbcl:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.