From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x232.google.com (mail-lj1-x232.google.com [IPv6:2a00:1450:4864:20::232]) by sourceware.org (Postfix) with ESMTPS id A244B3858402 for ; Fri, 5 Jan 2024 13:58:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A244B3858402 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org A244B3858402 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::232 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704463116; cv=none; b=BlsNobsziTDxcjPHOzlVDzSRDxpjlJr1r2ODFROPk4Nymj0z1zdX3WNiTfmHci23ymKeQGlmIFC8gylBxFMPvXexmPBbTujo5skF4tTJPo+RqaZSyP02kNbmXN2v8+vBQ7tMRstTSLUkVixmkrTyMd/i8YKx/pa6v1hvrricnzY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1704463116; c=relaxed/simple; bh=TtP5luac+yqq49lFvHQV+f4eQs8eAnFE3/Cy011YLfM=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=pjRFQzc4bGBjNTXK8HGXUu4O0AxhEW4duzDKlbHTX+tYyH3hyibMaOhx0jbencwoPVkuXvKALYYV/jst+MEEHPrYKyiZ76GCQEXB6/Z+9R3LAghImXfLr5VyXETO4gQb4eXqHBYJrlI3qbR3KE1SVI50izGtGb/kLT1EcoYJPbs= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lj1-x232.google.com with SMTP id 38308e7fff4ca-2cc9fa5e8e1so20077701fa.3 for ; Fri, 05 Jan 2024 05:58:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1704463108; x=1705067908; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:autocrypt:from:cc:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=LbdQzmiGpLOsd+ya8qwDyfcwPLWb3C+RMv4LBfMpgnI=; b=EBdG/ZUAyWhiLeXJ+RSg3doD0YEECSqwbza+1rqxYV64A2111MeGc1MESTV9vIy0cr Hy/Z+RencIJVZfCGBuZuKg/7TI+iuwhdlugvCAcoVgMLTPRLHxJMiMXzm3VXGWLK13vQ xGSevuVmdRtYOjPO4dajn/xQV6BGmPQQ0WE9W90Ea3UJznYE5wDoh4e7ea7FKegTzocP lrylX9xsDoMQC9rM8qQkRbmjPMtiUiXD6vEe7ieWySrtfT4VUdy1vMEO/nfLTXXRNVTS HCmGIHMCxS+iaCBH7Xc1dOp91s1NawVOdSY0leYemvXVnj0wRjPqw4s7FcOy3K9Qe68P hk4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704463108; x=1705067908; h=content-transfer-encoding:in-reply-to:autocrypt:from:cc:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=LbdQzmiGpLOsd+ya8qwDyfcwPLWb3C+RMv4LBfMpgnI=; b=HBlKRKBrL7REXEd18wCTAs0PMZYioRYHZEn/7DxfD/DJd7tavn0HkwGu/LJrv9QUe+ Z5981xOi9J/MpYDoG0Be3qRhbHY6K6kuEb3knAyxtzcniOYHC9tkC+iAMJ5d/HffBW2+ y6voFqqZCJi7/5QeQ8sML+EUHgu2HhNxvLAd8xVTjpcchc+VgU0gT/UcJmsyrqXUQyB8 fvXZ/2mZZWqz7iaVBThyH8b74VRWVGQb08aX6Ves8qs8mdjH90uaBGucxFhJ+eCOvelW 9OkMnpZU05vs6HiIDvMXwOZPjmOoDpQfYNxwv17qBI6qx8P70uKRyOSibr+a1Gj2STYH Tx3w== X-Gm-Message-State: AOJu0YzQ3OVR5Pjd3tAD2/uEoIbdy1zka3CABT2AT0n3hHzrfgWhpUUX DvUDi6Fy6w6Xh2Xl9i9U/HoqyLB7i4sk X-Google-Smtp-Source: AGHT+IHzhcrZmmdMoKMg+cG9xP4Pv1+m9M2UCQyMW+6zECk9BPl2vdrgPBoi5GwwIgFyMm7ElfCJVA== X-Received: by 2002:a2e:a481:0:b0:2cc:effb:cbd0 with SMTP id h1-20020a2ea481000000b002cceffbcbd0mr1004604lji.55.1704463107495; Fri, 05 Jan 2024 05:58:27 -0800 (PST) Received: from [10.156.60.236] (ip-037-024-206-209.um08.pools.vodafone-ip.de. [37.24.206.209]) by smtp.gmail.com with ESMTPSA id a4-20020a0566380b0400b0046b66fc7461sm427637jab.147.2024.01.05.05.58.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 05 Jan 2024 05:58:26 -0800 (PST) Message-ID: <0ecd9240-0700-4072-91d4-ccf9bdb56071@suse.com> Date: Fri, 5 Jan 2024 14:58:24 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH,V4 10/14] gas: synthesize CFI for hand-written asm Content-Language: en-US To: Indu Bhagat References: <20240103071526.3846985-1-indu.bhagat@oracle.com> <20240103071526.3846985-11-indu.bhagat@oracle.com> Cc: binutils@sourceware.org From: Jan Beulich Autocrypt: addr=jbeulich@suse.com; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL In-Reply-To: <20240103071526.3846985-11-indu.bhagat@oracle.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3026.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 03.01.2024 08:15, Indu Bhagat wrote: > --- a/gas/config/obj-elf.c > +++ b/gas/config/obj-elf.c > @@ -24,6 +24,7 @@ > #include "subsegs.h" > #include "obstack.h" > #include "dwarf2dbg.h" > +#include "ginsn.h" > > #ifndef ECOFF_DEBUGGING > #define ECOFF_DEBUGGING 0 > @@ -2311,6 +2312,13 @@ obj_elf_size (int ignore ATTRIBUTE_UNUSED) > symbol_get_obj (sym)->size = XNEW (expressionS); > *symbol_get_obj (sym)->size = exp; > } > + > + /* If the symbol in the directive matches the current function being > + processed, indicate end of the current stream of ginsns. */ > + if (flag_synth_cfi > + && S_IS_FUNCTION (sym) && sym == ginsn_data_func_symbol ()) > + ginsn_data_end (symbol_temp_new_now ()); > + > demand_empty_rest_of_line (); > } > > @@ -2499,6 +2507,14 @@ obj_elf_type (int ignore ATTRIBUTE_UNUSED) > elfsym->symbol.flags &= ~mask; > } > > + if (S_IS_FUNCTION (sym) && flag_synth_cfi) > + { > + /* Wrap up processing the previous block of ginsns first. */ > + if (frchain_now->frch_ginsn_data) > + ginsn_data_end (symbol_temp_new_now ()); > + ginsn_data_begin (sym); > + } > + > demand_empty_rest_of_line (); > } Documentation about .type and .size use could be more precise. Generally it is entirely benign where exactly these directives live relative to the code they annotate. > --- a/gas/config/tc-i386.c > +++ b/gas/config/tc-i386.c > @@ -30,6 +30,7 @@ > #include "subsegs.h" > #include "dwarf2dbg.h" > #include "dw2gencfi.h" > +#include "scfi.h" > #include "gen-sframe.h" > #include "sframe.h" > #include "elf/x86-64.h" > @@ -5287,6 +5288,978 @@ static INLINE bool may_need_pass2 (const insn_template *t) > && t->base_opcode == 0x63); > } > > +bool > +x86_scfi_callee_saved_p (unsigned int dw2reg_num) Iirc SCFI is ELF-only. We're not in ELF-only code here, though. Non-ELF gas, as indicated before, would better not carry any unreachable code. > +{ > + if (dw2reg_num == 3 /* rbx. */ > + || dw2reg_num == REG_FP /* rbp. */ > + || dw2reg_num == REG_SP /* rsp. */ > + || (dw2reg_num >= 12 && dw2reg_num <= 15) /* r12 - r15. */) > + return true; > + > + return false; > +} This entire function is SysV-ABI-specific without this being made clear by a comment. > +/* Check whether a '66H' prefix accompanies the instruction. With APX 16-bit operand size isn't necessarily represented by a 66h prefix, but perhaps with an "embedded prefix" inside the EVEX one. Therefore both the comment and even more so ... > + The current users of this API are in the handlers for PUSH, POP > + instructions. These instructions affect the stack pointer implicitly: the > + operand size (16, 32, or 64 bits) determines the amount by which the stack > + pointer is decremented (2, 4 or 8). When '66H' prefix is present, the > + instruction has a 16-bit operand. */ > + > +static bool > +ginsn_prefix_66H_p (i386_insn insn) ... the function name would better not allude to just the legacy encoding. Maybe ginsn_opsize_prefix_p()? > +{ > + return (insn.prefix[DATA_PREFIX] == 0x66); > +} I take it that all ginsn / scfi interaction is late enough in the process for this array element already having been reliably set? > +/* Get the DWARF register number for the given register entry. > + For specific byte/word register accesses like al, cl, ah, ch, r8dyte etc., What's "r8dyte"? I expect it's a typo, but I can't derive what was intended to be written. > + there is no valid DWARF register number. This function is a hack - it > + relies on relative ordering of reg entries in the i386_regtab. FIXME - it > + will be good to allow a more direct way to get this information. */ Saying it's a hack is a helpful sign, but it still would be useful to also briefly describe what the intentions here are. It's hard to spot whether the code is correct without knowing what's intended (i.e. how 8- and 16-bit registers, or non-GPRs, are meant to be treated). > +static unsigned int > +ginsn_dw2_regnum (const reg_entry *ireg) > +{ > + /* PS: Note the data type here as int32_t, because of Dw2Inval (-1). */ > + int32_t dwarf_reg = Dw2Inval; > + const reg_entry *temp = ireg; > + > + /* ginsn creation is available for AMD64 abi only ATM. Other flag_code > + are not expected. */ > + gas_assert (flag_code == CODE_64BIT); With this assertion it is kind of odd to see a further use of flag_code below. > + if (ireg <= &i386_regtab[3]) > + /* For al, cl, dl, bl, bump over to axl, cxl, dxl, bxl respectively by > + adding 8. */ > + temp = ireg + 8; > + else if (ireg <= &i386_regtab[7]) > + /* For ah, ch, dh, bh, bump over to axl, cxl, dxl, bxl respectively by > + adding 4. */ > + temp = ireg + 4; Assuming this is a frequently executed path, why do these not live ... > + dwarf_reg = temp->dw2_regnum[flag_code >> 1]; > + if (dwarf_reg == Dw2Inval) > + { ... here, thus at least not affecting the code path taken for 64-bit GPRs. > + /* The code relies on the relative ordering of the reg entries in > + i386_regtab. The assertion here ensures the code does not recurse > + indefinitely. */ > + gas_assert (temp + 16 < &i386_regtab[i386_regtab_size - 1]); Afaict this is (still) undefined behavior. You may not add to a pointer without knowing whether the result still points into or exactly past the underlying array. > + temp = temp + 16; Also - where's the 16 coming from? Was this not updated when rebasing over APX? > + dwarf_reg = ginsn_dw2_regnum (temp); > + } > + > + gas_assert (dwarf_reg != Dw2Inval); /* Needs to be addressed. */ Without actually addressing this (and possible similar cases elsewhere), I don't think this can go in as other than experimental code (which the NEWS entry then should state, and where there then should be a plan for an easy approach of probing gas for no-longer-experimental SCFI support). > + return (unsigned int) dwarf_reg; > +} > + > +static ginsnS * > +x86_ginsn_addsub_reg_mem (const symbolS *insn_end_sym) > +{ > + unsigned int dw2_regnum; > + unsigned int src2_dw2_regnum; > + ginsnS *ginsn = NULL; > + ginsnS * (*ginsn_func) (const symbolS *, bool, > + enum ginsn_src_type, unsigned int, offsetT, > + enum ginsn_src_type, unsigned int, offsetT, > + enum ginsn_dst_type, unsigned int, offsetT); > + uint16_t opcode = i.tm.base_opcode; > + > + gas_assert (opcode == 0x1 || opcode == 0x29); > + ginsn_func = (opcode == 0x1) ? ginsn_new_add : ginsn_new_sub; As mentioned before - checking opcode without also checking opcode space is pretty meaningless. > + /* op %reg, symbol. */ > + if (i.mem_operands == 1 && !i.base_reg && !i.index_reg) > + return ginsn; Why does this need special treatment, and why is returning NULL here okay? > + /* op reg, reg/mem. */ > + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > + if (i.reg_operands == 2) > + { > + src2_dw2_regnum = ginsn_dw2_regnum (i.op[1].regs); > + ginsn = ginsn_func (insn_end_sym, true, > + GINSN_SRC_REG, dw2_regnum, 0, > + GINSN_SRC_REG, src2_dw2_regnum, 0, > + GINSN_DST_REG, src2_dw2_regnum, 0); > + ginsn_set_where (ginsn); > + } > + /* Other cases where destination involves indirect access are unnecessary > + for SCFI correctness. TBD_GINSN_GEN_NOT_SCFI. */ > + > + return ginsn; > +} > + > +static ginsnS * > +x86_ginsn_addsub_mem_reg (const symbolS *insn_end_sym) > +{ > + unsigned int dw2_regnum; > + unsigned int src2_dw2_regnum; > + const reg_entry *mem_reg; > + int32_t gdisp = 0; > + ginsnS *ginsn = NULL; > + ginsnS * (*ginsn_func) (const symbolS *, bool, > + enum ginsn_src_type, unsigned int, offsetT, > + enum ginsn_src_type, unsigned int, offsetT, > + enum ginsn_dst_type, unsigned int, offsetT); > + uint16_t opcode = i.tm.base_opcode; > + > + gas_assert (opcode == 0x3 || opcode == 0x2b); > + ginsn_func = (opcode == 0x3) ? ginsn_new_add : ginsn_new_sub; > + > + /* op symbol, %reg. */ > + if (i.mem_operands && !i.base_reg && !i.index_reg) > + return ginsn; > + /* op mem, %reg. */ /* op reg/mem, reg. */ you mean? Which then raises the question ... > + dw2_regnum = ginsn_dw2_regnum (i.op[1].regs); > + > + if (i.mem_operands) > + { > + mem_reg = (i.base_reg) ? i.base_reg : i.index_reg; > + src2_dw2_regnum = ginsn_dw2_regnum (mem_reg); > + if (i.disp_operands == 1) > + gdisp = i.op[0].disps->X_add_number; > + ginsn = ginsn_func (insn_end_sym, true, > + GINSN_SRC_INDIRECT, src2_dw2_regnum, gdisp, > + GINSN_SRC_REG, dw2_regnum, 0, > + GINSN_DST_REG, dw2_regnum, 0); > + ginsn_set_where (ginsn); > + } > + > + return ginsn; > +} ... why a register source isn't dealt with here. > +static ginsnS * > +x86_ginsn_alu_imm (const symbolS *insn_end_sym) > +{ > + offsetT src_imm; > + unsigned int dw2_regnum; > + ginsnS *ginsn = NULL; > + enum ginsn_src_type src_type = GINSN_SRC_REG; > + enum ginsn_dst_type dst_type = GINSN_DST_REG; > + > + ginsnS * (*ginsn_func) (const symbolS *, bool, > + enum ginsn_src_type, unsigned int, offsetT, > + enum ginsn_src_type, unsigned int, offsetT, > + enum ginsn_dst_type, unsigned int, offsetT); > + > + /* FIXME - create ginsn where dest is REG_SP / REG_FP only ? */ > + /* Map for insn.tm.extension_opcode > + 000 ADD 100 AND > + 001 OR 101 SUB > + 010 ADC 110 XOR > + 011 SBB 111 CMP */ > + > + /* add/sub/and imm, %reg only at this time for SCFI. > + Although all three (and, or , xor) make the destination reg untraceable, Why would this also be done for CMP? And neither ADC nor SBB are mentioned at all in ... > + and op is handled but not or/xor because we will look into supporting > + the DRAP pattern at some point. */ ... this entire comment, justifying the choice made. > + if (i.tm.extension_opcode == 5) > + ginsn_func = ginsn_new_sub; > + else if (i.tm.extension_opcode == 4) > + ginsn_func = ginsn_new_and; > + else if (i.tm.extension_opcode == 0) > + ginsn_func = ginsn_new_add; > + else > + return ginsn; > + > + /* TBD_GINSN_REPRESENTATION_LIMIT: There is no representation for when a > + symbol is used as an operand, like so: > + addq $simd_cmp_op+8, %rdx > + Skip generating any ginsn for this. */ > + if (i.imm_operands == 1 > + && i.op[0].imms->X_op != O_constant) > + return ginsn; > + > + /* addq $1, symbol > + addq $1, -16(%rbp) > + Such instructions are not of interest for SCFI. */ > + if (i.mem_operands == 1) > + return ginsn; Perhaps not just here: TBD_GINSN_GEN_NOT_SCFI? > + gas_assert (i.imm_operands == 1); > + src_imm = i.op[0].imms->X_add_number; > + /* The second operand may be a register or indirect access. For SCFI, only > + the case when the second opnd is a register is interesting. Revisit this > + if generating ginsns for a different gen mode TBD_GINSN_GEN_NOT_SCFI. */ > + if (i.reg_operands == 1) > + { > + dw2_regnum = ginsn_dw2_regnum (i.op[1].regs); > + /* For ginsn, keep the imm as second src operand. */ > + ginsn = ginsn_func (insn_end_sym, true, > + src_type, dw2_regnum, 0, > + GINSN_SRC_IMM, 0, src_imm, > + dst_type, dw2_regnum, 0); > + > + ginsn_set_where (ginsn); > + } > + > + return ginsn; > +} > + > +static ginsnS * > +x86_ginsn_move (const symbolS *insn_end_sym) > +{ > + ginsnS *ginsn; > + unsigned int dst_reg; > + unsigned int src_reg; > + offsetT src_disp = 0; > + offsetT dst_disp = 0; > + const reg_entry *dst = NULL; > + const reg_entry *src = NULL; > + uint16_t opcode = i.tm.base_opcode; > + enum ginsn_src_type src_type = GINSN_SRC_REG; > + enum ginsn_dst_type dst_type = GINSN_DST_REG; > + > + if (opcode == 0x8b || opcode == 0x8a) Above when handling ALU insns you didn't care about byte ops - why do you do so here (by checking for 0x8a, and 0x88 below)? > + { > + /* mov disp(%reg), %reg. */ > + if (i.mem_operands && i.base_reg) > + { > + src = i.base_reg; > + if (i.disp_operands == 1) > + src_disp = i.op[0].disps->X_add_number; > + src_type = GINSN_SRC_INDIRECT; > + } > + else > + src = i.op[0].regs; Even when there's no base, the source isn't necessarily a register. And in such a case using i.op[0].regs isn't valid. > + dst = i.op[1].regs; > + } > + else if (opcode == 0x89 || opcode == 0x88) > + { > + /* mov %reg, disp(%reg). */ > + src = i.op[0].regs; > + if (i.mem_operands && i.base_reg) > + { > + dst = i.base_reg; > + if (i.disp_operands == 1) > + dst_disp = i.op[1].disps->X_add_number; > + dst_type = GINSN_DST_INDIRECT; > + } > + else > + dst = i.op[1].regs; Similarly here then. > + } > + > + src_reg = ginsn_dw2_regnum (src); > + dst_reg = ginsn_dw2_regnum (dst); > + > + ginsn = ginsn_new_mov (insn_end_sym, true, > + src_type, src_reg, src_disp, > + dst_type, dst_reg, dst_disp); > + ginsn_set_where (ginsn); > + > + return ginsn; > +} > + > +/* Generate appropriate ginsn for lea. > + Sub-cases marked with TBD_GINSN_INFO_LOSS indicate some loss of information > + in the ginsn. But these are fine for now for GINSN_GEN_SCFI generation > + mode. */ > + > +static ginsnS * > +x86_ginsn_lea (const symbolS *insn_end_sym) > +{ > + offsetT src_disp = 0; > + ginsnS *ginsn = NULL; > + unsigned int base_reg; > + unsigned int index_reg; > + offsetT index_scale; > + unsigned int dst_reg; > + > + if (!i.index_reg && !i.base_reg) > + { > + /* lea symbol, %rN. */ > + dst_reg = ginsn_dw2_regnum (i.op[1].regs); > + /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol. */ > + ginsn = ginsn_new_mov (insn_end_sym, false, > + GINSN_SRC_IMM, 0xf /* arbitrary const. */, 0, > + GINSN_DST_REG, dst_reg, 0); > + } > + else if (i.base_reg && !i.index_reg) > + { > + /* lea -0x2(%base),%dst. */ > + base_reg = ginsn_dw2_regnum (i.base_reg); What if base is %eip? Aiui ginsn_dw2_regnum() will hit an assertion then. And what about e.g. "lea symbol(%rbx), %rbp"? The ... > + dst_reg = ginsn_dw2_regnum (i.op[1].regs); > + > + if (i.disp_operands) > + src_disp = i.op[0].disps->X_add_number; ... constant retrieved here won't properly represent the displacement then. > + if (src_disp) > + /* Generate an ADD ginsn. */ > + ginsn = ginsn_new_add (insn_end_sym, true, > + GINSN_SRC_REG, base_reg, 0, > + GINSN_SRC_IMM, 0, src_disp, > + GINSN_DST_REG, dst_reg, 0); > + else > + /* Generate a MOV ginsn. */ > + ginsn = ginsn_new_mov (insn_end_sym, true, > + GINSN_SRC_REG, base_reg, 0, > + GINSN_DST_REG, dst_reg, 0); > + } > + else if (!i.base_reg && i.index_reg) > + { > + /* lea (,%index,imm), %dst. */ > + /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation, > + instead use GINSN_TYPE_OTHER. */ You're also losing the displacement here. > + index_scale = i.log2_scale_factor; > + index_reg = ginsn_dw2_regnum (i.index_reg); > + dst_reg = ginsn_dw2_regnum (i.op[1].regs); > + ginsn = ginsn_new_other (insn_end_sym, true, > + GINSN_SRC_REG, index_reg, > + GINSN_SRC_IMM, index_scale, > + GINSN_DST_REG, dst_reg); Wouldn't it make sense to represent a scale factor of 1 correctly here (i.e. not as "other", but rather similar to the base-without- index case above)? > + } > + else > + { > + /* lea disp(%base,%index,imm) %dst. */ > + /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm > + for index reg. */ > + base_reg = ginsn_dw2_regnum (i.base_reg); > + index_reg = ginsn_dw2_regnum (i.index_reg); > + dst_reg = ginsn_dw2_regnum (i.op[1].regs); > + /* Generate an ADD ginsn. */ > + ginsn = ginsn_new_add (insn_end_sym, true, > + GINSN_SRC_REG, base_reg, 0, > + GINSN_SRC_REG, index_reg, 0, > + GINSN_DST_REG, dst_reg, 0); Seeing the use of "other" above, why is this (wrongly) represented as "add"? > + } > + > + ginsn_set_where (ginsn); > + > + return ginsn; > +} Throughout this function (and perhaps others as well), how come you don't consider operand size at all? It matters whether results are 64-bit, 32-bit, or 16-bit, after all. > +static ginsnS * > +x86_ginsn_jump (const symbolS *insn_end_sym) > +{ > + ginsnS *ginsn = NULL; > + symbolS *src_symbol; Here and elsewhere - please use const whenever possible. (I think I said so already on an earlier version.) > + gas_assert (i.disp_operands == 1); > + > + /* A non-zero addend in jump target makes control-flow tracking difficult. > + Skip SCFI for now. */ > + if (i.op[0].disps->X_op == O_symbol && i.op[0].disps->X_add_number) > + { > + as_bad ("SCFI: jmp insn with non-zero addend to sym not supported"); > + return ginsn; > + } > + > + if (i.op[0].disps->X_op == O_symbol) Why the redundant evaluation of ->X_op? In fact, if you moved the earlier if() ... > + { ... here, this ... > + gas_assert (!i.op[0].disps->X_add_number); ... assertion would become entirely redundant. > + src_symbol = i.op[0].disps->X_add_symbol; > + ginsn = ginsn_new_jump (insn_end_sym, true, > + GINSN_SRC_SYMBOL, 0, src_symbol); > + > + ginsn_set_where (ginsn); > + } > + > + return ginsn; > +} > + > +static ginsnS * > +x86_ginsn_jump_cond (const symbolS *insn_end_sym) > +{ > + ginsnS *ginsn = NULL; > + symbolS *src_symbol; > + > + gas_assert (i.disp_operands == 1); > + > + /* A non-zero addend in JCC target makes control-flow tracking difficult. > + Skip SCFI for now. */ > + if (i.op[0].disps->X_op == O_symbol && i.op[0].disps->X_add_number) > + { > + as_bad ("SCFI: jcc insn with non-zero addend to sym not supported"); > + return ginsn; > + } > + > + if (i.op[0].disps->X_op == O_symbol) > + { > + gas_assert (i.op[0].disps->X_add_number == 0); > + src_symbol = i.op[0].disps->X_add_symbol; > + ginsn = ginsn_new_jump_cond (insn_end_sym, true, > + GINSN_SRC_SYMBOL, 0, src_symbol); > + ginsn_set_where (ginsn); > + } > + > + return ginsn; > +} This looks almost identical to x86_ginsn_jump() - can't the two be folded? > +static ginsnS * > +x86_ginsn_enter (const symbolS *insn_end_sym) > +{ > + ginsnS *ginsn = NULL; > + ginsnS *ginsn_next = NULL; > + ginsnS *ginsn_last = NULL; > + > + gas_assert (i.imm_operands == 2); > + > + /* For non-zero size operands, bail out as untraceable for SCFI. */ > + if ((i.op[0].imms->X_op != O_constant || i.op[0].imms->X_add_symbol != 0) > + || (i.op[1].imms->X_op != O_constant || i.op[1].imms->X_add_symbol != 0)) While the comment makes sufficiently clear what's meant, the use of (inner) parentheses here is still confusing as to whether indeed the || are meant. > + { > + as_bad ("SCFI: enter insn with non-zero operand not supported"); > + return ginsn; > + } > + > + /* If the nesting level is 0, the processor pushes the frame pointer from > + the BP/EBP/RBP register onto the stack, copies the current stack > + pointer from the SP/ESP/RSP register into the BP/EBP/RBP register, and > + loads the SP/ESP/RSP register with the current stack-pointer value > + minus the value in the size operand. */ > + ginsn = ginsn_new_sub (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, 8, I guess 8 is the operand size and you simply hope no-one's going to use a 16-bit ENTER? > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn); > + ginsn_next = ginsn_new_store (insn_end_sym, false, > + GINSN_SRC_REG, REG_FP, > + GINSN_DST_INDIRECT, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + ginsn_last = ginsn_new_mov (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_DST_REG, REG_FP, 0); > + ginsn_set_where (ginsn_last); > + gas_assert (!ginsn_link_next (ginsn_next, ginsn_last)); > + > + return ginsn; > +} > + > +static bool > +x86_ginsn_safe_to_skip (void) > +{ > + bool skip_p = false; > + uint16_t opcode = i.tm.base_opcode; > + > + switch (opcode) > + { > + case 0x39: This isn't the only CMP encoding, and ... > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* cmp reg, reg. */ > + skip_p = true; > + break; > + case 0x85: ... this isn't the only TEST one. > + /* test reg, reg/mem. */ > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + skip_p = true; > + break; > + default: > + break; > + } > + > + return skip_p; > +} > + > +#define X86_GINSN_UNHANDLED_NONE 0 > +#define X86_GINSN_UNHANDLED_DEST_REG 1 > +#define X86_GINSN_UNHANDLED_CFG 2 > +#define X86_GINSN_UNHANDLED_STACKOP 3 > + > +/* Check the input insn for its impact on the correctness of the synthesized > + CFI. Returns an error code to the caller. */ > + > +static int > +x86_ginsn_unhandled (void) > +{ > + int err = X86_GINSN_UNHANDLED_NONE; > + const reg_entry *reg_op; > + unsigned int dw2_regnum; > + > + /* Keep an eye out for instructions affecting control flow. */ > + if (i.tm.opcode_modifier.jump) > + err = X86_GINSN_UNHANDLED_CFG; > + /* Also, for any instructions involving an implicit update to the stack > + pointer. */ > + else if (i.tm.opcode_modifier.implicitstackop) > + err = X86_GINSN_UNHANDLED_STACKOP; > + /* Finally, also check if the missed instructions are affecting REG_SP or > + REG_FP. The destination operand is the last at all stages of assembly > + (due to following AT&T syntax layout in the internal representation). In > + case of Intel syntax input, this still remains true as swap_operands () > + is done by now. > + PS: These checks do not involve index / base reg, as indirect memory > + accesses via REG_SP or REG_FP do not affect SCFI correctness. > + (Also note these instructions are candidates for other ginsn generation > + modes in future. TBD_GINSN_GEN_NOT_SCFI.) */ > + else if (i.operands && i.reg_operands > + && !(i.flags[i.operands - 1] & Operand_Mem)) > + { > + reg_op = i.op[i.operands - 1].regs; > + if (reg_op) > + { > + dw2_regnum = ginsn_dw2_regnum (reg_op); > + if (dw2_regnum == REG_SP || dw2_regnum == REG_FP) > + err = X86_GINSN_UNHANDLED_DEST_REG; > + } else err = X86_GINSN_UNHANDLED_CONFUSED; ? You can't let this case go silently. An alternative would be to assert instead of using if(). > + } > + > + return err; > +} > + > +/* Generate one or more generic GAS instructions, a.k.a, ginsns for the current > + machine instruction. > + > + Returns the head of linked list of ginsn(s) added, if success; Returns NULL > + if failure. > + > + The input ginsn_gen_mode GMODE determines the set of minimal necessary > + ginsns necessary for correctness of any passes applicable for that mode. > + For supporting the GINSN_GEN_SCFI generation mode, following is the list of > + machine instructions that must be translated into the corresponding ginsns > + to ensure correctness of SCFI: > + - All instructions affecting the two registers that could potentially > + be used as the base register for CFA tracking. For SCFI, the base > + register for CFA tracking is limited to REG_SP and REG_FP only for > + now. > + - All change of flow instructions: conditional and unconditional branches, > + call and return from functions. > + - All instructions that can potentially be a register save / restore > + operation. This could do with being more fine grained, as "potentially" is pretty vague, and (as per earlier version review comments) my take on this is a much wider set than yours. > + - All instructions that perform stack manipulation implicitly: the CALL, > + RET, PUSH, POP, ENTER, and LEAVE instructions. > + > + The function currently supports GINSN_GEN_SCFI ginsn generation mode only. > + To support other generation modes will require work on this target-specific > + process of creation of ginsns: > + - Some of such places are tagged with TBD_GINSN_GEN_NOT_SCFI to serve as > + possible starting points. Oh, I see you're not meaning to have this annotation consistently. That's a little sad ... > + - Also note that ginsn representation may need enhancements. Specifically, > + note some TBD_GINSN_INFO_LOSS and TBD_GINSN_REPRESENTATION_LIMIT markers. > + */ > + > +static ginsnS * > +x86_ginsn_new (const symbolS *insn_end_sym, enum ginsn_gen_mode gmode) > +{ > + int err = 0; > + uint16_t opcode; > + unsigned int dw2_regnum; > + ginsnS *ginsn = NULL; > + ginsnS *ginsn_next = NULL; > + ginsnS *ginsn_last = NULL; > + /* In 64-bit mode, the default stack update size is 8 bytes. */ > + int stack_opnd_size = 8; > + > + /* Currently supports generation of selected ginsns, sufficient for > + the use-case of SCFI only. */ > + if (gmode != GINSN_GEN_SCFI) > + return ginsn; > + > + opcode = i.tm.base_opcode; > + > + switch (opcode) > + { > + case 0x1: > + /* add reg, reg/mem. */ > + case 0x29: > + if (i.tm.opcode_space != SPACE_BASE) > + break; What about the new APX NDD encodings in EVEX map 4? > + /* sub reg, reg/mem. */ Please be careful with placing such comments when there are multiple case labels (or fall-through). I think these would better go on the same lines as the case labels themselves. > + ginsn = x86_ginsn_addsub_reg_mem (insn_end_sym); > + break; > + > + case 0x3: > + /* add reg/mem, reg. */ > + case 0x2b: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* sub reg/mem, reg. */ > + ginsn = x86_ginsn_addsub_mem_reg (insn_end_sym); > + break; > + > + case 0xa0: > + case 0xa8: > + /* push fs / push gs have opcode_space == SPACE_0F. */ > + if (i.tm.opcode_space != SPACE_0F) > + break; > + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ > + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; But only if there's not also REX.W / REX2.W. > + /* push fs / push gs. */ > + ginsn = ginsn_new_sub (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn); > + ginsn_next = ginsn_new_store (insn_end_sym, false, > + GINSN_SRC_REG, dw2_regnum, > + GINSN_DST_INDIRECT, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + break; > + > + case 0xa1: > + case 0xa9: > + /* pop fs / pop gs have opcode_space == SPACE_0F. */ > + if (i.tm.opcode_space != SPACE_0F) > + break; > + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ > + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; > + /* pop fs / pop gs. */ > + ginsn = ginsn_new_load (insn_end_sym, false, > + GINSN_SRC_INDIRECT, REG_SP, 0, > + GINSN_DST_REG, dw2_regnum); > + ginsn_set_where (ginsn); > + ginsn_next = ginsn_new_add (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + break; > + > + case 0x50 ... 0x57: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* push reg. */ > + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ > + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; > + ginsn = ginsn_new_sub (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn); > + ginsn_next = ginsn_new_store (insn_end_sym, false, > + GINSN_SRC_REG, dw2_regnum, > + GINSN_DST_INDIRECT, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + break; > + > + case 0x58 ... 0x5f: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* pop reg. */ > + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > + ginsn = ginsn_new_load (insn_end_sym, false, > + GINSN_SRC_INDIRECT, REG_SP, 0, > + GINSN_DST_REG, dw2_regnum); > + ginsn_set_where (ginsn); > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ > + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; > + ginsn_next = ginsn_new_add (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + break; > + > + case 0x6a: > + case 0x68: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* push imm8/imm16/imm32. */ > + if (opcode == 0x6a) > + stack_opnd_size = 1; > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. > + However, this prefix may only be present when opcode is 0x68. */ Why would this be limited to opcode 0x68? > + else if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; > + else > + stack_opnd_size = 4; In 64-bit mode stack operations are never 32-bit. > + /* Skip getting the value of imm from machine instruction > + because this is not important for SCFI. */ > + ginsn = ginsn_new_sub (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn); > + ginsn_next = ginsn_new_store (insn_end_sym, false, > + GINSN_SRC_IMM, 0, > + GINSN_DST_INDIRECT, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + break; > + > + case 0x70 ... 0x7f: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + ginsn = x86_ginsn_jump_cond (insn_end_sym); > + break; I think this wants a comment briefly explaining why SPACE_0F opcodes 0x8[0-f] don't need handling explicitly. Same for JMP (0xeb) below. > + case 0x80: > + case 0x81: > + case 0x83: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + ginsn = x86_ginsn_alu_imm (insn_end_sym); > + break; > + > + case 0x8a: > + case 0x8b: > + /* Move reg/mem, reg (8/16/32/64). */ > + case 0x88: > + case 0x89: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* mov reg, reg/mem. (8/16/32/64). */ > + ginsn = x86_ginsn_move (insn_end_sym); > + break; > + > + case 0x8d: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* lea disp(%src), %dst */ disp(%src) doesn't really represent the full set of possibilities. Why not use "mem" as you do elsewhere? > + ginsn = x86_ginsn_lea (insn_end_sym); > + break; > + > + case 0x8f: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* pop to mem. */ Or to register. While this won't happen today, allowing a means to have the programmer request that alternative encoding would surely miss to update the code here. Hence this code would better be ready to deal with the case right away. > + gas_assert (i.base_reg); POP isn't different from other explicit memory accesses: All forms are allowed, and hence a base register may not be in use. Both remarks also apply to PUSH further down. > + dw2_regnum = ginsn_dw2_regnum (i.base_reg); > + ginsn = ginsn_new_load (insn_end_sym, false, > + GINSN_SRC_INDIRECT, REG_SP, 0, > + GINSN_DST_INDIRECT, dw2_regnum); > + ginsn_set_where (ginsn); > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ > + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; > + ginsn_next = ginsn_new_add (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + break; > + > + case 0x9c: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* pushf / pushfq. */ > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ > + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; > + ginsn = ginsn_new_sub (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn); > + /* Tracking EFLAGS register by number is not necessary. */ How does this fit with ... > + ginsn_next = ginsn_new_store (insn_end_sym, false, > + GINSN_SRC_IMM, 0, > + GINSN_DST_INDIRECT, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + break; > + > + case 0x9d: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* popf / popfq. */ > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ > + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; > + /* FIXME - hardcode the actual DWARF reg number value. As for SCFI > + correctness, although this behaves simply a placeholder value; its > + just clearer if the value is correct. */ > + dw2_regnum = 49; ... this? > + ginsn = ginsn_new_load (insn_end_sym, false, > + GINSN_SRC_INDIRECT, REG_SP, 0, > + GINSN_DST_REG, dw2_regnum); > + ginsn_set_where (ginsn); > + ginsn_next = ginsn_new_add (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + break; > + > + case 0xff: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* push from mem. */ > + if (i.tm.extension_opcode == 6) > + { > + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ > + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) > + stack_opnd_size = 2; > + ginsn = ginsn_new_sub (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, stack_opnd_size, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn); > + /* These instructions have no imm, only indirect access. */ > + gas_assert (i.base_reg); > + dw2_regnum = ginsn_dw2_regnum (i.base_reg); > + ginsn_next = ginsn_new_store (insn_end_sym, false, > + GINSN_SRC_INDIRECT, dw2_regnum, > + GINSN_DST_INDIRECT, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + } > + else if (i.tm.extension_opcode == 4) > + { > + /* jmp r/m. E.g., notrack jmp *%rax. */ > + if (i.reg_operands) > + { > + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > + ginsn = ginsn_new_jump (insn_end_sym, true, > + GINSN_SRC_REG, dw2_regnum, NULL); > + ginsn_set_where (ginsn); > + } > + else if (i.mem_operands && i.index_reg) > + { > + /* jmp *0x0(,%rax,8). */ > + dw2_regnum = ginsn_dw2_regnum (i.index_reg); > + ginsn = ginsn_new_jump (insn_end_sym, true, > + GINSN_SRC_REG, dw2_regnum, NULL); > + ginsn_set_where (ginsn); What if both base and index are in use? Like for PUSH/POP, all addressing forms are permitted here and ... > + } > + else if (i.mem_operands && i.base_reg) > + { > + dw2_regnum = ginsn_dw2_regnum (i.base_reg); > + ginsn = ginsn_new_jump (insn_end_sym, true, > + GINSN_SRC_REG, dw2_regnum, NULL); > + ginsn_set_where (ginsn); > + } > + } > + else if (i.tm.extension_opcode == 2) > + { > + /* 0xFF /2 (call). */ > + if (i.reg_operands) > + { > + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > + ginsn = ginsn_new_call (insn_end_sym, true, > + GINSN_SRC_REG, dw2_regnum, NULL); > + ginsn_set_where (ginsn); > + } > + else if (i.mem_operands && i.base_reg) > + { > + dw2_regnum = ginsn_dw2_regnum (i.base_reg); > + ginsn = ginsn_new_call (insn_end_sym, true, > + GINSN_SRC_REG, dw2_regnum, NULL); > + ginsn_set_where (ginsn); > + } ... here. > + } > + break; > + > + case 0xc2: > + case 0xc3: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* Near ret. */ > + ginsn = ginsn_new_return (insn_end_sym, true); > + ginsn_set_where (ginsn); > + break; No tracking of the stack pointer adjustment? > + case 0xc8: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* enter. */ > + ginsn = x86_ginsn_enter (insn_end_sym); > + break; > + > + case 0xc9: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* The 'leave' instruction copies the contents of the RBP register > + into the RSP register to release all stack space allocated to the > + procedure. */ > + ginsn = ginsn_new_mov (insn_end_sym, false, > + GINSN_SRC_REG, REG_FP, 0, > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn); > + /* Then it restores the old value of the RBP register from the stack. */ > + ginsn_next = ginsn_new_load (insn_end_sym, false, > + GINSN_SRC_INDIRECT, REG_SP, 0, > + GINSN_DST_REG, REG_FP); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); > + ginsn_last = ginsn_new_add (insn_end_sym, false, > + GINSN_SRC_REG, REG_SP, 0, > + GINSN_SRC_IMM, 0, 8, Same comment as for ENTER wrt operand size. > + GINSN_DST_REG, REG_SP, 0); > + ginsn_set_where (ginsn_next); > + gas_assert (!ginsn_link_next (ginsn_next, ginsn_last)); > + break; > + > + case 0xe0 ... 0xe2: > + /* loop / loope / loopne. */ > + case 0xe3: > + /* jecxz / jrcxz. */ > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + ginsn = x86_ginsn_jump_cond (insn_end_sym); > + ginsn_set_where (ginsn); > + break; > + > + case 0xe8: > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* PS: SCFI machinery does not care about which func is being > + called. OK to skip that info. */ > + ginsn = ginsn_new_call (insn_end_sym, true, > + GINSN_SRC_SYMBOL, 0, NULL); > + ginsn_set_where (ginsn); > + break; Again - what about the stack pointer adjustment? Or wait - you only care about the local function's view. Just that this will get you in trouble for something like call 1f 1: pop %rax CALL with zero displacement really acts as if "push %rip". > + case 0xeb: > + /* If opcode_space != SPACE_BASE, this is not a jmp insn. Skip it > + for GINSN_GEN_SCFI. */ > + if (i.tm.opcode_space != SPACE_BASE) > + break; > + /* Unconditional jmp. */ > + ginsn = x86_ginsn_jump (insn_end_sym); > + ginsn_set_where (ginsn); > + break; > + > + default: > + /* TBD_GINSN_GEN_NOT_SCFI: Skip all other opcodes uninteresting for > + GINSN_GEN_SCFI mode. */ > + break; > + } > + > + if (!ginsn && !x86_ginsn_safe_to_skip ()) > + { > + /* For all unhandled insns that are not whitelisted, check that they do > + not impact SCFI correctness. */ > + err = x86_ginsn_unhandled (); > + switch (err) > + { > + case X86_GINSN_UNHANDLED_NONE: > + break; > + case X86_GINSN_UNHANDLED_DEST_REG: > + /* Not all writes to REG_FP are harmful in context of SCFI. Simply > + generate a GINSN_TYPE_OTHER with destination set to the > + appropriate register. The SCFI machinery will bail out if this > + ginsn affects SCFI correctness. */ > + dw2_regnum = ginsn_dw2_regnum (i.op[i.operands - 1].regs); > + ginsn = ginsn_new_other (insn_end_sym, true, > + GINSN_SRC_IMM, 0, > + GINSN_SRC_IMM, 0, > + GINSN_DST_REG, dw2_regnum); > + ginsn_set_where (ginsn); > + break; > + case X86_GINSN_UNHANDLED_CFG: > + /* Fall through. */ > + case X86_GINSN_UNHANDLED_STACKOP: No fall-through comment please between immediately successive case labels. > + as_bad (_("SCFI: unhandled op 0x%x may cause incorrect CFI"), > + i.tm.base_opcode); As a remark: %#x is a one byte shorter representation with largely the same effect (plus, nicely imo, omitting the 0x when the value is zero). > + break; > + default: > + abort (); > + break; > + } > + } > + > + return ginsn; > +} > + > /* This is the guts of the machine-dependent assembler. LINE points to a > machine dependent instruction. This function is supposed to emit > the frags/bytes it assembles to. */ > @@ -5299,6 +6272,7 @@ md_assemble (char *line) > const char *end, *pass1_mnem = NULL; > enum i386_error pass1_err = 0; > const insn_template *t; > + ginsnS *ginsn; > struct last_insn *last_insn > = &seg_info(now_seg)->tc_segment_info_data.last_insn; > > @@ -5830,6 +6804,14 @@ md_assemble (char *line) > /* We are ready to output the insn. */ > output_insn (last_insn); > > + /* PS: SCFI is enabled only for AMD64 ABI. The ABI check has been > + performed in i386_target_format. */ See my earlier comment - it's yet more restrictive (as in not covering e.g. the Windows ABI, which importantly is also used in EFI). > + if (flag_synth_cfi) > + { > + ginsn = x86_ginsn_new (symbol_temp_new_now (), frch_ginsn_gen_mode ()); > + frch_ginsn_data_append (ginsn); > + } > + > insert_lfence_after (); > > if (i.tm.opcode_modifier.isprefix) > @@ -11333,6 +12315,7 @@ s_insn (int dummy ATTRIBUTE_UNUSED) > const char *end; > unsigned int j; > valueT val; > + ginsnS *ginsn; > bool vex = false, xop = false, evex = false; > struct last_insn *last_insn; > > @@ -12104,6 +13087,14 @@ s_insn (int dummy ATTRIBUTE_UNUSED) > last_insn->name = ".insn directive"; > last_insn->file = as_where (&last_insn->line); > > + /* PS: SCFI is enabled only for AMD64 ABI. The ABI check has been > + performed in i386_target_format. */ > + if (flag_synth_cfi) > + { > + ginsn = x86_ginsn_new (symbol_temp_new_now (), frch_ginsn_gen_mode ()); If you really want to use this function here, more cases will need handling (perhaps even beyond what I've commented on above). However, I'd strongly suggest splitting off the "unhandled" part of that function, and using only that here. After all you hardly know what exactly the programmer's intentions are. Because of that, you may also want to consider simply forbidding use of .insn when SCFI is to be generated. > + frch_ginsn_data_append (ginsn); > + } > + > done: > *saved_ilp = saved_char; > input_line_pointer = line; > @@ -15748,6 +16739,11 @@ i386_target_format (void) > else > as_fatal (_("unknown architecture")); > > +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF) > + if (flag_synth_cfi && x86_elf_abi != X86_64_ABI) > + as_fatal (_("Synthesizing CFI is not supported for this ABI")); > +#endif Elsewhere I've raised the question of whether we should really check OBJ_MAYBE_ELF anywhere in this file. However, as long as we do, you'll need to accompany that with an IS_ELF check in the if(). If, to address my unreachable code comment near the top, you elect to add further OBJ_ELF / OBJ_MAYBE_ELF checks, there you then wouldn't need that further check (as flag_synth_cfi set then implies ELF). Jan