RFR (S) 8144491 ElfSymbolTable::lookup returns bad value

[Ioi Lam]

On 12/2/15 9:54 AM, Daniel D. Daugherty wrote:
> On 12/2/15 9:48 AM, Stefan Karlsson wrote:
>> Hi Ioi,
>>
>> On 2015-12-02 15:30, Ioi Lam wrote:
>>> Please review a small fix:
>>>
>>> http://cr.openjdk.java.net/~iklam/8144491-ElfSymbolTable-lookup/
>>>
>>> Bug: ElfSymbolTable::lookup returns bad value when the lookup has 
>>> failed
>>>
>>>     https://bugs.openjdk.java.net/browse/JDK-8144491
>
> src/share/vm/utilities/elfSymbolTable.hpp
>     L67:   bool compare(const Elf_Sym* sym, address addr, int* 
> stringtableIndex,
>            int* posIndex, int* offset, ElfFuncDescTable* funcDescTable);
>         Should funcDescTable be a 'const'?
>
>
> src/share/vm/utilities/elfSymbolTable.cpp
>     L72: bool ElfSymbolTable::compare(const Elf_Sym* sym, address addr,
>          int* stringtableIndex, int* posIndex, int* offset,
>          ElfFuncDescTable* funcDescTable) {
>
>         Should funcDescTable be a 'const'?
>
Hi Dan,

I tried changing funcDescTable to const, but then gcc barfs on this line

         funcDescTable->lookup(sym->st_value)

as the lookup() function is not declared as const (it modifies 
funcDescTable->m_status).

So I will push the code exactly as in the webrev posted above.

Thanks
- Ioi

> Thumbs up! Don't need to see another webrev if you change the
> const-ness of funcDescTable
>
> More below...
>
>>>
>>> Summary of fix:
>>>
>>>     I found this when trying to make disassembler.cpp print out more 
>>> symbolic
>>>     information for PrintInterpreter.
>>>
>>>     The fix is straight-forward -- if the lookup fails, 
>>> ElfSymbolTable::lookup
>>>     should return false.
>>>
>>>     Impact:
>>>     The only caller to ElfSymbolTable::lookup is ElfFile::decode(), 
>>> so nothing
>>>     else would depend on the old (bad) behavior of 
>>> ElfSymbolTable::lookup.
>>>
>>>     I also took the chance to refactor lookup is ElfFile::decode() 
>>> to split out
>>>     two blocks of identical code into a new function, 
>>> ElfSymbolTable::compare.
>>
>> The change looks correct. I would prefer if the refactoring was done 
>> as a separate patch, but since this is Runtime code I'll let other 
>> reviewers decide on that.
>
> For JDK9 we've been been doing both:
>
> - product changes plus cleanup changes in the same changeset
> - cleanup changes in one changeset and product changes in another
>
> It really depends on how much the cleanup obscures the product
> change. I don't think this one is too distracting...
>
>
>>
>> Thanks,
>> StefanK
>>>
>>> Tests:
>>>
>>>     JPRT
>>>     RBT (hotspot/test/:hotspot_all)  <-- what other tests should I run?
>
> Dmitry Samersoff might have an opinion here since he tends to
> do work on the ELF code... and probably knows what tests hit
> this code.
>
> Dan
>
>
>
>>>
>>> Thanks
>>> - Ioi
>>
>