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

2016-04-25 Thread Jan Beulich
>>> On 25.04.16 at 08:41,  wrote:
> On 04/22/2016 10:10 PM, Konrad Rzeszutek Wilk wrote:
>>> As per my earlier reply to Konrad, there must be more to this. I.e.
>>> "normal" local symbols won't get dropped together with relocations
>>> referencing them getting resolved.
>>
>> Correct. These .LCx symbols only cover .rodata.* sections. Any other
>> local symbols:
>>
>> [konrad@x230 x86]$ readelf --symbols prelink.o |grep bad_hypercall
>>   8946: 00145549 0 NOTYPE  LOCAL  DEFAULT1 
> compat_bad_hypercall
>>   8967: 00145c79 0 NOTYPE  LOCAL  DEFAULT1 bad_hypercall
>>
>> don't get dropped and do show up in the binary (nm --defined finds them).
>>
>> While .LC matches the type:
>>   9147: 08c0 0 NOTYPE  LOCAL  DEFAULT   33 .LC5
>>
>> They are in four other sections:
>> konrad@x230 x86]$ readelf --symbols prelink.o |grep LC | awk '{print
>> $7}' | sort | uniq
>> 22
>> 23
>> 33
>> 34
>>
>> [22] .rodata.str1.1PROGBITS   0019d500
>>a088  0001 AMS   0 0 1
>>   [23] .rodata.str1.8PROGBITS   001a7588
>>00020a31  0001 AMS   0 0 8
>>   [33] .init.rodata.str1 PROGBITS   001d5a78
>>0a1e  0001 AMS   0 0 1
>>   [34] .init.rodata.str1 PROGBITS   001d6498
>>2331  0001 AMS   0 0 8
>>
> 
> With some helpful investigation by Konrad, I've found this snippet in 
> binutils:
> 
>/* 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)
>   && !bfd_link_relocatable (flinfo->info))
> || flinfo->info->discard == discard_l)
> && bfd_is_local_label_name (input_bfd, name)))
>  continue;
> 
> The default value for info->discard is discard_sec_merge, so:
> Local labels referring to a mergeable section are discarded when the 
> output is not relocatable.
> A local label is defined as:
> 1) Starts with .X (for i386 unixware compilers)
> 2) Starts with .L
> 3) Starts with .. (for SRV4 compilers)
> 4) Starts with _.L_ (for older buggy GCC)
> 5) Matches L0^A.*
> 6) Matches L[0-9]+{^A|^B}[0-9]* (for assembler generated local labels)
> 
> So to match what is used by the default hypervisor build, I think we 
> should change the check to discard when the symbol matches (2), (5), or 
> (6) above and refers to a mergeable section. The above rules are defined 
> in _bfd_elf_is_local_label_name and elf_i386_is_local_label_name.

Yes. (Note that we don't really care about
elf_i386_is_local_label_name(), as we don't have a 32-bit
hypervisor anymore.)

Jan


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


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

2016-04-25 Thread Jan Beulich
>>> On 24.04.16 at 23:41,  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


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

2016-04-24 Thread Ross Lagerwall

On 04/22/2016 10:10 PM, Konrad Rzeszutek Wilk wrote:

As per my earlier reply to Konrad, there must be more to this. I.e.
"normal" local symbols won't get dropped together with relocations
referencing them getting resolved.


Correct. These .LCx symbols only cover .rodata.* sections. Any other
local symbols:

[konrad@x230 x86]$ readelf --symbols prelink.o |grep bad_hypercall
  8946: 00145549 0 NOTYPE  LOCAL  DEFAULT1 compat_bad_hypercall
  8967: 00145c79 0 NOTYPE  LOCAL  DEFAULT1 bad_hypercall

don't get dropped and do show up in the binary (nm --defined finds them).

While .LC matches the type:
  9147: 08c0 0 NOTYPE  LOCAL  DEFAULT   33 .LC5

They are in four other sections:
konrad@x230 x86]$ readelf --symbols prelink.o |grep LC | awk '{print
$7}' | sort | uniq
22
23
33
34

[22] .rodata.str1.1PROGBITS   0019d500
   a088  0001 AMS   0 0 1
  [23] .rodata.str1.8PROGBITS   001a7588
   00020a31  0001 AMS   0 0 8
  [33] .init.rodata.str1 PROGBITS   001d5a78
   0a1e  0001 AMS   0 0 1
  [34] .init.rodata.str1 PROGBITS   001d6498
   2331  0001 AMS   0 0 8



With some helpful investigation by Konrad, I've found this snippet in 
binutils:


  /* 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)
&& !bfd_link_relocatable (flinfo->info))
   || flinfo->info->discard == discard_l)
   && bfd_is_local_label_name (input_bfd, name)))
continue;

The default value for info->discard is discard_sec_merge, so:
Local labels referring to a mergeable section are discarded when the 
output is not relocatable.

A local label is defined as:
1) Starts with .X (for i386 unixware compilers)
2) Starts with .L
3) Starts with .. (for SRV4 compilers)
4) Starts with _.L_ (for older buggy GCC)
5) Matches L0^A.*
6) Matches L[0-9]+{^A|^B}[0-9]* (for assembler generated local labels)

So to match what is used by the default hypervisor build, I think we 
should change the check to discard when the symbol matches (2), (5), or 
(6) above and refers to a mergeable section. The above rules are defined 
in _bfd_elf_is_local_label_name and elf_i386_is_local_label_name.


--
Ross Lagerwall

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


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

2016-04-24 Thread Konrad Rzeszutek Wilk
On Fri, Apr 22, 2016 at 05:18:16AM -0600, Jan Beulich wrote:
> >>> On 22.04.16 at 13:08,  wrote:
> > On Fri, Apr 22, 2016 at 04:50:34AM -0600, Jan Beulich wrote:
> >> >>> On 22.04.16 at 12:28,  wrote:
> >> > On Fri, Apr 22, 2016 at 04:08:10AM -0600, Jan Beulich wrote:
> >> >> >>> On 22.04.16 at 10:45,  wrote:
> >> >> > Rather than ignoring STT_NOTYPE, an alternative would be to ignore 
> >> >> > symbols starting with ".L".
> >> >> 
> >> >> That's an option, but as said before, the rules for which symbols to
> >> >> enter into the symbol table should be consistent for core and modules.
> >> > 
> >> > And they seem to - see above on the .o file.
> >> 
> >> Above .o file was a core one, and doesn't tell anyway whether in the
> >> runtime symbol table the local symbols would be properly prefixed by
> >> file name. Or did I misunderstand you?
> > 
> > I had 'built_in.o' or 'prelink.o' as the final one in mind - in which the
> > .LCx are present. But I think you are thinking about 'xen-syms' output.
> > 
> > Looking at the Makefile runes we hit this:
> > 127 $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
> > 128 $(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
> > 129 $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
> > 
> > And that ends up causing the .xen-syms.0 file to drop all the .LC0.
> 
> That's odd: I don't know of us telling the linker to do so, and imo
> it shouldn't do this by default. But yes, linkers often do strange
> things...
> 
> > However we can't do that. We _have_ to produce an relocatable object
> > and not do the final linking (so we append -r to the linker invocation).
> 
> Of course.
> 
> > I tried for fun to use strip:
> > xen/xen/arch/x86/test> strip --strip-symbol=".LC0" xen_replace_world.xsplice
> > strip: not stripping symbol `.LC0' because it is named in a relocation
> 
> Which it is rightfully saying.
> 
> So with the linker stripping .LC* (and why knows what else), you
> stripping them when building the runtime symbol table would be
> fine with me, provided we can somehow identify in the linker why
> this stripping happens, and which precise set of symbols get
> stripped (which you should then match in our code).

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.

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; 
..

The function prototype is suppose to adhere to:

DESCRIPTION
Return TRUE if a symbol with the name @var{name} in the BFD
@var{abfd} is a compiler generated local label, 


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:

[konrad@x230 x86]$ readelf --symbols prelink.o |grep LC | awk '{print $7}' | 
sort | uniq
22
23
33
34

 [22] .rodata.str1.1PROGBITS   0019d500
   a088  0001 AMS   0 0 1
  [23] .rodata.str1.8PROGBITS   001a7588
   00020a31  0001 AMS   0 0 8

  [33] .init.rodata.str1 PROGBITS   001d5a78
   0a1e  0001 AMS   0 0 1
  [34] .init.rodata.str1 PROGBITS   001d6498
   2331  0001 AMS   0 0 8


 [22] .rodata.str1.1PROGBITS   0019d500
   a088  0001 AMS   0 0 1
  [23] .rodata.str1.8PROGBITS  

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

2016-04-22 Thread Konrad Rzeszutek Wilk
> As per my earlier reply to Konrad, there must be more to this. I.e.
> "normal" local symbols won't get dropped together with relocations
> referencing them getting resolved.

Correct. These .LCx symbols only cover .rodata.* sections. Any other
local symbols:

[konrad@x230 x86]$ readelf --symbols prelink.o |grep bad_hypercall
 8946: 00145549 0 NOTYPE  LOCAL  DEFAULT1 compat_bad_hypercall
 8967: 00145c79 0 NOTYPE  LOCAL  DEFAULT1 bad_hypercall

don't get dropped and do show up in the binary (nm --defined finds them).

While .LC matches the type:
 9147: 08c0 0 NOTYPE  LOCAL  DEFAULT   33 .LC5

They are in four other sections:
konrad@x230 x86]$ readelf --symbols prelink.o |grep LC | awk '{print
$7}' | sort | uniq
22
23
33
34

[22] .rodata.str1.1PROGBITS   0019d500
  a088  0001 AMS   0 0 1
 [23] .rodata.str1.8PROGBITS   001a7588
  00020a31  0001 AMS   0 0 8
 [33] .init.rodata.str1 PROGBITS   001d5a78
  0a1e  0001 AMS   0 0 1
 [34] .init.rodata.str1 PROGBITS   001d6498
  2331  0001 AMS   0 0 8

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


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

2016-04-22 Thread Konrad Rzeszutek Wilk
On Fri, Apr 22, 2016 at 12:13:02PM +0100, Ross Lagerwall wrote:
> On 04/22/2016 11:08 AM, Jan Beulich wrote:
> On 22.04.16 at 10:45,  wrote:
> >>On 04/22/2016 08:51 AM, Jan Beulich wrote:
> >>On 22.04.16 at 09:17,  wrote:
> On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote: snip
> >>
> >>>+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?
> 
> 
> The payload will typically have many entries like:
> 
> 9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1 10:
> 0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2 11:
> 000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3 12:
> 0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4 13:
> 0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5
> 
> used when referencing strings (due to the use of -fPIC). While it
> is not a problem for them to go into the symbol table, if more than
> one payload is loaded, there will be duplicate conflicting symbols.
> So, to prevent these symbols from going into the symbol table, I
> disallowed STT_NOTYPE. Perhaps not the best solution but...
> >>>
> >>>First of all symbols starting with .L aren't meant to and up in the
> >>>symbol table at all (i.e. even that of any intermediate .o file). So
> >>>there's likely (but not necessarily) something wrong with the tool
> >>>chain used (i.e. normally such symbols wouldn't be needed for e.g.
> >>>relocations, as those should get converted to section relative
> >>>ones).
> >>
> >>This is not particular to the xsplice build process. Any version of
> >>GCC+binutils that I've tested with will generate .LC
> >>symbols for strings into the .o file. Clang generates similar .L.str*
> >>symbols, in addition to other useless ones like 'NOTYPE  LOCAL  DEFAULT
> >>ABS X86_FEATURE_FFXSR'...
> >
> >I can confirm the symbols getting generated in the .s file, ...
> >
> >>Maybe it uses these .LC symbols rather than section relative ones
> >>because they point to a mergeable string section, and merging string
> >>sections would be harder with section relative references?
> >
> >... but I can't confirm them making it into the .o file, not to speak
> >of being used for relocations. I've tried gcc 4.3.4 as well as 5.3.0
> >(with and without -fPIC).
> 
> I've looked into this a little further. The only .LC* symbols left in the .o
> file are the ones which are used in bug_frame relas. These symbols do not
> make it into the core symbol table because the relas are dropped when the
> xen binary is linked just before tools/symbols is run. Obviously we can't
> drop the rela sections for xsplice because it needs to be relocatable.


In my case I seem to have one of the test-cases have one the .text
section.
 That is:

File: arch/x86/test/xen_bye_world.xsplice

Relocation section '.rela.text' at offset 0x11a8 contains 1 entries:
  Offset  Info   Type   Sym. ValueSym. Name + Addend
0007  00130002 R_X86_64_PC32  .LC0 - 4

Which .. is related to the string:

Disassembly of section .text:

 :
   0:   55  push   %rbp
   1:   48 89 e5mov%rsp,%rbp
   4:   48 8d 05 00 00 00 00lea0x0(%rip),%rax# b 

   b:   c9  leaveq 
   c:   c3  retq   

Which we obviously do need.

> 
> >
> >>>Yet _if_ such symbols make it into the symbol table of a .o, then
> >>>there is no reason for them to not also make it into the runtime
> >>>symbol table. Of course similar ones from different modules then
> >>>shouldn't conflict with one another, and as these are local symbols
> >>>perhaps the reason for them conflicting is that in the process of
> >>>creating the runtime symbol table entries you neglect to prefix them
> >>>with their source or object file names, as is done by
> >>>xen/tools/symbols.c for the core symbol table? Quite obviously the
> >>>symbol name generation should match between core and modules...
> >>>
> >>
> >>The build tool does prefix the required functions and objects with their
> >>source/object file names. The problem is that these are generated
> >>symbols, so even if you had e.g. keyhandler.c#.LC0, keyhandler.c#.LC1,
> >>in the symbol table, they might be completed unrelated if you change the
> >>source even slightly. Having these entries in the symbo

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

2016-04-22 Thread Jan Beulich
>>> On 22.04.16 at 13:13,  wrote:
> On 04/22/2016 11:08 AM, Jan Beulich wrote:
> On 22.04.16 at 10:45,  wrote:
>>> On 04/22/2016 08:51 AM, Jan Beulich wrote:
>>> On 22.04.16 at 09:17,  wrote:
> On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote: snip
>>>
 +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?
>
>
> The payload will typically have many entries like:
>
> 9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1 10:
> 0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2 11:
> 000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3 12:
> 0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4 13:
> 0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5
>
> used when referencing strings (due to the use of -fPIC). While it
> is not a problem for them to go into the symbol table, if more than
> one payload is loaded, there will be duplicate conflicting symbols.
> So, to prevent these symbols from going into the symbol table, I
> disallowed STT_NOTYPE. Perhaps not the best solution but...

 First of all symbols starting with .L aren't meant to and up in the
 symbol table at all (i.e. even that of any intermediate .o file). So
 there's likely (but not necessarily) something wrong with the tool
 chain used (i.e. normally such symbols wouldn't be needed for e.g.
 relocations, as those should get converted to section relative
 ones).
>>>
>>> This is not particular to the xsplice build process. Any version of
>>> GCC+binutils that I've tested with will generate .LC
>>> symbols for strings into the .o file. Clang generates similar .L.str*
>>> symbols, in addition to other useless ones like 'NOTYPE  LOCAL  DEFAULT
>>> ABS X86_FEATURE_FFXSR'...
>>
>> I can confirm the symbols getting generated in the .s file, ...
>>
>>> Maybe it uses these .LC symbols rather than section relative ones
>>> because they point to a mergeable string section, and merging string
>>> sections would be harder with section relative references?
>>
>> ... but I can't confirm them making it into the .o file, not to speak
>> of being used for relocations. I've tried gcc 4.3.4 as well as 5.3.0
>> (with and without -fPIC).
> 
> I've looked into this a little further. The only .LC* symbols left in 
> the .o file are the ones which are used in bug_frame relas.

Not according to what I see - there are certainly also .text
relocations referencing .LC*.

> These 
> symbols do not make it into the core symbol table because the relas are 
> dropped when the xen binary is linked just before tools/symbols is run. 
> Obviously we can't drop the rela sections for xsplice because it needs 
> to be relocatable.

As per my earlier reply to Konrad, there must be more to this. I.e.
"normal" local symbols won't get dropped together with relocations
referencing them getting resolved.

>>> The build tool does prefix the required functions and objects with their
>>> source/object file names. The problem is that these are generated
>>> symbols, so even if you had e.g. keyhandler.c#.LC0, keyhandler.c#.LC1,
>>> in the symbol table, they might be completed unrelated if you change the
>>> source even slightly. Having these entries in the symbol table would not
>>> make any sense.
>>
>> Why not? They could still serve as anchor for subsequent patches.
> 
> They're not useful because they're autogenerated and the numbering may 
> change from build to build.

That's not of interest.

> So two patch modules may have conflicting 
> symbols just because they happen to use the same .LCx symbol.

Not if they're, as mentioned before, get properly prefixed with the
respective file name.

But as said in reply to Konrad, I'm fine with them getting dropped
as long as our rule matches ld's (which btw may be arch dependent).

Jan

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


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

2016-04-22 Thread Jan Beulich
>>> On 22.04.16 at 13:08,  wrote:
> On Fri, Apr 22, 2016 at 04:50:34AM -0600, Jan Beulich wrote:
>> >>> On 22.04.16 at 12:28,  wrote:
>> > On Fri, Apr 22, 2016 at 04:08:10AM -0600, Jan Beulich wrote:
>> >> >>> On 22.04.16 at 10:45,  wrote:
>> >> > Rather than ignoring STT_NOTYPE, an alternative would be to ignore 
>> >> > symbols starting with ".L".
>> >> 
>> >> That's an option, but as said before, the rules for which symbols to
>> >> enter into the symbol table should be consistent for core and modules.
>> > 
>> > And they seem to - see above on the .o file.
>> 
>> Above .o file was a core one, and doesn't tell anyway whether in the
>> runtime symbol table the local symbols would be properly prefixed by
>> file name. Or did I misunderstand you?
> 
> I had 'built_in.o' or 'prelink.o' as the final one in mind - in which the
> .LCx are present. But I think you are thinking about 'xen-syms' output.
> 
> Looking at the Makefile runes we hit this:
> 127 $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
> 128 $(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
> 129 $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
> 
> And that ends up causing the .xen-syms.0 file to drop all the .LC0.

That's odd: I don't know of us telling the linker to do so, and imo
it shouldn't do this by default. But yes, linkers often do strange
things...

> However we can't do that. We _have_ to produce an relocatable object
> and not do the final linking (so we append -r to the linker invocation).

Of course.

> I tried for fun to use strip:
> xen/xen/arch/x86/test> strip --strip-symbol=".LC0" xen_replace_world.xsplice
> strip: not stripping symbol `.LC0' because it is named in a relocation

Which it is rightfully saying.

So with the linker stripping .LC* (and why knows what else), you
stripping them when building the runtime symbol table would be
fine with me, provided we can somehow identify in the linker why
this stripping happens, and which precise set of symbols get
stripped (which you should then match in our code).

Jan


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


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

2016-04-22 Thread Ross Lagerwall

On 04/22/2016 11:08 AM, Jan Beulich wrote:

On 22.04.16 at 10:45,  wrote:

On 04/22/2016 08:51 AM, Jan Beulich wrote:

On 22.04.16 at 09:17,  wrote:

On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote: snip



+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?



The payload will typically have many entries like:

9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1 10:
0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2 11:
000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3 12:
0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4 13:
0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5

used when referencing strings (due to the use of -fPIC). While it
is not a problem for them to go into the symbol table, if more than
one payload is loaded, there will be duplicate conflicting symbols.
So, to prevent these symbols from going into the symbol table, I
disallowed STT_NOTYPE. Perhaps not the best solution but...


First of all symbols starting with .L aren't meant to and up in the
symbol table at all (i.e. even that of any intermediate .o file). So
there's likely (but not necessarily) something wrong with the tool
chain used (i.e. normally such symbols wouldn't be needed for e.g.
relocations, as those should get converted to section relative
ones).


This is not particular to the xsplice build process. Any version of
GCC+binutils that I've tested with will generate .LC
symbols for strings into the .o file. Clang generates similar .L.str*
symbols, in addition to other useless ones like 'NOTYPE  LOCAL  DEFAULT
ABS X86_FEATURE_FFXSR'...


I can confirm the symbols getting generated in the .s file, ...


Maybe it uses these .LC symbols rather than section relative ones
because they point to a mergeable string section, and merging string
sections would be harder with section relative references?


... but I can't confirm them making it into the .o file, not to speak
of being used for relocations. I've tried gcc 4.3.4 as well as 5.3.0
(with and without -fPIC).


I've looked into this a little further. The only .LC* symbols left in 
the .o file are the ones which are used in bug_frame relas. These 
symbols do not make it into the core symbol table because the relas are 
dropped when the xen binary is linked just before tools/symbols is run. 
Obviously we can't drop the rela sections for xsplice because it needs 
to be relocatable.





Yet _if_ such symbols make it into the symbol table of a .o, then
there is no reason for them to not also make it into the runtime
symbol table. Of course similar ones from different modules then
shouldn't conflict with one another, and as these are local symbols
perhaps the reason for them conflicting is that in the process of
creating the runtime symbol table entries you neglect to prefix them
with their source or object file names, as is done by
xen/tools/symbols.c for the core symbol table? Quite obviously the
symbol name generation should match between core and modules...



The build tool does prefix the required functions and objects with their
source/object file names. The problem is that these are generated
symbols, so even if you had e.g. keyhandler.c#.LC0, keyhandler.c#.LC1,
in the symbol table, they might be completed unrelated if you change the
source even slightly. Having these entries in the symbol table would not
make any sense.


Why not? They could still serve as anchor for subsequent patches.


They're not useful because they're autogenerated and the numbering may 
change from build to build. So two patch modules may have conflicting 
symbols just because they happen to use the same .LCx symbol.





Rather than ignoring STT_NOTYPE, an alternative would be to ignore
symbols starting with ".L".


That's an option, but as said before, the rules for which symbols to
enter into the symbol table should be consistent for core and modules.



Yes. And, as best I can tell, .L symbols are not in the core table so 
this would then make it consistent for modules.


--
Ross Lagerwall

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


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

2016-04-22 Thread Konrad Rzeszutek Wilk
On Fri, Apr 22, 2016 at 04:50:34AM -0600, Jan Beulich wrote:
> >>> On 22.04.16 at 12:28,  wrote:
> > On Fri, Apr 22, 2016 at 04:08:10AM -0600, Jan Beulich wrote:
> >> >>> On 22.04.16 at 10:45,  wrote:
> >> > On 04/22/2016 08:51 AM, Jan Beulich wrote:
> >> > On 22.04.16 at 09:17,  wrote:
> >> >>> On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote: snip
> >> >
> >> >> +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?
> >> >>>
> >> >>>
> >> >>> The payload will typically have many entries like:
> >> >>>
> >> >>> 9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1 10:
> >> >>> 0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2 11:
> >> >>> 000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3 12:
> >> >>> 0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4 13:
> >> >>> 0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5
> >> >>>
> >> >>> used when referencing strings (due to the use of -fPIC). While it
> >> >>> is not a problem for them to go into the symbol table, if more than
> >> >>> one payload is loaded, there will be duplicate conflicting symbols.
> >> >>> So, to prevent these symbols from going into the symbol table, I
> >> >>> disallowed STT_NOTYPE. Perhaps not the best solution but...
> >> >>
> >> >> First of all symbols starting with .L aren't meant to and up in the
> >> >> symbol table at all (i.e. even that of any intermediate .o file). So
> >> >> there's likely (but not necessarily) something wrong with the tool
> >> >> chain used (i.e. normally such symbols wouldn't be needed for e.g.
> >> >> relocations, as those should get converted to section relative
> >> >> ones).
> >> > 
> >> > This is not particular to the xsplice build process. Any version of 
> >> > GCC+binutils that I've tested with will generate .LC
> >> > symbols for strings into the .o file. Clang generates similar .L.str* 
> >> > symbols, in addition to other useless ones like 'NOTYPE  LOCAL  DEFAULT
> >> > ABS X86_FEATURE_FFXSR'...
> >> 
> >> I can confirm the symbols getting generated in the .s file, ...
> >> 
> >> > Maybe it uses these .LC symbols rather than section relative ones
> >> > because they point to a mergeable string section, and merging string
> >> > sections would be harder with section relative references?
> >> 
> >> ... but I can't confirm them making it into the .o file, not to speak
> >> of being used for relocations. I've tried gcc 4.3.4 as well as 5.3.0
> >> (with and without -fPIC).
> > 
> > /home/konrad/xen/xen/arch/x86
> > [konrad@build-external x86]$ readelf --symbols microcode.o  | grep \.LC
> > 50:  0 NOTYPE  LOCAL  DEFAULT   12 .LC0
> > 51: 00e8 0 NOTYPE  LOCAL  DEFAULT   13 .LC5
> > 52: 0110 0 NOTYPE  LOCAL  DEFAULT   13 .LC6
> > 
> > ?
> > (GCC) 4.4.4 20100503 (Red Hat 4.4.4-2)
> 
> Ah, this happens only with optimization, and I had tried only
> without. Looks to indeed be connected to these strings getting
> put into mergeable string sections, instead of just plain .rodata.
> 
> >> >> Yet _if_ such symbols make it into the symbol table of a .o, then
> >> >> there is no reason for them to not also make it into the runtime
> >> >> symbol table. Of course similar ones from different modules then
> >> >> shouldn't conflict with one another, and as these are local symbols
> >> >> perhaps the reason for them conflicting is that in the process of
> >> >> creating the runtime symbol table entries you neglect to prefix them
> >> >> with their source or object file names, as is done by
> >> >> xen/tools/symbols.c for the core symbol table? Quite obviously the
> >> >> symbol name generation should match between core and modules...
> >> >>
> >> > 
> >> > The build tool does prefix the required functions and objects with their
> >> > source/object file names. The problem is that these are generated
> >> > symbols, so even if you had e.g. keyhandler.c#.LC0, keyhandler.c#.LC1,
> >> > in the symbol table, they might be completed unrelated if you change the
> >> > source even slightly. Having these entries in the symbol table would not
> >> > make any sense.
> >> 
> >> Why not? They could still serve as anchor for subsequent patches.
> > 
> >> 
> >> > Rather than ignoring STT_NOTYPE, an alternative would be to ignore 
> >> > symbols starting with ".L".
> >> 
> >> That's an optio

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

2016-04-22 Thread Jan Beulich
>>> On 22.04.16 at 12:28,  wrote:
> On Fri, Apr 22, 2016 at 04:08:10AM -0600, Jan Beulich wrote:
>> >>> On 22.04.16 at 10:45,  wrote:
>> > On 04/22/2016 08:51 AM, Jan Beulich wrote:
>> > On 22.04.16 at 09:17,  wrote:
>> >>> On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote: snip
>> >
>> >> +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?
>> >>>
>> >>>
>> >>> The payload will typically have many entries like:
>> >>>
>> >>> 9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1 10:
>> >>> 0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2 11:
>> >>> 000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3 12:
>> >>> 0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4 13:
>> >>> 0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5
>> >>>
>> >>> used when referencing strings (due to the use of -fPIC). While it
>> >>> is not a problem for them to go into the symbol table, if more than
>> >>> one payload is loaded, there will be duplicate conflicting symbols.
>> >>> So, to prevent these symbols from going into the symbol table, I
>> >>> disallowed STT_NOTYPE. Perhaps not the best solution but...
>> >>
>> >> First of all symbols starting with .L aren't meant to and up in the
>> >> symbol table at all (i.e. even that of any intermediate .o file). So
>> >> there's likely (but not necessarily) something wrong with the tool
>> >> chain used (i.e. normally such symbols wouldn't be needed for e.g.
>> >> relocations, as those should get converted to section relative
>> >> ones).
>> > 
>> > This is not particular to the xsplice build process. Any version of 
>> > GCC+binutils that I've tested with will generate .LC
>> > symbols for strings into the .o file. Clang generates similar .L.str* 
>> > symbols, in addition to other useless ones like 'NOTYPE  LOCAL  DEFAULT
>> > ABS X86_FEATURE_FFXSR'...
>> 
>> I can confirm the symbols getting generated in the .s file, ...
>> 
>> > Maybe it uses these .LC symbols rather than section relative ones
>> > because they point to a mergeable string section, and merging string
>> > sections would be harder with section relative references?
>> 
>> ... but I can't confirm them making it into the .o file, not to speak
>> of being used for relocations. I've tried gcc 4.3.4 as well as 5.3.0
>> (with and without -fPIC).
> 
> /home/konrad/xen/xen/arch/x86
> [konrad@build-external x86]$ readelf --symbols microcode.o  | grep \.LC
> 50:  0 NOTYPE  LOCAL  DEFAULT   12 .LC0
> 51: 00e8 0 NOTYPE  LOCAL  DEFAULT   13 .LC5
> 52: 0110 0 NOTYPE  LOCAL  DEFAULT   13 .LC6
> 
> ?
> (GCC) 4.4.4 20100503 (Red Hat 4.4.4-2)

Ah, this happens only with optimization, and I had tried only
without. Looks to indeed be connected to these strings getting
put into mergeable string sections, instead of just plain .rodata.

>> >> Yet _if_ such symbols make it into the symbol table of a .o, then
>> >> there is no reason for them to not also make it into the runtime
>> >> symbol table. Of course similar ones from different modules then
>> >> shouldn't conflict with one another, and as these are local symbols
>> >> perhaps the reason for them conflicting is that in the process of
>> >> creating the runtime symbol table entries you neglect to prefix them
>> >> with their source or object file names, as is done by
>> >> xen/tools/symbols.c for the core symbol table? Quite obviously the
>> >> symbol name generation should match between core and modules...
>> >>
>> > 
>> > The build tool does prefix the required functions and objects with their
>> > source/object file names. The problem is that these are generated
>> > symbols, so even if you had e.g. keyhandler.c#.LC0, keyhandler.c#.LC1,
>> > in the symbol table, they might be completed unrelated if you change the
>> > source even slightly. Having these entries in the symbol table would not
>> > make any sense.
>> 
>> Why not? They could still serve as anchor for subsequent patches.
> 
>> 
>> > Rather than ignoring STT_NOTYPE, an alternative would be to ignore 
>> > symbols starting with ".L".
>> 
>> That's an option, but as said before, the rules for which symbols to
>> enter into the symbol table should be consistent for core and modules.
> 
> And they seem to - see above on the .o file.

Above .o file was a core one, and doesn't tell anyway whether in the
runtime s

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

2016-04-22 Thread Konrad Rzeszutek Wilk
On Fri, Apr 22, 2016 at 04:08:10AM -0600, Jan Beulich wrote:
> >>> On 22.04.16 at 10:45,  wrote:
> > On 04/22/2016 08:51 AM, Jan Beulich wrote:
> > On 22.04.16 at 09:17,  wrote:
> >>> On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote: snip
> >
> >> +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?
> >>>
> >>>
> >>> The payload will typically have many entries like:
> >>>
> >>> 9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1 10:
> >>> 0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2 11:
> >>> 000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3 12:
> >>> 0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4 13:
> >>> 0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5
> >>>
> >>> used when referencing strings (due to the use of -fPIC). While it
> >>> is not a problem for them to go into the symbol table, if more than
> >>> one payload is loaded, there will be duplicate conflicting symbols.
> >>> So, to prevent these symbols from going into the symbol table, I
> >>> disallowed STT_NOTYPE. Perhaps not the best solution but...
> >>
> >> First of all symbols starting with .L aren't meant to and up in the
> >> symbol table at all (i.e. even that of any intermediate .o file). So
> >> there's likely (but not necessarily) something wrong with the tool
> >> chain used (i.e. normally such symbols wouldn't be needed for e.g.
> >> relocations, as those should get converted to section relative
> >> ones).
> > 
> > This is not particular to the xsplice build process. Any version of 
> > GCC+binutils that I've tested with will generate .LC
> > symbols for strings into the .o file. Clang generates similar .L.str* 
> > symbols, in addition to other useless ones like 'NOTYPE  LOCAL  DEFAULT
> > ABS X86_FEATURE_FFXSR'...
> 
> I can confirm the symbols getting generated in the .s file, ...
> 
> > Maybe it uses these .LC symbols rather than section relative ones
> > because they point to a mergeable string section, and merging string
> > sections would be harder with section relative references?
> 
> ... but I can't confirm them making it into the .o file, not to speak
> of being used for relocations. I've tried gcc 4.3.4 as well as 5.3.0
> (with and without -fPIC).

/home/konrad/xen/xen/arch/x86
[konrad@build-external x86]$ readelf --symbols microcode.o  | grep \.LC
50:  0 NOTYPE  LOCAL  DEFAULT   12 .LC0
51: 00e8 0 NOTYPE  LOCAL  DEFAULT   13 .LC5
52: 0110 0 NOTYPE  LOCAL  DEFAULT   13 .LC6

?
(GCC) 4.4.4 20100503 (Red Hat 4.4.4-2)

> 
> >> Yet _if_ such symbols make it into the symbol table of a .o, then
> >> there is no reason for them to not also make it into the runtime
> >> symbol table. Of course similar ones from different modules then
> >> shouldn't conflict with one another, and as these are local symbols
> >> perhaps the reason for them conflicting is that in the process of
> >> creating the runtime symbol table entries you neglect to prefix them
> >> with their source or object file names, as is done by
> >> xen/tools/symbols.c for the core symbol table? Quite obviously the
> >> symbol name generation should match between core and modules...
> >>
> > 
> > The build tool does prefix the required functions and objects with their
> > source/object file names. The problem is that these are generated
> > symbols, so even if you had e.g. keyhandler.c#.LC0, keyhandler.c#.LC1,
> > in the symbol table, they might be completed unrelated if you change the
> > source even slightly. Having these entries in the symbol table would not
> > make any sense.
> 
> Why not? They could still serve as anchor for subsequent patches.

> 
> > Rather than ignoring STT_NOTYPE, an alternative would be to ignore 
> > symbols starting with ".L".
> 
> That's an option, but as said before, the rules for which symbols to
> enter into the symbol table should be consistent for core and modules.

And they seem to - see above on the .o file.

> 
> Jan

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


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

2016-04-22 Thread Jan Beulich
>>> On 22.04.16 at 10:45,  wrote:
> On 04/22/2016 08:51 AM, Jan Beulich wrote:
> On 22.04.16 at 09:17,  wrote:
>>> On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote: snip
>
>> +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?
>>>
>>>
>>> The payload will typically have many entries like:
>>>
>>> 9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1 10:
>>> 0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2 11:
>>> 000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3 12:
>>> 0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4 13:
>>> 0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5
>>>
>>> used when referencing strings (due to the use of -fPIC). While it
>>> is not a problem for them to go into the symbol table, if more than
>>> one payload is loaded, there will be duplicate conflicting symbols.
>>> So, to prevent these symbols from going into the symbol table, I
>>> disallowed STT_NOTYPE. Perhaps not the best solution but...
>>
>> First of all symbols starting with .L aren't meant to and up in the
>> symbol table at all (i.e. even that of any intermediate .o file). So
>> there's likely (but not necessarily) something wrong with the tool
>> chain used (i.e. normally such symbols wouldn't be needed for e.g.
>> relocations, as those should get converted to section relative
>> ones).
> 
> This is not particular to the xsplice build process. Any version of 
> GCC+binutils that I've tested with will generate .LC
> symbols for strings into the .o file. Clang generates similar .L.str* 
> symbols, in addition to other useless ones like 'NOTYPE  LOCAL  DEFAULT
> ABS X86_FEATURE_FFXSR'...

I can confirm the symbols getting generated in the .s file, ...

> Maybe it uses these .LC symbols rather than section relative ones
> because they point to a mergeable string section, and merging string
> sections would be harder with section relative references?

... but I can't confirm them making it into the .o file, not to speak
of being used for relocations. I've tried gcc 4.3.4 as well as 5.3.0
(with and without -fPIC).

>> Yet _if_ such symbols make it into the symbol table of a .o, then
>> there is no reason for them to not also make it into the runtime
>> symbol table. Of course similar ones from different modules then
>> shouldn't conflict with one another, and as these are local symbols
>> perhaps the reason for them conflicting is that in the process of
>> creating the runtime symbol table entries you neglect to prefix them
>> with their source or object file names, as is done by
>> xen/tools/symbols.c for the core symbol table? Quite obviously the
>> symbol name generation should match between core and modules...
>>
> 
> The build tool does prefix the required functions and objects with their
> source/object file names. The problem is that these are generated
> symbols, so even if you had e.g. keyhandler.c#.LC0, keyhandler.c#.LC1,
> in the symbol table, they might be completed unrelated if you change the
> source even slightly. Having these entries in the symbol table would not
> make any sense.

Why not? They could still serve as anchor for subsequent patches.

> Rather than ignoring STT_NOTYPE, an alternative would be to ignore 
> symbols starting with ".L".

That's an option, but as said before, the rules for which symbols to
enter into the symbol table should be consistent for core and modules.

Jan

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


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

2016-04-22 Thread Ross Lagerwall

On 04/22/2016 08:51 AM, Jan Beulich wrote:

On 22.04.16 at 09:17,  wrote:

On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote: snip



+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?



The payload will typically have many entries like:

9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1 10:
0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2 11:
000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3 12:
0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4 13:
0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5

used when referencing strings (due to the use of -fPIC). While it
is not a problem for them to go into the symbol table, if more than
one payload is loaded, there will be duplicate conflicting symbols.
So, to prevent these symbols from going into the symbol table, I
disallowed STT_NOTYPE. Perhaps not the best solution but...


First of all symbols starting with .L aren't meant to and up in the
symbol table at all (i.e. even that of any intermediate .o file). So
there's likely (but not necessarily) something wrong with the tool
chain used (i.e. normally such symbols wouldn't be needed for e.g.
relocations, as those should get converted to section relative
ones).


This is not particular to the xsplice build process. Any version of 
GCC+binutils that I've tested with will generate .LC
symbols for strings into the .o file. Clang generates similar .L.str* 
symbols, in addition to other useless ones like 'NOTYPE  LOCAL  DEFAULT

ABS X86_FEATURE_FFXSR'...

Maybe it uses these .LC symbols rather than section relative ones
because they point to a mergeable string section, and merging string
sections would be harder with section relative references?



Yet _if_ such symbols make it into the symbol table of a .o, then
there is no reason for them to not also make it into the runtime
symbol table. Of course similar ones from different modules then
shouldn't conflict with one another, and as these are local symbols
perhaps the reason for them conflicting is that in the process of
creating the runtime symbol table entries you neglect to prefix them
with their source or object file names, as is done by
xen/tools/symbols.c for the core symbol table? Quite obviously the
symbol name generation should match between core and modules...



The build tool does prefix the required functions and objects with their
source/object file names. The problem is that these are generated
symbols, so even if you had e.g. keyhandler.c#.LC0, keyhandler.c#.LC1,
in the symbol table, they might be completed unrelated if you change the
source even slightly. Having these entries in the symbol table would not
make any sense.

Rather than ignoring STT_NOTYPE, an alternative would be to ignore 
symbols starting with ".L".


--
Ross Lagerwall

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


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

2016-04-22 Thread Jan Beulich
>>> On 22.04.16 at 09:17,  wrote:
> On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote:
> snip
>>>
 +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?
> 
> 
> The payload will typically have many entries like:
> 
>   9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1
>  10: 0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2
>  11: 000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3
>  12: 0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4
>  13: 0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5
> 
> used when referencing strings (due to the use of -fPIC). While it is not 
> a problem for them to go into the symbol table, if more than one payload 
> is loaded, there will be duplicate conflicting symbols. So, to prevent 
> these symbols from going into the symbol table, I disallowed STT_NOTYPE. 
> Perhaps not the best solution but...

First of all symbols starting with .L aren't meant to and up in the
symbol table at all (i.e. even that of any intermediate .o file). So
there's likely (but not necessarily) something wrong with the tool
chain used (i.e. normally such symbols wouldn't be needed for e.g.
relocations, as those should get converted to section relative ones).

Yet _if_ such symbols make it into the symbol table of a .o, then
there is no reason for them to not also make it into the runtime
symbol table. Of course similar ones from different modules then
shouldn't conflict with one another, and as these are local symbols
perhaps the reason for them conflicting is that in the process of
creating the runtime symbol table entries you neglect to prefix
them with their source or object file names, as is done by
xen/tools/symbols.c for the core symbol table? Quite obviously
the symbol name generation should match between core and
modules...

Jan


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


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

2016-04-22 Thread Ross Lagerwall

On 04/21/2016 01:26 AM, Konrad Rzeszutek Wilk wrote:
snip



+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?



The payload will typically have many entries like:

 9:  0 NOTYPE  LOCAL  DEFAULT5 .LC1
10: 0006 0 NOTYPE  LOCAL  DEFAULT5 .LC2
11: 000d 0 NOTYPE  LOCAL  DEFAULT5 .LC3
12: 0028 0 NOTYPE  LOCAL  DEFAULT4 .LC4
13: 0058 0 NOTYPE  LOCAL  DEFAULT4 .LC5

used when referencing strings (due to the use of -fPIC). While it is not 
a problem for them to go into the symbol table, if more than one payload 
is loaded, there will be duplicate conflicting symbols. So, to prevent 
these symbols from going into the symbol table, I disallowed STT_NOTYPE. 
Perhaps not the best solution but...


--
Ross Lagerwall

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


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

2016-04-21 Thread Jan Beulich
>>> On 21.04.16 at 15:56,  wrote:
>> > So I did try that and it all worked nicely on x86. However on ARM32:
>> >
>> > arm  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.
>>
>> Well, why would any kind of address be uint64_t on ARM32?
>>
> 
> Oh, the xensyms_read - which is used in a hypercalls (for reading the
> symbols) as well - requires an pointer to an uint64_t as an argument. Hence
> the need for using that type.
> 
> I can certainly muck with it so that it can do xen_pfn_t.

Which also is a 64-bit type on ARM, and which would be really wrong
to use here. There's really no point in xensyms_read() taking other
than an unsigned long *, and if the sysctl wants a uint64_t, then it
should use a temporary variable to do the conversion.

Jan


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


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

2016-04-21 Thread Konrad Rzeszutek Wilk
> > So I did try that and it all worked nicely on x86. However on ARM32:
> >
> > arm  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.
>
> Well, why would any kind of address be uint64_t on ARM32?
>

Oh, the xensyms_read - which is used in a hypercalls (for reading the
symbols) as well - requires an pointer to an uint64_t as an argument. Hence
the need for using that type.

I can certainly muck with it so that it can do xen_pfn_t.

> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel


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

2016-04-20 Thread Jan Beulich
>>> On 21.04.16 at 02:26,  wrote:
>> >+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?

Yes. Thinko of mine.

>> >@@ -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  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.

Well, why would any kind of address be uint64_t on ARM32?

Jan


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


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

2016-04-20 Thread Konrad Rzeszutek Wilk
> >+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  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 
> +
> to fill ->old_addr?

No. Just . 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@lists.xen.org
http://lists.xen.org/xen-devel


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

2016-04-20 Thread Ross Lagerwall

On 04/19/2016 08:31 PM, Jan Beulich wrote:
snip

+ASSERT(spin_is_locked(&payload_lock));
+list_for_each_entry ( data, &payload_list, list )
+{
+unsigned int i;
+
+for ( i = 0; i < data->nsyms; i++ )
+{
+if ( !data->symtab[i].new_symbol )
+continue;


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?


new_symbol indicates whether it is newly introduced in this binary patch 
or whether it overrides a symbol in a previous binary patch (or the core 
binary). Either way, the symbol will be used when resolving a backtrace.



--
Ross Lagerwall

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


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

2016-04-19 Thread Jan Beulich
>>> Konrad Rzeszutek Wilk  04/14/16 12:02 AM >>>
>--- a/xen/arch/x86/test/xen_hello_world.c
>+++ b/xen/arch/x86/test/xen_hello_world.c
>@@ -10,11 +10,14 @@
 >static char hello_world_patch_this_fnc[] = "xen_extra_version";
 >extern const char *xen_hello_world(void);
 >
>+/* External symbol. */
>+extern const char *xen_extra_version(void);

