Re: [RFC 2/6] objtool: arm64: Add required implementation for supporting the aarch64 architecture in objtool.

[Josh Poimboeuf]

On Wed, Apr 24, 2019 at 04:16:26PM +0000, Raphael Gault wrote:
> On 4/23/19 9:18 PM, Josh Poimboeuf wrote:
> > On Tue, Apr 09, 2019 at 02:52:39PM +0100, Raphael Gault wrote:
> >> Provide implementation for the arch-dependent functions that are called by the main check
> >> function of objtool.
> >> The ORC unwinder is not yet supported by the arm64 architecture so we only provide a dummy
> >> interface for now.
> >> The decoding of the instruction is split into classes and subclasses as described into
> >> the Instruction Encoding in the ArmV8.5 Architecture Reference Manual.
> >
> > Where did the code for the decoder come from?  Was it written from
> > scratch?
> >
> 
> This decoder was indeed written from scratch based on the armv8 ARM. The
> automatic generation idea hasn't really been discussed yet.

Ok.  That's a lot of code.  Hopefully ARM folks can review it closely.

> >> +#ifndef __ASSEMBLY__
> >> +/*
> >> + * This struct is more or less a vastly simplified version of the DWARF Call
> >> + * Frame Information standard.  It contains only the necessary parts of DWARF
> >> + * CFI, simplified for ease of access by the in-kernel unwinder.  It tells the
> >> + * unwinder how to find the previous SP and BP (and sometimes entry regs) on
> >> + * the stack for a given code address.  Each instance of the struct corresponds
> >> + * to one or more code locations.
> >> + */
> >> +struct orc_entry {
> >> +s16sp_offset;
> >> +s16bp_offset;
> >> +unsignedsp_reg:4;
> >> +unsignedbp_reg:4;
> >> +unsignedtype:2;
> >> +unsignedend:1;
> >> +} __packed;
> >> +
> >> +/*
> >> + * This struct is used by asm and inline asm code to manually annotate the
> >> + * location of registers on the stack for the ORC unwinder.
> >> + *
> >> + * Type can be either ORC_TYPE_* or UNWIND_HINT_TYPE_*.
> >> + */
> >> +struct unwind_hint {
> >> +u32ip;
> >> +s16sp_offset;
> >> +u8sp_reg;
> >> +u8type;
> >> +u8end;
> >> +};
> >> +#endif /* __ASSEMBLY__ */
> >> +
> >> +#endif /* _ORC_TYPES_H */
> >
> > It seems odd to have the above header file in arm64 code, since it
> > doesn't implement ORC.  Is it really needed?
> >
> 
> The unwind_hint part can safely be removed. However the orc_entry seems
> to be needed since the struct instruction comports a struct orc_entry
> field. I have chosen to let it here as well but maybe a better approach
> is possible.

Ideally we can figure out a way to decouple 'struct instruction' from
'struct orc_entry'.  But yes, I think your approach is fine for now.

Though I think using an arch-independent header file would be better, to
avoid creating duplicated (dead) code.

-- 
Josh