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

From: "Jan Beulich" <JBeulich@suse.com>
To: Konrad Rzeszutek Wilk <konrad@kernel.org>
Cc: keir@xen.org, andrew.cooper3@citrix.com, mpohlack@amazon.com,
	Ross Lagerwall <ross.lagerwall@citrix.com>,
	xen-devel@lists.xenproject.org, sasha.levin@oracle.com
Subject: Re: [PATCH v8.1 14/27] xsplice, symbols: Implement symbol name resolution on address.
Date: Mon, 25 Apr 2016 02:02:48 -0600	[thread overview]
Message-ID: <571DEB4802000078000E5279@prv-mh.provo.novell.com> (raw)
In-Reply-To: <20160424214108.GA9784@localhost.localdomain>

>>> On 24.04.16 at 23:41, <konrad@kernel.org> wrote:
> I spoke over the weekend with Dan Jacobowitz (who in the past worked 
> on binutils) who mentioned that ELF visibility is what one should
> pay attention to. And that we shouldn't need to look at LOCAL symbols
> at all if we are resolving between objects.

Well, in general that's correct, but xSplice intentionally hooks up to
all the symbols available (including local ones in the core image).

> Also he mentions that the  handling of .L* symbols is special since
> they're intermediate artifacts of assembly.  They're ... "even more local".
> 
> Digging in the binutils over the weekend I found:
> 
> /* Return whether a symbol name implies a local symbol.  Most targets
>    use this function for the is_local_label_name entry point, but some
>    override it.  */
> 
> 
> _bfd_elf_is_local_label_name (bfd *abfd ATTRIBUTE_UNUSED,
>                               const char *name)
> {
>   /* Normal local symbols start with ``.L''.  */
>   if (name[0] == '.' && name[1] == 'L')
>     return TRUE; 
> ..

Yes, that's then what we want to match in behavior (as also
suggested by Ross in his reply).

> Looking at the callchain I saw:
> 
> /* Link an input file into the linker output file.  This function
>    handles all the sections and relocations of the input file at once.
>    This is so that we only have to read the local symbols once, and
>    don't have to keep them in memory.  */
> 
> static bfd_boolean
> elf_link_input_bfd (..
> 
> And inside there is a big loop in which:
> 
>    /* See if we are discarding symbols with this name.  */
>       if ((flinfo->info->strip == strip_some
>            && (bfd_hash_lookup (flinfo->info->keep_hash, name, FALSE, FALSE)
>                == NULL))
>           || (((flinfo->info->discard == discard_sec_merge
>                 && (isec->flags & SEC_MERGE) && !flinfo->info->relocatable)
>                || flinfo->info->discard == discard_l)
>               && bfd_is_local_label_name (input_bfd, name)))
>         continue;
> 
> Which really matches with what I see - that is the .LCx symbols are in
> mergable sections:
> [...]
> So coming back to this discussion - the code has this:
> 
> 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);
> }
> 
> You OK with it being the way it is? I would naturally include a 
> comment about these local symbols like .L*

But that's what is there already, or am I overlooking some change
you did to it? I.e. STT_NOTYPE are still not being considered, and
there's still no dependency on the .L name prefix.

Jan

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