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

[Vladimir Kozlov]

I would keep "#ifdef PPC64" in ElfFile::load_tables()) to be safe 
because someone can create .opd file for different purpose on other 
platforms.

Few nitpicks. In our coding style we don't put 'else' and 'else if' on 
separate line:

} else if () {

I see you put 'else' on separate line in elfSymbolTable.cpp and elfFile.cpp.

In elfFuncDescTable.cpp missed _APPLE in #endif comment.

Thanks,
Vladimir

On 12/4/13 11:17 AM, Volker Simonis 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
>>>>>
>>>>>