Bug 21446 - Incorrect disassembly for msr to dbgdtrtx_el0 in AArch64
Summary: Incorrect disassembly for msr to dbgdtrtx_el0 in AArch64
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.28
: P2 normal
Target Milestone: 2.31
Assignee: Tamar Christina
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-28 17:44 UTC by Chris
Modified: 2018-06-07 10:56 UTC (History)
1 user (show)

See Also:
Host:
Target: aarch64-*
Build:
Last reconfirmed: 2018-04-18 00:00:00


Attachments
Test case (55 bytes, text/plain)
2017-04-28 17:44 UTC, Chris
Details
Pre-assembled object file (172 bytes, application/octet-stream)
2017-04-28 17:46 UTC, Chris
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris 2017-04-28 17:44:30 UTC
Created attachment 10032 [details]
Test case

The AArch64 system registers dbgdtrrx_el0 and dbgdtrtx_el0 use the same encoding, but the former is only used for MRS instructions and the latter for MSR.

Assembling the following code:
.text
            msr     dbgdtrtx_el0, x3
            mrs     x3, dbgdtrrx_el0

And then disassembling with objdump gives the incorrect register name for the MSR

0000000000000000 <.text>:
   0:   d5130503        msr     dbgdtrrx_el0, x3
   4:   d5330503        mrs     x3, dbgdtrrx_el0

The resulting binary is, of course, fine, but the disassembly output is off for the MSR.
Comment 1 Chris 2017-04-28 17:46:45 UTC
Created attachment 10033 [details]
Pre-assembled object file
Comment 2 Sourceware Commits 2018-05-15 16:21:59 UTC
The master branch has been updated by Tamar Christina <tnfchris@sourceware.org>:

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

