Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading

From: Julien Thierry <jthierry@redhat.com>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: linux-kernel@vger.kernel.org, peterz@infradead.org,
	mbenes@suse.cz, raphael.gault@arm.com, benh@kernel.crashing.org
Subject: Re: [PATCH v2 9/9] objtool: Abstract unwind hint reading
Date: Fri, 31 Jul 2020 08:00:58 +0100	[thread overview]
Message-ID: <1a078563-001d-c666-d2f5-9291f0efd35a@redhat.com> (raw)
In-Reply-To: <20200730150341.udqnykbw7yfsjvin@treble>

On 7/30/20 4:03 PM, Josh Poimboeuf wrote:
> On Thu, Jul 30, 2020 at 10:46:52AM +0100, Julien Thierry wrote:
>> The type of unwind hints and the semantics associated with them depend
>> on the architecture. Let arch specific code convert unwind hints into
>> objtool stack state descriptions.
>>
>> Signed-off-by: Julien Thierry <jthierry@redhat.com>
>> ---
>>   tools/objtool/arch.h            |  5 +--
>>   tools/objtool/arch/x86/decode.c | 54 ++++++++++++++++++++++++++++++
>>   tools/objtool/cfi.h             |  3 +-
>>   tools/objtool/check.c           | 58 +++++----------------------------
>>   tools/objtool/orc_gen.c         |  4 ++-
>>   5 files changed, 71 insertions(+), 53 deletions(-)
>>
>> diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
>> index 2e2ce089b0e9..44107e9aab71 100644
>> --- a/tools/objtool/arch.h
>> +++ b/tools/objtool/arch.h
>> @@ -7,12 +7,11 @@
>>   #define _ARCH_H
>>   
>>   #include <stdbool.h>
>> +#include <linux/frame.h>
>>   #include <linux/list.h>
>>   #include "objtool.h"
>>   #include "cfi.h"
>>   
>> -#include <asm/orc_types.h>
>> -
>>   enum insn_type {
>>   	INSN_JUMP_CONDITIONAL,
>>   	INSN_JUMP_UNCONDITIONAL,
>> @@ -86,4 +85,6 @@ unsigned long arch_dest_reloc_offset(int addend);
>>   
>>   const char *arch_nop_insn(int len);
>>   
>> +int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint);
>> +
>>   #endif /* _ARCH_H */
>> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
>> index 1967370440b3..2099809925af 100644
>> --- a/tools/objtool/arch/x86/decode.c
>> +++ b/tools/objtool/arch/x86/decode.c
>> @@ -6,6 +6,8 @@
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   
>> +#include <linux/frame.h>
>> +
>>   #define unlikely(cond) (cond)
>>   #include <asm/insn.h>
>>   #include "../../../arch/x86/lib/inat.c"
>> @@ -15,6 +17,7 @@
>>   #include "../../elf.h"
>>   #include "../../arch.h"
>>   #include "../../warn.h"
>> +#include <asm/orc_types.h>
>>   
>>   static unsigned char op_to_cfi_reg[][2] = {
>>   	{CFI_AX, CFI_R8},
>> @@ -583,3 +586,54 @@ const char *arch_nop_insn(int len)
>>   
>>   	return nops[len-1];
>>   }
>> +
>> +int arch_decode_insn_hint(struct instruction *insn, struct unwind_hint *hint)
>> +{
>> +	struct cfi_reg *cfa = &insn->cfi.cfa;
>> +
>> +	if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
>> +		insn->ret_offset = hint->sp_offset;
>> +		return 0;
>> +	}
>> +
>> +	insn->hint = true;
>> +
>> +	switch (hint->sp_reg) {
>> +	case ORC_REG_UNDEFINED:
>> +		cfa->base = CFI_UNDEFINED;
>> +		break;
>> +	case ORC_REG_SP:
>> +		cfa->base = CFI_SP;
>> +		break;
>> +	case ORC_REG_BP:
>> +		cfa->base = CFI_BP;
>> +		break;
>> +	case ORC_REG_SP_INDIRECT:
>> +		cfa->base = CFI_SP_INDIRECT;
>> +		break;
>> +	case ORC_REG_R10:
>> +		cfa->base = CFI_R10;
>> +		break;
>> +	case ORC_REG_R13:
>> +		cfa->base = CFI_R13;
>> +		break;
>> +	case ORC_REG_DI:
>> +		cfa->base = CFI_DI;
>> +		break;
>> +	case ORC_REG_DX:
>> +		cfa->base = CFI_DX;
>> +		break;
>> +	default:
>> +		WARN_FUNC("unsupported unwind_hint sp base reg %d",
>> +			  insn->sec, insn->offset, hint->sp_reg);
>> +		return -1;
>> +	}
>> +
>> +	cfa->offset = hint->sp_offset;
>> +	insn->cfi.hint_type = hint->type;
>> +	insn->cfi.end = hint->end;
>> +
>> +	insn->cfi.sp_only = hint->type == ORC_TYPE_REGS || hint->type == ORC_TYPE_REGS_IRET;
> 
> What does "sp" mean here in sp_only?
>

Stack pointer, like in CFI_SP. When objtool encounters one of these 
hints, it starts to only track the stack frame with the stack pointer 
(no BP, no drap register, no move to temporary registers). Just trying 
to make some sense of this corner case.

>> +		if (arch_decode_insn_hint(insn, hint)) {
>> +			WARN_FUNC("Bad unwind hint", insn->sec, insn->offset);
> 
> No need for a warning here, since the arch-specific function already
> prints one.
> 

Right.

Thanks,

-- 
Julien Thierry