[Ltrace-devel] [PATCH 6/8] mipsel: Add mips specific symbol info loading

[Edgar E. Iglesias]

On Thu, Sep 27, 2012 at 01:31:53AM +0200, Petr Machata wrote:
> edgar.iglesias at gmail.com writes:
> > @@ -131,6 +163,9 @@ arch_elf_init(struct ltelf *lte, struct library *lib)
> >  		}
> >  	}
> >  
> > +	/* Tell the generic code how many dynamic trace:able symbols
> > +	 * we've got.  */
> > +	lte->relplt_count = lte->dynsym_count - lte->arch.mips_gotsym;
> 
> Hmm, I'm not too fond of backend code overwriting core values like that,
> but I guess it's OK.  It's in the right hook anyway.  It's not worth it
> to build a separate hooklet for this.
> 
> >  	return 0;
> >  }
> >  
> > @@ -139,4 +174,128 @@ arch_elf_destroy(struct ltelf *lte)
> >  {
> >  }
> >  
> > +enum callback_status cb_each_sym(struct library_symbol *libsym, void *data)
> 
> This should be static.  cb_each_sym should start a new line.
> 
> > +{
> > +	struct Process *proc = data;
> > +	struct breakpoint *bp;
> > +
> > +	if (libsym->plt_type != LS_TOPLT_GOTONLY)
> > +		return CBS_CONT;
> > +
> > +	/* Update state.  */
> > +	libsym->arch.resolved_addr = (uintptr_t) sym2addr(proc, libsym);
> > +
> > +	if (!libsym->arch.resolved_addr)
> 
> Use libsym->arch.resolved_addr == 0.  It's an address, not a boolean.  I
> know this idiom is occasionally used in ltrace, but I can't stand it.
> 
> > +		/* FIXME: What does this mean?  */
> > +		return CBS_CONT;
> > +
> > +	libsym->arch.type = RESOLVED;
> > +
> > +	/* Add breakpoint.  */
> > +	bp = malloc(sizeof *bp);
> > +	if (!bp
> 
> Use bp == NULL.
> 
> > +	    || breakpoint_init(bp, proc,
> > +			       (void *) (uintptr_t) libsym->arch.resolved_addr,
> 
> The double cast shouldn't be necessary here.  uintptr_t already is
> void*, and when we convert it to integral, breakpoint_init will be
> adjusted as well.

Hi,

It seems to be that you have confused uintptr_t with arch_addr_t i your
response?

The issue in this case is that resolved_addr is a GElf_addr, 64bit integral.
breakpoint init wants a pointer type, so I need to first transform the
64bit intergral value into a 32bit integral value and then cast it to
t. By using uintptr_t as the intermediate integral type, I think the code
will be reusable for both 32 and 64 bit mips archs.

I've added the same comment at you suggest a bit further down this email,
it's actually the exact same types being involved.


> 
> > +			       libsym) < 0) {
> > +		goto fail;
> > +	}
> > +
> > +	if (proc_add_breakpoint(proc, bp) < 0) {
> > +		breakpoint_destroy(bp);
> > +		goto fail;
> > +	}
> > +
> > +	if (breakpoint_turn_on(bp, proc) < 0) {
> > +		proc_remove_breakpoint(proc, bp);
> > +		breakpoint_destroy(bp);
> > +		goto fail;
> > +	}
> > +
> > +	return CBS_CONT;
> > +fail:
> > +	free(bp);
> > +	fprintf(stderr, "Failed to add breakpoint for %s\n", libsym->name);
> > +	return CBS_FAIL;
> > +}
> > +
> > +enum callback_status cb_each_lib(struct Process *proc,
> > +				 struct library *lib, void *data)
> 
> Static, newline.
> 
> > +{
> > +	struct library_symbol *libsym = NULL;
> > +	while ((libsym = library_each_symbol(lib, libsym,
> > +					     cb_each_sym, proc)) != NULL) {
> 
> Hmm, above you could just return CBS_CONT instead of CBS_FAIL, and
> change this into a simple call.

Nice cleanup, thanks.



> > +	}
> > +	return CBS_CONT;
> > +}
> > +
> > +void arch_dynlink_done(struct Process *proc)
> > +{
> > +	proc_each_library(proc, NULL, cb_each_lib, NULL);
> > +}
> > +
> > +enum plt_status
> > +arch_elf_add_plt_entry(struct Process *proc, struct ltelf *lte,
> > +                       const char *a_name, GElf_Rela *rela, size_t ndx,
> > +                       struct library_symbol **ret)
> > +{
> > +	struct library_symbol *libsym = NULL;
> > +	arch_addr_t bp_addr;
> > +	char *name = NULL;
> > +	int sym_index = ndx + lte->arch.mips_gotsym;
> > +
> > +	libsym = malloc(sizeof(*libsym));
> > +	if (!libsym)
> > +		return plt_fail;
> 
> if (libsym == NULL)
> 
> > +
> > +	GElf_Addr addr = arch_plt_sym_val(lte, sym_index, 0);
> > +
> > +	name = strdup(a_name);
> > +	if (library_symbol_init(libsym,
> > +				(arch_addr_t) (uintptr_t) addr,
> 
> Please add a comment like this:
> 
> 		/* XXX The double cast should be removed when
> 		 * arch_addr_t becomes integral type.  */
> 
> When I get around to it, I'll grep for "double cast" to find individual
> cases.
> 
> > +				name, 1, LS_TOPLT_EXEC) < 0) {
> > +		fprintf(stderr, "%s: failed %s : %llx\n", __func__, name, addr);
> > +		goto fail;
> > +	}
> > +
> > +	bp_addr = sym2addr(proc, libsym);
> > +	libsym->arch.stub_addr = (uintptr_t) bp_addr;
> > +
> > +	if (!bp_addr) {
> > +		/* Function pointers without PLT entries.  */
> > +		libsym->plt_type = LS_TOPLT_GOTONLY;
> > +		libsym->arch.type = UNRESOLVED;
> > +	}
> 
> if (bp_addr == 0)
> 
> With these nits fixed, I'll apply this patch.
> 
> Thanks,
> PM