RFR(M) 8193373: Cleanup ElfFile and family

[coleen.phillimore at oracle.com]

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