commit 561a72d4ddf825ffaf8e88551e9bd6707cd6c59f
Author: Tamar Christina <tamar.christina@arm.com>
Date:   Tue May 15 16:11:42 2018 +0100

    Modify AArch64 Assembly and disassembly functions to be able to fail and report why.
    
    This patch if the first patch in a series to add the ability to add constraints
    to system registers that an instruction must adhere to in order for the register
    to be usable with that instruction.
    
    These constraints can also be used to disambiguate between registers with the
    same encoding during disassembly.
    
    This patch adds a new flags entry in the sysreg structures and ensures it is
    filled in and read out during assembly/disassembly. It also adds the ability for
    the assemble and disassemble functions to be able to gracefully fail and re-use
    the existing error reporting infrastructure.
    
    The return type of these functions are changed to a boolean to denote success or
    failure and the error structure is passed around to them. This requires
    aarch64-gen changes so a lot of the changes here are just mechanical.
    
    gas/
    
    	PR binutils/21446
    	* config/tc-aarch64.c (parse_sys_reg): Return register flags.
    	(parse_operands): Fill in register flags.
    
    gdb/
    
    	PR binutils/21446
    	* aarch64-tdep.c (aarch64_analyze_prologue,
    	aarch64_software_single_step, aarch64_displaced_step_copy_insn):
    	Indicate not interested in errors.
    
    include/
    
    	PR binutils/21446
    	* opcode/aarch64.h (aarch64_opnd_info): Change sysreg to struct.
    	(aarch64_decode_insn): Accept error struct.
    
    opcodes/
    
    	PR binutils/21446
    	* aarch64-asm.h (aarch64_insert_operand, aarch64_##x): Return boolean
    	and take error struct.
    	* aarch64-asm.c (aarch64_ext_regno, aarch64_ins_reglane,
    	aarch64_ins_reglist, aarch64_ins_ldst_reglist,
    	aarch64_ins_ldst_reglist_r, aarch64_ins_ldst_elemlist,
    	aarch64_ins_advsimd_imm_shift, aarch64_ins_imm, aarch64_ins_imm_half,
    	aarch64_ins_advsimd_imm_modified, aarch64_ins_fpimm,
    	aarch64_ins_imm_rotate1, aarch64_ins_imm_rotate2, aarch64_ins_fbits,
    	aarch64_ins_aimm, aarch64_ins_limm_1, aarch64_ins_limm,
    	aarch64_ins_inv_limm, aarch64_ins_ft, aarch64_ins_addr_simple,
    	aarch64_ins_addr_regoff, aarch64_ins_addr_offset, aarch64_ins_addr_simm,
    	aarch64_ins_addr_simm10, aarch64_ins_addr_uimm12,
    	aarch64_ins_simd_addr_post, aarch64_ins_cond, aarch64_ins_sysreg,
    	aarch64_ins_pstatefield, aarch64_ins_sysins_op, aarch64_ins_barrier,
    	aarch64_ins_prfop, aarch64_ins_hint, aarch64_ins_reg_extended,
    	aarch64_ins_reg_shifted, aarch64_ins_sve_addr_ri_s4xvl,
    	aarch64_ins_sve_addr_ri_s6xvl, aarch64_ins_sve_addr_ri_s9xvl,
    	aarch64_ins_sve_addr_ri_s4, aarch64_ins_sve_addr_ri_u6,
    	aarch64_ins_sve_addr_rr_lsl, aarch64_ins_sve_addr_rz_xtw,
    	aarch64_ins_sve_addr_zi_u5, aarch64_ext_sve_addr_zz,
    	aarch64_ins_sve_addr_zz_lsl, aarch64_ins_sve_addr_zz_sxtw,
    	aarch64_ins_sve_addr_zz_uxtw, aarch64_ins_sve_aimm,
    	aarch64_ins_sve_asimm, aarch64_ins_sve_index, aarch64_ins_sve_limm_mov,
    	aarch64_ins_sve_quad_index, aarch64_ins_sve_reglist,
    	aarch64_ins_sve_scale, aarch64_ins_sve_shlimm, aarch64_ins_sve_shrimm,
    	aarch64_ins_sve_float_half_one, aarch64_ins_sve_float_half_two,
    	aarch64_ins_sve_float_zero_one, aarch64_opcode_encode): Likewise.
    	* aarch64-dis.h (aarch64_extract_operand, aarch64_##x): Likewise.
    	* aarch64-dis.c (aarch64_ext_regno, aarch64_ext_reglane,
    	aarch64_ext_reglist, aarch64_ext_ldst_reglist,
    	aarch64_ext_ldst_reglist_r, aarch64_ext_ldst_elemlist,
    	aarch64_ext_advsimd_imm_shift, aarch64_ext_imm, aarch64_ext_imm_half,
    	aarch64_ext_advsimd_imm_modified, aarch64_ext_fpimm,
    	aarch64_ext_imm_rotate1, aarch64_ext_imm_rotate2, aarch64_ext_fbits,
    	aarch64_ext_aimm, aarch64_ext_limm_1, aarch64_ext_limm, decode_limm,
    	aarch64_ext_inv_limm, aarch64_ext_ft, aarch64_ext_addr_simple,
    	aarch64_ext_addr_regoff, aarch64_ext_addr_offset, aarch64_ext_addr_simm,
    	aarch64_ext_addr_simm10, aarch64_ext_addr_uimm12,
    	aarch64_ext_simd_addr_post, aarch64_ext_cond, aarch64_ext_sysreg,
    	aarch64_ext_pstatefield, aarch64_ext_sysins_op, aarch64_ext_barrier,
    	aarch64_ext_prfop, aarch64_ext_hint, aarch64_ext_reg_extended,
    	aarch64_ext_reg_shifted, aarch64_ext_sve_addr_ri_s4xvl,
    	aarch64_ext_sve_addr_ri_s6xvl, aarch64_ext_sve_addr_ri_s9xvl,
    	aarch64_ext_sve_addr_ri_s4, aarch64_ext_sve_addr_ri_u6,
    	aarch64_ext_sve_addr_rr_lsl, aarch64_ext_sve_addr_rz_xtw,
    	aarch64_ext_sve_addr_zi_u5, aarch64_ext_sve_addr_zz,
    	aarch64_ext_sve_addr_zz_lsl, aarch64_ext_sve_addr_zz_sxtw,
    	aarch64_ext_sve_addr_zz_uxtw, aarch64_ext_sve_aimm,
    	aarch64_ext_sve_asimm, aarch64_ext_sve_index, aarch64_ext_sve_limm_mov,
    	aarch64_ext_sve_quad_index, aarch64_ext_sve_reglist,
    	aarch64_ext_sve_scale, aarch64_ext_sve_shlimm, aarch64_ext_sve_shrimm,
    	aarch64_ext_sve_float_half_one, aarch64_ext_sve_float_half_two,
    	aarch64_ext_sve_float_zero_one, aarch64_opcode_decode): Likewise.
    	(determine_disassembling_preference, aarch64_decode_insn,
    	print_insn_aarch64_word, print_insn_data): Take errors struct.
    	(print_insn_aarch64): Use errors.
    	* aarch64-asm-2.c: Regenerate.
    	* aarch64-dis-2.c: Regenerate.
    	* aarch64-gen.c (print_operand_inserter): Use errors and change type to
    	boolean in aarch64_insert_operan.
    	(print_operand_extractor): Likewise.
    	* aarch64-opc.c (aarch64_print_operand): Use sysreg struct.
Comment 3 Sourceware Commits 2018-05-15 16:22:08 UTC
The master branch has been updated by Tamar Christina <tnfchris@sourceware.org>:

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

commit 7d02540ab73206249779ced77a6abe0be156442e
Author: Tamar Christina <tamar.christina@arm.com>
Date:   Tue May 15 16:34:54 2018 +0100

    Allow non-fatal errors to be emitted and for disassembly notes be placed on AArch64
    
    This patch adds a new platform option "notes" that can be used to indicate if
    disassembly notes should be placed in the disassembly as comments.
    
    These notes can contain information about a failing constraint such as reading
    from a write-only register.  The disassembly will not be blocked because of this
    but -M notes will emit a comment saying that the operation is not allowed.
    
    For assembly this patch adds a new non-fatal status for errors.  This is
    essentially a warning.  The reason for not creating an actual warning type is
    that this causes the interaction between the ordering of warnings and errors to
    be problematic.  Currently the error buffer is almost always filled because of
    the way operands are matched during assembly. An earlier template may have put
    an error there that would only be displayed if no other template matches or
    generates a higher priority error.  But by definition a warning is lower
    priority than a warning, so the error (which is incorrect if another template
    matched) will supersede the warning.  By treating warnings as errors and only
    later relaxing the severity this relationship keeps working and the existing
    reporting infrastructure can be re-used.
    
    binutils/
    
    	PR binutils/21446
    	* doc/binutils.texi (-M): Document AArch64 options.
    	* NEWS: Document notes and warnings.
    
    gas/
    
    	PR binutils/21446
    	* config/tc-aarch64.c (print_operands): Indicate no notes.
    	(output_operand_error_record): Support non-fatal errors.
    	(output_operand_error_report, warn_unpredictable_ldst, md_assemble):
    	Likewise.
    
    include/
    
    	PR binutils/21446
    	* opcode/aarch64.h (aarch64_operand_error): Add non_fatal.
    	(aarch64_print_operand): Support notes.
    
    opcodes/
    
    	PR binutils/21446
    	* aarch64-dis.c (no_notes: New.
    	(parse_aarch64_dis_option): Support notes.
    	(aarch64_decode_insn, print_operands): Likewise.
    	(print_aarch64_disassembler_options): Document notes.
    	* aarch64-opc.c (aarch64_print_operand): Support notes.
Comment 4 Sourceware Commits 2018-05-15 16:22:13 UTC
The master branch has been updated by Tamar Christina <tnfchris@sourceware.org>:

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

commit f9830ec1655e7cc2aa88c9c34a20503978d9dc88
Author: Tamar Christina <tamar.christina@arm.com>
Date:   Tue May 15 16:37:20 2018 +0100

    Implement Read/Write constraints on system registers on AArch64
    
    This patch adds constraints for read and write only system registers with the
    msr and mrs instructions.  The code will treat having both flags set and none
    set as the same.  These flags add constraints that must be matched up. e.g. a
    system register with a READ only flag set, can only be used with mrs.  If The
    constraint fails a warning is emitted.
    
    Examples of the warnings generated:
    
    test.s: Assembler messages:
    test.s:5: Warning: specified register cannot be written to at operand 1 -- `msr dbgdtrrx_el0,x3'
    test.s:7: Warning: specified register cannot be read from at operand 2 -- `mrs x3,dbgdtrtx_el0'
    test.s:8: Warning: specified register cannot be written to at operand 1 -- `msr midr_el1,x3'
    
    and disassembly notes:
    
    0000000000000000 <main>:
       0:	d5130503 	msr	dbgdtrtx_el0, x3
       4:	d5130503 	msr	dbgdtrtx_el0, x3
       8:	d5330503 	mrs	x3, dbgdtrrx_el0
       c:	d5330503 	mrs	x3, dbgdtrrx_el0
      10:	d5180003 	msr	midr_el1, x3	; note: writing to a read-only register.
    
    Note that because dbgdtrrx_el0 and dbgdtrtx_el0 have the same encoding, during
    disassembly the constraints are use to disambiguate between the two.  An exact
    constraint match is always prefered over partial ones if available.
    
    As always the warnings can be suppressed with -w and also be made errors using
    warnings as errors.
    
    binutils/
    
    	PR binutils/21446
    	* doc/binutils.texi (-M): Document AArch64 options.
    
    gas/
    
    	PR binutils/21446
    	* testsuite/gas/aarch64/illegal-sysreg-2.s: Fix pmbidr_el1 test.
    	* testsuite/gas/aarch64/illegal-sysreg-2.l: Likewise.
    	* testsuite/gas/aarch64/illegal-sysreg-2.d: Likewise.
    	* testsuite/gas/aarch64/sysreg-diagnostic.s: New.
    	* testsuite/gas/aarch64/sysreg-diagnostic.l: New.
    	* testsuite/gas/aarch64/sysreg-diagnostic.d: New.
    
    include/
    
    	PR binutils/21446
    	* opcode/aarch64.h (F_SYS_READ, F_SYS_WRITE): New.
    
    opcodes/
    
    	PR binutils/21446
    	* aarch64-asm.c (opintl.h): Include.
    	(aarch64_ins_sysreg): Enforce read/write constraints.
    	* aarch64-dis.c (aarch64_ext_sysreg): Likewise.
    	* aarch64-opc.h (F_DEPRECATED, F_ARCHEXT, F_HASXT): Moved here.
    	(F_REG_READ, F_REG_WRITE): New.
    	* aarch64-opc.c (aarch64_print_operand): Generate notes for
    	AARCH64_OPND_SYSREG.
    	(F_DEPRECATED, F_ARCHEXT, F_HASXT): Move to aarch64-opc.h.
    	(aarch64_sys_regs): Add constraints to currentel, midr_el1, ctr_el0,
    	mpidr_el1, revidr_el1, aidr_el1, dczid_el0, id_dfr0_el1, id_pfr0_el1,
    	id_pfr1_el1, id_afr0_el1, id_mmfr0_el1, id_mmfr1_el1, id_mmfr2_el1,
    	id_mmfr3_el1, id_mmfr4_el1, id_isar0_el1, id_isar1_el1, id_isar2_el1,
    	id_isar3_el1, id_isar4_el1, id_isar5_el1, mvfr0_el1, mvfr1_el1,
    	mvfr2_el1, ccsidr_el1, id_aa64pfr0_el1, id_aa64pfr1_el1,
    	id_aa64dfr0_el1, id_aa64dfr1_el1, id_aa64isar0_el1, id_aa64isar1_el1,
    	id_aa64mmfr0_el1, id_aa64mmfr1_el1, id_aa64mmfr2_el1, id_aa64afr0_el1,
    	id_aa64afr0_el1, id_aa64afr1_el1, id_aa64zfr0_el1, clidr_el1,
    	csselr_el1, vsesr_el2, erridr_el1, erxfr_el1, rvbar_el1, rvbar_el2,
    	rvbar_el3, isr_el1, tpidrro_el0, cntfrq_el0, cntpct_el0, cntvct_el0,
    	mdccsr_el0, dbgdtrrx_el0, dbgdtrtx_el0, osdtrrx_el1, osdtrtx_el1,
    	mdrar_el1, oslar_el1, oslsr_el1, dbgauthstatus_el1, pmbidr_el1,
    	pmsidr_el1, pmswinc_el0, pmceid0_el0, pmceid1_el0.
    	* aarch64-tbl.h (aarch64_opcode_table): Add constraints to
    	msr (F_SYS_WRITE), mrs (F_SYS_READ).
Comment 5 Tamar Christina 2018-05-15 16:43:03 UTC
Issue is now fixed in master.
Comment 6 Sourceware Commits 2018-06-07 10:56:27 UTC
The master branch has been updated by Tamar Christina <tnfchris@sourceware.org>:

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

commit 2a9b2c1abe29e1063240604d2464b8de9a46011f
Author: Tamar Christina <tamar.christina@arm.com>
Date:   Thu Jun 7 11:53:13 2018 +0100

    Fix AArch64 unintialized variable which can cause diagnostic failures.
    
    This patch fixes an uninitialized memory issue that can under certain
    circumstances turn an error into a warning because non_fatal was not initialised
    in all cases.
    
    Verified using valgrind.
    
    gas/config/tc-aarch64.c
    
    	PR binutils/21446
    	* tc-aarch64.c (record_operand_error, record_operand_error_with_data):
    	  Initialize non_fatal.