[PATCH, V2 07/10] gas: synthesize CFI for hand-written asm
Indu Bhagat
indu.bhagat@oracle.com
Tue Dec 19 07:07:32 GMT 2023
On 12/10/23 23:59, Jan Beulich wrote:
> 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.
>
I have now handled this case properly I think (will include this in V4).
Thanks for your comment below.
>>>>>> +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?
>
Yes, now a ginsn on type GINSN_TYPE_OTHER with destination REG_SP/REG_FP
as applicable is generated. The SCFI machinery then bails out if this
ginsn makes REG_FP untraceable, when REG_FP based CFA tracking is
active. If CFA tracking is _not_ REG_FP based, it is fine to manipulate
REG_FP in a untraceable way.
>>>>>> +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).
>
Thanks. We will now take the last reg operand, i.op[i.operands -
1].regs (after checks on i.operands and i.reg_operands) as destination
and if the register is REG_SP/REG_FP, we generate GINSN_TYPE_OTHER with
the appropriate destination register.
I will include this change in a V4 posting.
>>>>>> + 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?
I see that Push, Pop instructions do not need the REX prefix. Hence,
checking for 66H to detect 16-bit operand size suffices. Default
operand size is 64-bit in 64-bit mode.
More information about the Binutils
mailing list