Re: [Xen-devel] [PATCH v8.1 14/27] xsplice, symbols: Implement symbol name resolution on address.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> >+unsigned long xsplice_symbols_lookup_by_name(const char *symname)
> >+{
> >+    struct payload *data;
> 
> Do you need symbols other than those marked "new_symbol" past the loading
> of the module? If not, wouldn't it be worthwhile to shrink the symbol table 
> to just
> those, likely speeding up the lookup?

Ross hopefully answered that to your satisfaction?
> 
> >@@ -379,11 +405,129 @@ static int prepare_payload(struct payload *payload,
>          >for ( j = 0; j < ARRAY_SIZE(f->u.pad); j++ )
>              >if ( f->u.pad[j] )
>                  >return -EINVAL;
> >+
> >+        /* Lookup function's old address if not already resolved. */
> >+        if ( !f->old_addr )
> >+        {
> >+            f->old_addr = (void *)symbols_lookup_by_name(f->name);
> >+            if ( !f->old_addr )
> >+            {
> >+                f->old_addr = (void 
> >*)xsplice_symbols_lookup_by_name(f->name);
> 
> The two casts make me wonder whether the two functions shouldn't return
> void *, and then whether struct xsplice_symbol's value field shouldn't then
> perhaps be void * too.

So I did try that and it all worked nicely on x86. However on ARM32:

arm <konrad@localhost:~/xen/xen> make -j8 1>1
symbols.c: In function 'symbols_lookup_by_name':
symbols.c:287:20: error: cast to pointer from integer of different size
[-Werror=int-to-pointer-cast]

275     uint64_t addr = 0; /* MUST be initialized. */                           
    

286         if ( !strcmp(name, symname) )                                       
    
287             return (void *)addr; 

Which is rather unfortunate. I think I will have to make it unsigned
long.
> 
> >+                if ( !f->old_addr )
> >+                {
> >+                    dprintk(XENLOG_ERR, XSPLICE "%s: Could not resolve old 
> >address of %s\n",
> >+                            elf->name, f->name);
> >+                    return -ENOENT;
> >+                }
> >+            }
> >+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Resolved old address %s => 
> >%p\n",
> >+                    elf->name, f->name, f->old_addr);
> >+        }
>      >}
>  >
>      >return 0;
>  >}
>  
> So one thing I'm realizing only now: Is there no support for using 
> <symbol>+<offset>
> to fill ->old_addr?

No. Just <symbol>. I updated the design document to outline this missing
functionality in the Todo section.

> 
> >+static bool_t is_payload_symbol(const struct xsplice_elf *elf,
> >+                                const struct xsplice_elf_sym *sym)
> >+{
> >+    if ( sym->sym->st_shndx == SHN_UNDEF ||
> >+         sym->sym->st_shndx >= elf->hdr->e_shnum )
> >+        return 0;
> >+
> >+    return (elf->sec[sym->sym->st_shndx].sec->sh_flags & SHF_ALLOC) &&
> >+            (ELF64_ST_TYPE(sym->sym->st_info) == STT_OBJECT ||
> >+             ELF64_ST_TYPE(sym->sym->st_info) == STT_FUNC);
> 
> I don't recall having seen a reply to the question on not allowing STT_NOTYPE 
> here.

Ross, could you elaborate a bit please on this?

> 
> >+static int build_symbol_table(struct payload *payload,
> >+                              const struct xsplice_elf *elf)
> >+{
..snip..
> >+
> >+    /* Recall that section @0 is always NULL. */
> >+    for ( i = 1; i < elf->nsym; i++ )
> >+    {
> >+            symtab[nsyms].new_symbol = 0; /* To be checked below. */
> 
> Why "checked"? The only thing happening further down is this possibly
> getting overwritten with 1.

I changed it to say "May be overwritten below."

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel