Re: [PATCH v3 10/10] perf: perf interface for uprobes.

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

> 
> Could you also rebase your patch on the latest tip tree? :)
> (and remove die() too)

I was aware that I have to rebase the patch to the latest tip.
However I wanted to get a working set out.
Please expect the rebased version in the next version of the posting.

> 
> > Here is a terminal snapshot of placing, using and removing a probe on a
> > process with pid 3329 (corresponding to zsh)
> > 
> > [ Probing a function in the executable using function name  ]
> > -------------------------------------------------------------
> > [root@ABCD]# perf probe -p 3329 zfree@zsh
> 
> Hmm, I'm not sure why we have to specify both of PID and exec name.
> Is that possible to give a symbol as below? (omit exec name)
> 
> # perf probe -p 3329 zfree

Yes, If the symbol name belongs to th execfile then we can omit the
execfile part. However for symbols outside the execname, the
execfile(library file) is a must(atleast for now).

> 
> > Added new event:
> >   probe:p_3329_zfree                       (on 0x446420)
> 
> Hmm, the event name should be simpler, as like as zfree_3329.
> or, we might better make a new event group for each process, as like as
> 'probe_3329:zfree'
> 

Either zfree_3329 or having a new event group for each process is
possible.

> 
> 
> Hmm, if user gave --list with -p, what happened?
> We need to check a bad combination and warn it.
> 

Currently it would ignore -p option. i.e --list overrides.
We could either warn, or error out. What do you suggest?

> >  
> > +	if (session.pid)
> > +		goto end_dwarf;
> > +
> 
> Again, it must be checked that the combination of -p option and dwarf requirement.
> The latest code has perf_probe_event_need_dwarf(), so you can check it easier.
> 

Okay, 

> [...]
> > +	else
> > +		free(pp->function);
> > +	pp->function = zalloc(sizeof(char *) * 20);
> > +	if (!pp->function)
> > +		die("Failed to allocate memory by zalloc.");
> > +	e_snprintf(pp->function, 20, "0x%llx", vaddr);
> > +}
> 
> Well, after rebasing, you'll need to remove die()s from here and
> make it returns errors. 
> 

Okay. 


> [...]
> > @@ -383,7 +450,11 @@ int synthesize_trace_kprobe_event(struct probe_point *pp)
> >  	pp->probes[0] = buf = zalloc(MAX_CMDLEN);
> >  	if (!buf)
> >  		die("Failed to allocate memory by zalloc.");
> > -	ret = e_snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function, pp->offset);
> > +	if (pp->offset)
> > +		ret = e_snprintf(buf, MAX_CMDLEN, "%s+%d", pp->function,
> > +					pp->offset);
> > +	else
> > +		ret = e_snprintf(buf, MAX_CMDLEN, "%s", pp->function);
> >  	if (ret <= 0)
> >  		goto error;
> >  	len = ret;
> 
> Isn't it a cleanup ?

Can you please elaborate?

> 
> > +void add_trace_uprobe_events(struct probe_point *probes, int nr_probes,
> > +			     bool force_add, pid_t pid)
> > +{
> > +	int fd = open_uprobe_events(O_RDWR, O_APPEND);
> > +	add_trace_probe_events(fd, probes, nr_probes, force_add, pid);
> > +}
> 
> Please close fd in the function which opened it.
> 

Okay.

> 
> >  	memset(&sym, 0, sizeof(sym));
> > -	if (!self->kernel) {
> > +	if (self->kernel || symbol_conf.sort_by_name)
> > +		self->adjust_symbols = 0;
> > +	else {
> >  		self->adjust_symbols = (ehdr.e_type == ET_EXEC ||
> >  				elf_section_by_name(elf, &ehdr, &shdr,
> >  						     ".gnu.prelink_undo",
> >  						     NULL) != NULL);
> > -	} else self->adjust_symbols = 0;
> > +	}
> >  
> >  	elf_symtab__for_each_symbol(syms, nr_syms, idx, sym) {
> >  		struct symbol *f;
> 
> This one changes the symbol.c, so I think you'd better make a
> separate patch for this change.

Okay, I will separate it in the next version.
Thanks Masami for the review.

--
Thanks and Regards
Srikar