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

Daniel D. Daugherty daniel.daugherty at oracle.com
Wed Dec 2 17:54:19 UTC 2015

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

     L67:   bool compare(const Elf_Sym* sym, address addr, int* 
            int* posIndex, int* offset, ElfFuncDescTable* funcDescTable);
         Should funcDescTable be a 'const'?

     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.


>> Thanks
>> - Ioi

More information about the hotspot-dev mailing list