To give a good example, I think this would better include the respective
header.

>struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_hello_world 
>= {
 >.version = XSPLICE_PAYLOAD_VERSION,
 >.name = hello_world_patch_this_fnc,
 >.new_addr = (unsigned long)(xen_hello_world),
>-.old_addr = OLD_CODE,
>+.old_addr = (unsigned long)(xen_extra_version),
 
Pointless parentheses (also btw on the new_addr initializer, which would be
nice to get fixed in the earlier patch).

>+unsigned long symbols_lookup_by_name(const char *symname)
>+{
>+char name[KSYM_NAME_LEN + 1] = { 0 };

I continue to think that this initializer is pointless. And if this or a 
variant filling
just the first byte was needed, then please be consistent with the rest of the
function and either use '\0' here or plain 0 in the other two relevant places.

>+uint32_t symnum = 0;
>+char type;
>+uint64_t addr = 0;

Same here.

>+int rc;
>+
>+if ( symname == '\0' )

DYM *symname?

>@@ -51,6 +52,9 @@ struct payload {
 >struct list_head applied_list;   /* Linked to 'applied_list'. */
 >struct xsplice_patch_func_internal *funcs;/* The array of functions 
to patch. */
 >unsigned int nfuncs; /* Nr of functions to patch. */
>+struct xsplice_symbol *symtab;   /* All symbols. */
>+char *strtab;/* Pointer to .strtab. */

At least the latter of the two I'm convinced can be const.

>+unsigned long xsplice_symbols_lookup_by_name(const char *symname)
>+{
>+struct payload *data;

const

>+ASSERT(spin_is_locked(&payload_lock));
>+list_for_each_entry ( data, &payload_list, list )
>+{
>+unsigned int i;
>+
>+for ( i = 0; i < data->nsyms; i++ )
>+{
>+if ( !data->symtab[i].new_symbol )
>+continue;

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?

>@@ -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.

>+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 
+
to fill ->old_addr?

>+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.

>+static int build_symbol_table(struct payload *payload,
>+  const struct xsplice_elf *elf)
>+{
>+unsigned int i, j, nsyms = 0;
>+size_t strtab_len = 0;
>+struct xsplice_symbol *symtab;
>+char *strtab;
>+
>+ASSERT(payload->nfuncs);
>+
>+/* Recall that section @0 is always NULL. */
>+for ( i = 1; i < elf->nsym; i++ )
>+{
>+if ( is_payload_symbol(elf, elf->sym + i) )
>+{
>+nsyms++;
>+strtab_len += strlen(elf->sym[i].name) + 1;
>+}
>+}
>+
>+symtab = xmalloc_array(struct xsplice_symbol, nsyms);
>+strtab = xmalloc_array(char, strtab_len);
>+
>+if ( !strtab || !symtab )
>+{
>+xfree(strtab);
>+xfree(symtab);
>+return -ENOMEM;
>+}
>+
>+nsyms = 0;

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

2016-04-13 Thread Konrad Rzeszutek Wilk
From: Ross Lagerwall 

If in the payload we do not have the old_addr we can resolve
the virtual address based on the UNDEFined symbols.

We also use an boolean flag: new_symbol to track symbols. The usual
case this is used is by:

* A payload may introduce a new symbol
* A payload may override an existing symbol (introduced in Xen or another
  payload)
* Overriding symbols must exist in the symtab for backtraces.
* A payload must always link against the object which defines the new symbol.

Considering that payloads may be loaded in any order it would be incorrect to
link against a payload which simply overrides a symbol because you could end
up with a chain of jumps which is inefficient and may result in the expected
function not being executed.

Also we include a local definition block in the symbols.c file.

Signed-off-by: Konrad Rzeszutek Wilk 
Signed-off-by: Ross Lagerwall 
Reviewed-by: Andrew Cooper 

---
Cc: Keir Fraser 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v1: Ross original version.
v2: Include test-case and document update.
v2: s/size_t/ssize_t/
Include core_text_size, core_text calculation
v4: Cast on dprintk to uint64_t to make ELF 32bit build.
v6: Rebase where the spinlock is no more recursive. Drop the spinlock
usage in xsplice_symbols_lookup_by_name
v7: Add Andrew's Reviewed-by
Initialize addr and symname to zero as xensyms_read uses it.
v8: Change one XENLOG_DEBUG to XENLOG_ERR.
Change printk to dprintk on symbols and one error case.
---
 xen/arch/x86/Makefile   |  16 +++-
 xen/arch/x86/test/Makefile  |   4 +-
 xen/arch/x86/test/xen_hello_world.c |   5 +-
 xen/common/symbols.c|  34 
 xen/common/xsplice.c| 156 +++-
 xen/common/xsplice_elf.c|  19 -
 xen/include/xen/symbols.h   |   2 +
 xen/include/xen/xsplice.h   |   8 ++
 8 files changed, 232 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 6c2d5fa..57c93e1 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -72,6 +72,12 @@ efi-y := $(shell if [ ! -r $(BASEDIR)/include/xen/compile.h 
-o \
   -O $(BASEDIR)/include/xen/compile.h ]; then \
  echo '$(TARGET).efi'; fi)
 
+ifdef CONFIG_XSPLICE
+all_symbols = --all-symbols
+else
+all_symbols =
+endif
+
 $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
./boot/mkelf32 $(TARGET)-syms $(TARGET) 0x10 \
`$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
@@ -111,12 +117,14 @@ $(TARGET)-syms: prelink.o xen.lds 
$(BASEDIR)/common/symbols-dummy.o
$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
$(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
-   | $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0.S
+   | $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort \
+   >$(@D)/.$(@F).0.S
$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o
$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
$(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
-   | $(BASEDIR)/tools/symbols --sysv --sort --warn-dup 
>$(@D)/.$(@F).1.S
+   | $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort 
--warn-dup \
+   >$(@D)/.$(@F).1.S
$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
$(@D)/.$(@F).1.o -o $@
@@ -140,14 +148,14 @@ $(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o 
$(BASEDIR)/common/symbol
$(BASEDIR)/common/symbols-dummy.o -o 
$(@D)/.$(@F).$(base).0 &&) :
$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) 
$(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
-   | $(guard) $(BASEDIR)/tools/symbols --sysv --sort 
>$(@D)/.$(@F).0s.S
+   | $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv 
--sort >$(@D)/.$(@F).0s.S
$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o 
$(@D)/.$(@F).0s.o
$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
  $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
$(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o 
$(@D)/.$(@F).$(base).1 &&) :
$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) 
$(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
-   | $(guard) $(BASEDIR)/tools/symbols --sysv --sort 
>$(@D)/.$(@F).1s.S
+   | $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv 
--sort >$(@D)/.$(@F).1s.S
$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o 
$(@D)/.$(@F).1s.o
$(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $< \