RFR(M) 8193373: Cleanup ElfFile and family

[Zhengyu Gu]

Hi Coleen,

Thanks for the review and sponsor. Please find attached patch.

-Zhengyu

On 02/13/2018 04:45 PM, coleen.phillimore at oracle.com wrote:
> 
> I have a few minor comments, but am not really knowledgeable about this 
> code.
> 
> http://cr.openjdk.java.net/~zgu/8193373/webrev.01/src/hotspot/os/linux/decoder_linux.cpp.udiff.html 
> 
> 
> + // AARCH64 defaults to noexecstack. All others default to execstack.
> +#ifdef AARCH64
> + bool result = true;
> +#else
> + bool result = false;
> +#endif
> 
> Can you write as a more compact:
> 
> + // AARCH64 defaults to noexecstack. All others default to execstack.
> + bool result = AARCH64_ONLY(true) NOT_AARCH64(false);
> 
> http://cr.openjdk.java.net/~zgu/8193373/webrev.01/src/hotspot/share/prims/whitebox.cpp.udiff.html 
> 
> 
> + ElfFile::_donnot_cache_elf_section = true;
> 
> 
> Can you add an underscore _do_not_cache_elf_section ?
> 
> http://cr.openjdk.java.net/~zgu/8193373/webrev.01/test/hotspot/jtreg/runtime/8193373/TestElfDirectRead.java.html 
> 
> 
> Can you make the directory name something like: ElfDecoder rather than 
> the bugid number?
> 
> Did you test this against some hs_err_pid files?  I don't need to see 
> another review with these changes and I can sponsor this for you.
> 
> thanks,
> Coleen
> On 2/12/18 9:44 PM, Zhengyu Gu wrote:
>> Hi Yumin,
>>
>> I made change according to your comment. Also, added a test.
>>
>> Webrev: http://cr.openjdk.java.net/~zgu/8193373/webrev.01/index.html
>>
>> Test:
>>   Reran hotspot_runtime tests on Linux 64 (fastdebug and release)
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>>
>> On 02/08/2018 12:51 PM, yumin qi wrote:
>>> Hi, Zhengyu
>>>
>>>    Looks good to me. One minor comment:
>>>
>>> +bool ElfFile::specifies_noexecstack(const char* filepath) {
>>> + // Returns true if the elf file is marked NOT to require an 
>>> executable stack,
>>> + // or if the file could not be opened.
>>> + // Returns false if the elf file requires an executable stack, the 
>>> stack flag
>>> + // is not set at all, or if the file can not be read.
>>>
>>>
>>>    should the comments in side the function be moved above the function?
>>>
>>>     others looks OK to me.
>>>
>>> Thanks
>>> Yumin
>>>
>>>
>>>
>>>
>>> On Mon, Feb 5, 2018 at 8:08 AM, Zhengyu Gu <zgu at redhat.com 
>>> <mailto:zgu at redhat.com>> wrote:
>>>
>>>     Please review this cleanup and refactoring of elf decoder.
>>>
>>>     * Fixed coding styles.
>>>        - Moved non-static members to the top of classes
>>>        - Fixed variable names (from Windows naming style to hotspot 
>>> style)
>>>
>>>     * Moved Linux implementation of ElfFile::specifies_noexecstack to os
>>>        directory
>>>
>>>     * Refactored section cache logic into common ElfSection class.
>>>
>>>     * Added utility classes FileReader and MarkedFileReader to cleanup
>>>        duplicated code.
>>>
>>>     * Deleted section header string table once initialization is 
>>> completed,
>>>        cause it is not used for decoding.
>>>
>>>     * When decoder failed to cache section data, current implementation
>>>        tries to fallback to read data from file directly. This never
>>>        worked, cause it put decoder into out of memory state when it 
>>> failed
>>>        load section data, so decoder always bailed out prematurely.
>>>        This patch also fixed this bug.
>>>
>>>     Bug: https://bugs.openjdk.java.net/browse/JDK-8193373
>>>     <https://bugs.openjdk.java.net/browse/JDK-8193373>
>>>     Webrev: http://cr.openjdk.java.net/~zgu/8193373/webrev.00/
>>>     <http://cr.openjdk.java.net/~zgu/8193373/webrev.00/>
>>>
>>>
>>>     Test:
>>>
>>>        hotspot_runtime on Linux 64 (fastdebug and release)
>>>
>>>     Thanks,
>>>
>>>     -Zhengyu
>>>
>>>
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8193373.patch
Type: text/x-patch
Size: 48689 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20180214/21c97998/8193373-0001.patch>