From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) by sourceware.org (Postfix) with ESMTPS id 685663858D32 for ; Mon, 11 Dec 2023 07:59:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 685663858D32 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 685663858D32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::336 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702281565; cv=none; b=oeyEJQ2DmGkjgrSRUOwdsCUZxmSROKiIZ4uSfVkucJ+2c4nxqP+N45doIDSWQ04ykaLuakHqsYfICyYdoAC0khQ7cRBFEX0zuAZ7ckO5yhuBko0e7kZ2KPxIKdF9Z0EQe0Tc6l9QPu/ZddH/1FVt3aKxCxTLZPHsZPdamSkDuvI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1702281565; c=relaxed/simple; bh=plo+s8NbCs4eDTAVoYLS5OqiSs53sqKnxsy3oiwaRas=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=XM+MerAVCgvw3RrW544HOqprtV2WdIcsVwP4wpiWzgZqVxUleyGLfzUWEMmEnLjQhgoVKwGMrPWvPzbJtRB2EvOqF5jpTYqZsyfO5Xt5X96IV/gmFaMhPjhwof75+VmvB4ZYbD0n7/ENBnWEc5SwZTyD+I/rAqUgjyogej52pls= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-x336.google.com with SMTP id 5b1f17b1804b1-40c3ceded81so17594695e9.1 for ; Sun, 10 Dec 2023 23:59:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1702281561; x=1702886361; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:autocrypt:from:references:cc :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=lQHKVm3gnVDk5jTMzc+bOXdoCJSsoSYr7wVwJTNwiko=; b=H6e8m5cAuUbw2/kpLWwIRD9rv9eGqnpsS3pgQtl7plZ4+O1/jMt1nHg4jTthMvlDzi GzL5dLJO5+wjkt3WLMRwyBF9NPFJ7PFe7VA+iYG8Qipy44hXicpPDrPRzWzWKFGsZ+ac vV/awjdW2DZQ6/6iUFPiuskd+bkw8VZFu+IAkgJ6n0p3J+2mngBNVucFd57pJ/4mbPnh K71L/SLDt7otg/Or+A3G9v2/P7FqhpRZEUKz0LjQW+zlpI4L1WGBGepteHPYWZ1zxTbw tkVC7e61jk5PL5W3fJzWXbzj+r8YY9stmFcGEhs/6Yh6o9aL3pRVN+VnUBQEN0t8Rqoi NdYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702281561; x=1702886361; h=content-transfer-encoding:in-reply-to:autocrypt:from:references:cc :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=lQHKVm3gnVDk5jTMzc+bOXdoCJSsoSYr7wVwJTNwiko=; b=UcY3WD1PEgvpQ5gsccIy5/7TJTspFo/1XUZQCfa+0qPx2QR6Jim/DgWLdwJzdlSFJx G4rECZaHIzb+oGuJzqOml/nbHoydQNkrqDnOvxolWan5Imy5UuhlnH49npjQtU1YyyrJ G5ICLyL62UUvEX0VxPsE81eV/xr01/P3BAcb2bVvK4KsGwYSpdmDYuYqubOz7Nl6nxlY GaCRNYzLQLDOYD7ipcXh4bDxpAPuDmFQ+AD09vJEtH7N2nEA9Mbr5jI/juvV2TTlu9us 6kn2wsbgFHKQA2AJK0GaQAtviFCNJZsyvrmT2AAhCPpQ028suECx0Bg1dbJaY++As3TR G0GQ== X-Gm-Message-State: AOJu0YzooH1uXdnw1anDGzvzabRJGGyBuu9w1caYcYBY8qcAQhEyZ6xm eLbKSnVMz/zw2A9H+U8Py7tU X-Google-Smtp-Source: AGHT+IGHuE23nkzbCgmvBPfRK+cai72HcyAvP6wQAU3KIs+EZeVzOiHUJF3C5g/35IPccVXjmY7r1Q== X-Received: by 2002:a7b:cd0a:0:b0:40c:2305:f0f4 with SMTP id f10-20020a7bcd0a000000b0040c2305f0f4mr1714731wmj.85.1702281560935; Sun, 10 Dec 2023 23:59:20 -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 b16-20020a05600c4e1000b0040c310abc4bsm12420570wmq.43.2023.12.10.23.59.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 10 Dec 2023 23:59:20 -0800 (PST) Message-ID: Date: Mon, 11 Dec 2023 08:59:02 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH, V2 07/10] gas: synthesize CFI for hand-written asm Content-Language: en-US To: Indu Bhagat Cc: binutils@sourceware.org References: <20231030165137.2570939-1-indu.bhagat@oracle.com> <20231030165137.2570939-8-indu.bhagat@oracle.com> <7a101e5c-fe1d-4b3f-afdb-0ee11cf6a7c9@oracle.com> <060736aa-3a4f-320c-d0f6-37512be2e216@suse.com> <9ca4fc45-45b0-44f8-b9bc-3151f387d10a@oracle.com> 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: <9ca4fc45-45b0-44f8-b9bc-3151f387d10a@oracle.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3026.5 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 10.12.2023 11:22, Indu Bhagat wrote: > On 11/2/23 04:39, Jan Beulich wrote: >> On 02.11.2023 09:15, Indu Bhagat wrote: >>> On 10/31/23 07:10, Jan Beulich wrote: >>>> On 30.10.2023 17:51, Indu Bhagat wrote: >>>>> gas/ >>>>> * Makefile.am: Add new files. >>>>> * Makefile.in: Regenerated. >>>>> * as.c (defined): Handle documentation and listing option for >>>>> ginsns and SCFI. >>>>> * config/obj-elf.c (obj_elf_size): Invoke ginsn_data_end. >>>>> (obj_elf_type): Invoke ginsn_data_begin. >>>>> * config/tc-i386.c (ginsn_new): New functionality to generate >>>>> ginsns. >>>>> (x86_scfi_callee_saved_p): New function. >>>>> (ginsn_dw2_regnum): Likewise. >>>>> (ginsn_set_where): Likewise. >>>>> (x86_ginsn_alu): Likewise. >>>>> (x86_ginsn_move): Likewise. >>>>> (x86_ginsn_lea): Likewise. >>>>> (x86_ginsn_jump): Likewise. >>>>> (x86_ginsn_jump_cond): Likewise. >>>>> (md_assemble): Invoke ginsn_new. >>>>> (s_insn): Likewise. >>>>> (i386_target_format): Add hard error for usage of --scfi with non AMD64 ABIs. >>>>> * config/tc-i386.h (TARGET_USE_GINSN): New definition. >>>>> (TARGET_USE_SCFI): Likewise. >>>>> (SCFI_NUM_REGS): Likewise. >>>>> (REG_FP): Likewise. >>>>> (REG_SP): Likewise. >>>>> (SCFI_INIT_CFA_OFFSET): Likewise. >>>>> (SCFI_CALLEE_SAVED_REG_P): Likewise. >>>>> (x86_scfi_callee_saved_p): Likewise. >>>> >>>> For this arch-specific code there's a fundamental question of maintenance >>>> cost here: It doesn't look very reasonable to me to demand of people adding >>>> support for new ISA extensions to also take into consideration all of this >>>> new machinery. Yet if any such addition affects SCFI, things will go out- >>>> of-sync without updating this code as well. It may not be very often that >>>> such updating is necessary, but right now there's APX work in progress >>>> which I expect will need dealing with here as well. >>>> >>> >>> I understand your concerns. FWIW, for SCFI, we will need to translate >>> only a subset of instructions into ginsns (change of flow insns, >>> save/restore and arith on REG_SP/REG_FP). >> >> Considering what you say further down regarding untraceability, I'm afraid >> you will need to care about all insns which have SP/FP as their destination. >> > > Yeah, All insns which have SP/FP as their destination must be seen by > the SCFI machinery. If any instruction which has SP/FP as destination is > missed, and the user has specified a --scfi command line argument, my > thinking is that GAS should spit out an error or a diagnostic. > > That said, the current implementation handles the most commonly used > instructions to manage stack pointer for dynamic and static stack > allocation. For unhandled instructions which end up _possibly_ affecting > REG_FP/REG_SP (I say _possibly_ because at this time I check both > i.op[0].regs and i.op[1].regs for REG_SP/REG_FP), there is a warning: > "Warning: SCFI: unhandled op 0x63 may cause incorrect CFI" Yet neither i.op[0] nor i.op[1] may represent the destination register of an insn, when there are more than 2 operands. >>>>> +static ginsnS * >>>>> +x86_ginsn_alu (i386_insn insn, symbolS *insn_end_sym) >>>>> +{ >>>>> + offsetT src_imm; >>>>> + uint32_t dw2_regnum; >>>>> + enum ginsn_src_type src_type; >>>>> + enum ginsn_dst_type dst_type; >>>>> + ginsnS *ginsn = NULL; >>>>> + >>>>> + /* FIXME - create ginsn for REG_SP target 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 imm, %reg. >>>>> + and imm, %reg only at this time for SCFI. */ >>>>> + if (!(insn.tm.extension_opcode == 0 >>>>> + || insn.tm.extension_opcode == 4 >>>>> + || insn.tm.extension_opcode == 5)) >>>>> + return ginsn; >>>> >>>> Why is AND permitted, but OR/XOR aren't? >>>> >>>> Also this is about ALU insns with immediate operands only, yet that >>>> fact isn't reflected in the function name. >>>> >>> >>> OR/XOR should be handled. I will fix this. >>> > > Spoke too soon. I remembered later why OR/XOR is not handled but AND is > (I have now added a code comment around this). > > An OR/XOR on REG_SP/REG_FP as destination makes the destination > untraceable. So these operations are skipped here. At a later point > (towards the end of x86_ginsn_new): the ginsn generation machinery will > complain about these OR/XOR as "unhandled opcode for SCFI" and bail out > if destination was REG_SP/REG_FP. > > On that note, AND operation also makes REG_SP/REG_FP untraceable, but > they are being generated currently. This is because supporting DRAP > pattern is something we plan to look into. Currently these are > generated, but the SCFI machinery later bails out with a message "Error: > SCFI: unsupported stack manipulation pattern" But why would you need special handling just to mark a register untraceable? Generic code merely looking at the destination register ought to be enough? >>>>> +static ginsnS * >>>>> +x86_ginsn_lea (i386_insn insn, symbolS *insn_end_sym) >>>>> +{ >>>>> + offsetT src_disp = 0; >>>>> + ginsnS *ginsn = NULL; >>>>> + uint32_t base_reg; >>>>> + uint32_t index_reg; >>>>> + offsetT index_scale; >>>>> + uint32_t dst_reg; >>>>> + >>>>> + if (!insn.index_reg && !insn.base_reg) >>>>> + { >>>>> + /* lea symbol, %rN. */ >>>>> + dst_reg = ginsn_dw2_regnum (insn.op[1].regs); >>>>> + /* FIXME - Skip encoding information about the symbol. >>>>> + This is TBD_GINSN_INFO_LOSS, but it is fine if the mode is >>>>> + GINSN_GEN_SCFI. */ >>>>> + ginsn = ginsn_new_mov (insn_end_sym, false, >>>>> + GINSN_SRC_IMM, 0xf /* arbitrary const. */, 0, >>>>> + GINSN_DST_REG, dst_reg, 0); >>>>> + } >>>>> + else if (insn.base_reg && !insn.index_reg) >>>>> + { >>>>> + /* lea -0x2(%base),%dst. */ >>>>> + base_reg = ginsn_dw2_regnum (insn.base_reg); >>>>> + dst_reg = ginsn_dw2_regnum (insn.op[1].regs); >>>>> + >>>>> + if (insn.disp_operands) >>>>> + src_disp = insn.op[0].disps->X_add_number; >>>> >>>> What if the displacement expression is other than O_constant? >>>> >>> >>> For SCFI, a "complex" lea instruction will imply some untraceable change >>> to the destination reg. For SCFI, the extent of untraceable change is >>> not of interest, hence, in general, some information loss in ginsn is >>> tolerable. >> >> As a fundamental request: For anything that's of no interest, can you >> please try to cover this with as little (and thus clear) code as possible? >> No taking apart of sub-cases when those are indifferent in the end anyway. >> > > In general, this is something I have battled with on and off. In the end > I chose the principle "Encode as precise ginsn as possible given its > current representation". I have tried to follow this mostly > (x86_ginsn_lea is the known outlier). For all cases known to be > imprecise, it is my intention to have them marked with > TBD_GINSN_INFO_LOSS etc. And to not have too many of those.. > > If I leave out more sub-cases (elsewhere) because they are indifferent > in the end for SCFI at this time, two problems: > - More of the currently generated ginsn will look imprecise. > - More code will need to be revisited when there is other ginsn > generation modes to be supported. I can see your view. Still there's the other view of code not really being needed right now potentially also net being in the exact shape it may be needed down the road, at which point it would similarly need touching again. >>>>> +{ >>>>> + uint16_t opcode; >>>>> + uint32_t dw2_regnum; >>>>> + uint32_t src2_dw2_regnum; >>>>> + int32_t gdisp = 0; >>>>> + ginsnS *ginsn = NULL; >>>>> + ginsnS *ginsn_next = NULL; >>>>> + ginsnS *ginsn_last = NULL; >>>>> + >>>>> + /* FIXME - Need a way to check whether the decoding is sane. The specific >>>>> + checks around i.tm.opcode_space were added as issues were seen. Likely >>>>> + insufficient. */ >>>> >>>> Furthermore opcode encoding space (SPACE_...) need to be taken into >>>> account in all cases. >>>> >>>>> + /* Currently supports generation of selected ginsns, sufficient for >>>>> + the use-case of SCFI only. To remove this condition 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 >>>>> + examples. */ >>>>> + if (gmode != GINSN_GEN_SCFI) >>>>> + return ginsn; >>>>> + >>>>> + opcode = i.tm.base_opcode; >>>>> + >>>>> + switch (opcode) >>>>> + { >>>>> + case 0x1: >>>>> + /* add reg, reg. */ >>>>> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >>>> >>>> You don't care about opcode 0 (byte operation). Then what about 16-bit >>>> operand size? Or, since we're talking of a 64-bit-ABI-only feature, >>>> even 32-bit operand size? >>>> >>> >>> Operand size in some cases does not affect SCFI. So we dont keep >>> information about operand sizes. The case that I should handle operand >>> size is when they are pushed to / popped from stack. >> >> So you care about recognizing when %rbp is overwritten by an insn. And >> you also care about the same for %ebp and %bp. In that sense operand >> size may indeed be unnecessary to determine. Except that then you also >> want to deal with byte operations, i.e. %bpl being the destination. >> > > I have adjusted the ginsn_dw2_regnum to handle correctly 8-bit register > usages so that any writes to REG_SP/REG_FP are detected. I also added a > new test "ginsn-dw2-regnum" to exercise this API. > >>>> Also what about opcode 0x3? >>> >>> LSL instruction ? It doesnt look like it can affect DWARF CFI of a >>> function. Please correct me if this is wrong. >> >> LSL is 0f03, i.e. opcode 0x3 in SPACE_0F. What my comment was about is >> ADD with inversed operands (ModRM.rm soure, ModRM.reg destination). All >> four of these flavors of ADD are covered by a single template, using the >> D and W attributes to establish opcode bits 0 and 1 based on actual >> operands (and possibly pseudo-prefixes). >> > > I added handling for 0x3 ADD. > > Can you elaborate on the "using the D and W attributes to establish > opcode bits 0 and 1 based on actual operands (and possibly > pseudo-prefixes)." a bit ? Well, reg <-> reg/mem ADDs (taking the specific example) come in two forms: ModR/M.reg reprsenting the source and the same representing the destination. They're represented by a single opcode template, with D indicating that both directions can be used. Similarly there are multiple operand sizes supported by several such templates, with W indicating that both byte nad word/dword/qword operation is possible to encode by OR-ing in a single opcode bit. For your purposes, W is likely of no direct interest. D may be, yet then I take it that you work on actual operands, not on what templates permit. In actual operands, destination (due to following to AT&T syntax layout in the internal representation) is last at all stages of assembly (for Intel syntax input after swapping operands, of course). >>>>> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >>>>> + /* push fs / push gs. */ >>>>> + ginsn = ginsn_new_sub (insn_end_sym, false, >>>>> + GINSN_SRC_REG, REG_SP, 0, >>>>> + GINSN_SRC_IMM, 0, 8, >>>> >>>> Note how the 8 here assumes flag_code == CODE_64BIT. (You further assume >>>> no operand size override here.) >>>> >>> >>> Hmm. Yes, I do assume no operand size override here. I will need to >>> understand how to calculate the operand size here. Looks like I need to >>> check for instruction prefixes 66H or REX.W. Is there an API in the >>> backend that be used for this ? >> >> i.prefixes[] and (depending on the phase of assembly) i.rex should be >> telling you. >> > > I have added checks for prefix 66H to detect 16-bit ops now. IIUC, I > dont need to detect REX.W prefix specifically. I'm not sure. REX.W clear likely means the destination becomes untraceable? Whereas REX.W set means to full register is updated (upper half not zeroed) and hence it may remain traceable? Jan