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

[Daniel D. Daugherty]

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

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
>