Re: [PATCH v2 2/5] x86/uprobes: implement x86 specific arch_uprobe_*_step

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 08/13/2012 03:24 PM, Oleg Nesterov wrote:
> On 08/09, Sebastian Andrzej Siewior wrote:
>>
>> v1..v2: re-use auprobe->fixups for fixups
>
> Yes, but
>
>> @@ -46,6 +46,8 @@ struct arch_uprobe_task {
>>   #ifdef CONFIG_X86_64
>>   	unsigned long			saved_scratch_register;
>>   #endif
>> +#define UPROBE_CLEAR_TF			(1<<  0)
>> +	unsigned int			restore_flags;
>>   };
>
> this patch still adds restore_flags into arch_uprobe_task.

Yes, but

>>   static void prepare_fixups(struct arch_uprobe *auprobe, struct insn *insn)
>>   {
>> -	bool fix_ip = true, fix_call = false;	/* defaults */
>> +	bool fix_ip = true, fix_call = false, fix_tf = false;	/* defaults */
>>   	int reg;
>>
>>   	insn_get_opcode(insn);	/* should be a nop */
>>
>>   	switch (OPCODE1(insn)) {
>> +	case 0x9d:
>> +		/* popf */
>> +		fix_tf = true;
>> +		break;
>>   	case 0xc3:		/* ret/lret */
>>   	case 0xcb:
>>   	case 0xc2:
>> @@ -277,6 +284,8 @@ static void prepare_fixups(struct arch_uprobe *auprobe, struct insn *insn)
>>   		auprobe->fixups |= UPROBE_FIX_IP;
>>   	if (fix_call)
>>   		auprobe->fixups |= UPROBE_FIX_CALL;
>> +	if (fix_tf)
>> +		auprobe->fixups |= UPROBE_TF_CHANGES;
>>   }
>
> I won't insist, but do we really need fix_tf? "case 0x9d" could simply
> add UPROBE_TF_CHANGES.

if it is not 0x9d (in most cases) we need to decide on per-process
basis (not per-breakpoint) whether the task has gdb watching it or not.
So this code is evaluated once (by the time the breakpoint is
installed) but it may be executed two times: once with gdb and once
without it. On first execution the SIGTRAP will wakeup gdb, on the
second the SIGTRAP will terminate the program because there is no TRAP
handler installed.

> Oleg.

Sebastian