RFR(S): 8019929: PPC64 (part 107): Extend ELF-decoder to support PPC64 function descriptor tables

[Volker Simonis]

Hi Vitaly,

you're right - I'll fix it.

Thanks,
Volker


On Thu, Dec 5, 2013 at 2:15 AM, Vitaly Davidovich <vitalyd at gmail.com> wrote:
> Ok.
>
> 171         if (string_table->string_at(shdr.sh_name, buf, sizeof(buf)) &&
> !strncmp(".opd", buf, 4)) {
> 172           m_funcDesc_table = new (std::nothrow) ElfFuncDescTable(m_file,
> shdr);
> 173           break;
>
> So if that alloc fails, I see that code handles a null m_funcDesc_table
> where it's used.  But is that what you want for PPC64? Won't you get wrong
> symbol info? Code reading other tables does this for OOM cases:
>
> m_status = NullDecoder::out_of_memory;
> return false;
>
> Sent from my phone
>
> On Dec 4, 2013 2:17 PM, "Volker Simonis" <volker.simonis at gmail.com> wrote:
>>
>> Hi,
>>
>> thanks for the comments.
>>
>> I think the function descriptor table logically belongs to the
>> ELF-file itself and not to symbol table. An ELF file can have several
>> symbol tables but just one function descriptor table. Also, the
>> function descriptor table is read in when the ELF file is opened (i.e.
>> in the ElfFile constructor).
>>
>> So after Vladimirs suggestion to remove most of the "#ifdef PPC64"
>> there was no reason to keep the ElfFuncDescTable class in
>> elfSymbolTable.{hpp,cpp} so I created two new files
>> elfFuncDescTable.{hpp, cpp} for it. Now the only remaining "#ifdef
>> PPC64" is in ElfFile::load_tables() when the function descriptor table
>> is loaded (as requested by Vladimir).
>>
>> But actually, the corresponding '.opd' section is only available on
>> PPC64 (see
>> http://refspecs.linuxfoundation.org/LSB_3.1.1/LSB-Core-PPC64/LSB-Core-PPC64/specialsections.html)
>> and I don't think the code will do any harm if it would be executed on
>> a non-PPC64 system - the '.opd' section would just not be found. I
>> also think the corresponding performance impact would be minimal
>> compared to the loading of the symbol and string tables. So I tend to
>> remove the last "#ifdef PPC64" as well. So what do you think - I'm OK
>> with both solutions?
>>
>> Below is a webrev with the described changes (and still with the last
>> "#ifdef PPC64" in ElfFile::load_tables()):
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/8019929.v2/
>>
>> If you agree with it, I would appreciate if you could push it trough JPRT.
>>
>> Thank you and best regards,
>> Volker
>>
>> PS: the little change in make/aix/makefiles/vm.make was necessarx to
>> exlude the new file from the AIX-build because AIX uses XCOFF instead
>> of ELF.
>>
>>
>> On Wed, Dec 4, 2013 at 3:39 AM, Vitaly Davidovich <vitalyd at gmail.com>
>> wrote:
>> > Hi Volker,
>> >
>> > Would it be cleaner if you were to extend ElfSymbolTable for PPC and
>> > embed
>> > the funcDesc in there, keeping the lookup() signature the same? It seems
>> > like the funcDesc should be a hidden indirection as part of lookup()
>> > rather
>> > than a parameter.
>> >
>> > Just a thought ...
>> >
>> > Sent from my phone
>> >
>> > On Dec 3, 2013 5:43 PM, "Volker Simonis" <volker.simonis at gmail.com>
>> > wrote:
>> >>
>> >> Hi Vladimir,
>> >>
>> >> thanks for looking at the change. I initially did it this way to
>> >> keep changes to the existing platforms as small as possible but I'll be
>> >> happy to change in the way you suggested if nobody objects.
>> >>
>> >> Regards,
>> >> Volker
>> >>
>> >> On Tuesday, December 3, 2013, Vladimir Kozlov wrote:
>> >>
>> >> > Volker,
>> >> >
>> >> > It looks fine to me except #ifdef pollution.
>> >> >
>> >> > I think ElfSymbolTable::lookup() should always take ElfFuncDescTable
>> >> > argument and you need ElfFuncDescTable always defined.
>> >> > In ElfSymbolTable::lookup() you can check funcDescTable for null
>> >> > instead
>> >> > of ifdefs.
>> >> > The only place we can keep #ifdef is m_funcDesc_table setting in
>> >> > ElfFile::load_tables().
>> >> > ElfFuncDescTable class's methods are not so big to ifdef them.
>> >> >
>> >> > But others may have different opinion.
>> >> >
>> >> > Thanks,
>> >> > Vladimir
>> >> >
>> >> > On 12/3/13 9:39 AM, Volker Simonis wrote:
>> >> >
>> >> >> On PowerPC-64 (and other architectures like for example IA64) a
>> >> >> pointer to a function is not just a plain code address, but a
>> >> >> pointer
>> >> >> to a so called function descriptor (see
>> >> >> http://refspecs.linuxfoundation.org/ELF/ppc64/
>> >> >> PPC-elf64abi-1.9.html#FUNC-DES).
>> >> >> This fact is also reflected in the ELF ABI for PowerPC-64. This
>> >> >> small
>> >> >> changes adds support for ELF function descriptor tables to the
>> >> >> current
>> >> >> ELF decoder:
>> >> >>
>> >> >> http://cr.openjdk.java.net/~simonis/webrevs/8019929/
>> >> >>
>> >> >> On architectures like x86 or SPARC, the ELF symbol table contains
>> >> >> the
>> >> >> start address and size of an object. So for example for a function
>> >> >> object (i.e. type FUNC) the symbol table's value field directly
>> >> >> represents the starting address and the size field the size of a
>> >> >> function. On PPC64 however, the symbol table's value field only
>> >> >> contains an index into a PPC64 specific .opd (official procedure
>> >> >> descriptors) section, while the size field still holds the size of
>> >> >> the
>> >> >> corresponding function. In order to get the actual start address of
>> >> >> a
>> >> >> function, it is necessary to read the corresponding function
>> >> >> descriptor entry in the .opd section at the corresponding index and
>> >> >> extract the start address from there.
>> >> >>
>> >> >> This change extends the current HotSpot ELF utilities to support the
>> >> >> .opd (official procedure descriptors) section on PPC64 platforms. It
>> >> >> does this by adding a new field m_funcDesc_table of type
>> >> >> ElfFuncDescTable to the ElfFile class. The m_funcDesc_table is
>> >> >> initialized in the ElfFile::load_tables() in the same way like the
>> >> >> symbol table members by parsing the corresponding .opd section if it
>> >> >> is available.
>> >> >>
>> >> >> The ElfSymbolTable::lookup() method is changed on PPC64 to take an
>> >> >> extra ElfFuncDescTable argument. If running on PPC64, this argument
>> >> >> is
>> >> >> used to do the extra level of indirection through the function
>> >> >> description table to get the real start address associated with a
>> >> >> symbol.
>> >> >>
>> >> >> This change also slightly improves the implementation of
>> >> >> ElfSymbolTable::lookup(). Before, the method always iterated over
>> >> >> all
>> >> >> symbols in the symbol table and returned the one with the highest
>> >> >> address below the requested addr argument. This not only could take
>> >> >> a
>> >> >> significant amount of time for big libraries, it could also return
>> >> >> bogus symbols for addresses which were not really covered by that
>> >> >> symbol table at all. The new versions additionally uses the symbol
>> >> >> table's st_size field to verify that the requested addr argument is
>> >> >> indeed within the range covered by the corresponding symbol table
>> >> >> entry. If so, the search is stopped and the symbol is returned
>> >> >> immediately.
>> >> >>
>> >> >> Thank you and best regards,
>> >> >> Volker
>> >> >>
>> >> >>