Messages in this thread | | | Date | Wed, 24 Apr 2019 11:23:24 -0500 | From | Josh Poimboeuf <> | Subject | Re: [RFC 2/6] objtool: arm64: Add required implementation for supporting the aarch64 architecture in objtool. |
| |
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
| |