[10] RFR(S) 8183262: noexecstack check in os::dll_load on Linux is too expensive

[Thomas Stüfe]

Hi Vladimir,

thanks for taking my suggestion. This looks ok.

The mixup of return values for file errors (true for fopen-failed?) is
weird, but that was weird before the patch. You could use access(3) to
determine if the shared object can be accessed before using
specifies_noexecstack(). But I leave this up to you, the fix is fine for me
as it is. No need for a new webrev.

Thanks, Thomas


On Tue, Jul 4, 2017 at 9:13 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com>
wrote:

> Here is updated changes which converted ElfFile::specifies_noexecstack()
> to static method.
>
> http://cr.openjdk.java.net/~kvn/8183262/webrev.01/
>
> RBT testing passed.
>
> AOT initialization time is still small - new code reads only first block
> from file.
>
> Thanks,
> Vladimir
>
>
> On 6/30/17 3:52 PM, Vladimir Kozlov wrote:
>
>> Thank you, Thomas
>>
>> I thought about that but choose to do this since I will have to duplicate
>> checks done in constructor.
>>
>> But starting looking on it I realized that my fix is incorrect because
>> load_tables() reads file header which is used by specifies_noexecstack().
>> It is mess - os::dll_load() also reads header later.
>>
>> Anyway, to make my life easy I will duplicate header read code in
>> specifies_noexecstack().
>>
>> I will send updated webrev after I test it.
>>
>> Thanks,
>> Vladimir
>>
>> On 6/30/17 12:44 PM, Thomas Stüfe wrote:
>>
>>> Hi Vladimir,
>>>
>>> This seems fine, but I am a bit concerned that this misuses an
>>> inconsistency in the API, namely the fact that specifies_noexecstack()
>>> behaves differently than all other functions (re-read the file each time
>>> it
>>> is called instead of using cached information). In order to understand
>>> the
>>> "load_tables" argument in the constructor, one has to understand that odd
>>> design, and know that load_tables=false means the only method which can
>>> be
>>> used is specifies_noexecstack().
>>>
>>> As a proposal, could you instead make specifies_noexecstack() a static
>>> function in ElfFile, such that:
>>>
>>> static bool ElfFile::specifies_noexecstack(const char* filename);
>>>
>>> Then it would be immediately clear that this function is to be used
>>> without
>>> constructing the ElfFile object.
>>>
>>> Kind Regards, Thomas
>>>
>>> On Fri, Jun 30, 2017 at 7:07 PM, Vladimir Kozlov <
>>> vladimir.kozlov at oracle.com
>>>
>>>> wrote:
>>>>
>>>
>>> webrev: http://cr.openjdk.java.net/~kvn/8183262/webrev/
>>>> https://bugs.openjdk.java.net/browse/JDK-8183262
>>>>
>>>> To load AOT library we use os::dll_load() method provided on all
>>>> platforms.
>>>>
>>>> During investigation why AOT library load takes so long on Linux I found
>>>> that it spend most of time in ElfFile::load_tables() method which looks
>>>> for
>>>> string tables in library. It is called from ElfFile() constructor used
>>>> in
>>>> os::dll_load() just to check if library is built with noexecstack:
>>>>
>>>>   if (os::uses_stack_guard_pages() && !os::Linux::_stack_is_executable)
>>>> {
>>>>     ElfFile ef(filename);
>>>>     if (!ef.specifies_noexecstack()) {
>>>>
>>>> There is no need for string tables load in such case. Avoiding loads
>>>> helps
>>>> a lot:
>>>>
>>>> without fix:
>>>>
>>>> [0.047s][info][aot,startuptime] AOT initialization, 0.0424919 secs
>>>>
>>>> with fix:
>>>>
>>>> [0.002s][info][aot,startuptime] AOT initialization, 0.0002087 secs
>>>>
>>>>
>>>> Note, JDK 9 is not affected because AOT uses system dlopen() since only
>>>> Linux is supported.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20170705/a54c6f9e/attachment.html>