Re: [PATCH, V2 07/10] gas: synthesize CFI for hand-written asm

[Indu Bhagat <indu.bhagat@oracle.com>]

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.