On Mon, 2009-11-30 at 14:38 -0800, David Brownell wrote:
> On Monday 30 November 2009, Zachary T Welch wrote:
> > Adds a fully-documented API for dynamically loading libraries and
> > looking up symbols or addresses in them.
>
> Seems to me I've seen one or two or three or four of these
> before ... must there be another?? I know that portability
> of such things to non-UNIXey environments has been painful.
I was avoiding adding any new external dependencies. I bet that's why
you've seen N of them. I added it now to improve the stack tracing, but
I think we are getting to the point where loadable modules could be
possible. This is definitely part of that strategic plan.
> I'm used to seeing systems either stick to the "libdl" stuff,
> or use some multiplatform beast which tracks all of the
> platform-specific annoyances.
Ummm... this is libdl, isn't it? That's what the man page for these
APIs tell me to link against..... That library is already included in
most of our use cases, though I admit that I haven't tried these patches
without any drivers enabled (i.e. a "minimal" build).
> Also, I'm a bit uneasy at the notion of adding this, so I hope
> this doesn't merge unless it gets a lot better understood.
I am not in any hurry to push it. I wanted it for my own use.
It's nicely encapsulated, si I will see virtually no merge conflicts
while I maintain this in my tree.
> What's the intended change which will have us loading
> modules? What modules would be loaded?
JTAG interfaces, flash devices, targets.... Any of those things could
be turned into loadable modules, so only the features used are resident.
>
> Plus:
>
> > +struct module_symbol *module_symbol_by_addr(void *addr)
> > +{
> > + struct module_symbol *sym = calloc(1, sizeof(*sym));
> > + if (NULL == sym)
> > + return NULL;
> > +
> > + Dl_info info;
> > + int retval = dladdr(addr, &info);
>
> As noted, not very portable ...
Yes. I can trivially split the non-portable portions of the API into a
module_<imp>.c file, only one of which would be included. This allows
for libdl, Win32/LoadLibrary, or any other implementation required.
> > + if (0 == retval)
> > + {
> > + free(sym);
> > + return NULL;
> > + }
> > +
> > + sym->so_name = info.dli_fname;
> > + sym->so_addr = info.dli_fbase;
> > + sym->sym_name = info.dli_sname;
> > + if (NULL != sym->sym_name) {
> > + sym->sym_addr = info.dli_saddr;
> > + sym->sym_offset = addr - sym->sym_addr;
> > + } else {
> > + sym->sym_addr = addr;
> > + sym->sym_offset = 0;
>
> This looks wrong/dangerous. Surely better to just fail?
It works and produces reasonable traces.
> > + }
> > +
> > + if (NULL == sym->so_name)
> > + sym->so_name = "<unknown>";
> > + if (NULL == sym->sym_name)
> > + sym->sym_name = "<unknown>";
>
> ... and this even more so; those names aren't valid.
True. Better alternatives?
> > + return sym;
> > +}
> > +
> > +struct module_symbol *module_symbol_by_name(struct module_instance *module,
> > + const char *name)
> > +{
> > + void *addr = dlsym(module->dlhandle, name);
>
> But dlsym can fail...
And returns NULL, which is an address that will be looked up (below)...
That function will fail "safely", excepting the issues you note above.
I am not defending this API... just saying that it works
> > + return module_symbol_by_addr(addr);
> > +}
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
