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

[Jan Beulich <jbeulich@suse.com>]

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