RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Jim Laskey-3
We should never close the jimage since java threads can still be running after a hard exit().

-------------

Commit messages:
 - 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Changes: https://git.openjdk.java.net/jdk/pull/3304/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3304&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8166727
  Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3304.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3304/head:pull/3304

PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Ioi Lam-2
On Thu, 1 Apr 2021 11:48:21 GMT, Jim Laskey <[hidden email]> wrote:

> We should never close the jimage since java threads can still be running after a hard exit().

LGTM.

-------------

Marked as reviewed by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Alan Bateman-2
In reply to this post by Jim Laskey-3
On Thu, 1 Apr 2021 11:48:21 GMT, Jim Laskey <[hidden email]> wrote:

> We should never close the jimage since java threads can still be running after a hard exit().

src/java.base/share/native/libjimage/imageFile.cpp line 219:

> 217: // WARNING: Should never close the jimage file.
> 218: //          Threads may still be running at shutdown.
> 219: #if 0

Are you keeping the code in order to re-visit it again? Just wondering why it's not deleted.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Jim Laskey-3
On Thu, 1 Apr 2021 18:19:35 GMT, Alan Bateman <[hidden email]> wrote:

>> We should never close the jimage since java threads can still be running after a hard exit().
>
> src/java.base/share/native/libjimage/imageFile.cpp line 219:
>
>> 217: // WARNING: Should never close the jimage file.
>> 218: //          Threads may still be running at shutdown.
>> 219: #if 0
>
> Are you keeping the code in order to re-visit it again? Just wondering why it's not deleted.

Leaving the code as an example of what is required to close in case the topic gets revisited in the future and I get hit by a bus or dementia.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Alan Bateman-2
In reply to this post by Jim Laskey-3
On Thu, 1 Apr 2021 11:48:21 GMT, Jim Laskey <[hidden email]> wrote:

> We should never close the jimage since java threads can still be running after a hard exit().

Marked as reviewed by alanb (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Alan Bateman-2
In reply to this post by Jim Laskey-3
On Thu, 1 Apr 2021 18:48:15 GMT, Jim Laskey <[hidden email]> wrote:

>> src/java.base/share/native/libjimage/imageFile.cpp line 219:
>>
>>> 217: // WARNING: Should never close the jimage file.
>>> 218: //          Threads may still be running at shutdown.
>>> 219: #if 0
>>
>> Are you keeping the code in order to re-visit it again? Just wondering why it's not deleted.
>
> Leaving the code as an example of what is required to close in case the topic gets revisited in the future and I get hit by a bus or dementia.

Okay although I assume someone will spot this and be tempted to remove it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

David Holmes
On 2/04/2021 5:34 pm, Alan Bateman wrote:

> On Thu, 1 Apr 2021 18:48:15 GMT, Jim Laskey <[hidden email]> wrote:
>
>>> src/java.base/share/native/libjimage/imageFile.cpp line 219:
>>>
>>>> 217: // WARNING: Should never close the jimage file.
>>>> 218: //          Threads may still be running at shutdown.
>>>> 219: #if 0
>>>
>>> Are you keeping the code in order to re-visit it again? Just wondering why it's not deleted.
>>
>> Leaving the code as an example of what is required to close in case the topic gets revisited in the future and I get hit by a bus or dementia.
>
> Okay although I assume someone will spot this and be tempted to remove it.

I didn't comment on this as I assumed it would contravene core-libs
coding guidelines. I'm suprised to see dead code kept this way (there
are existing cases elsewhere but it isn't considered good form).
Previous code can always be retrieved from the repo history.

Cheers,
David

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3304
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Jim Laskey-2
Is it in bad form to declare a #define CLOSE_JIMAGE 0 ?

📱

> On Apr 3, 2021, at 9:49 AM, David Holmes <[hidden email]> wrote:
>
> On 2/04/2021 5:34 pm, Alan Bateman wrote:
>> On Thu, 1 Apr 2021 18:48:15 GMT, Jim Laskey <[hidden email]> wrote:
>>>> src/java.base/share/native/libjimage/imageFile.cpp line 219:
>>>>
>>>>> 217: // WARNING: Should never close the jimage file.
>>>>> 218: //          Threads may still be running at shutdown.
>>>>> 219: #if 0
>>>>
>>>> Are you keeping the code in order to re-visit it again? Just wondering why it's not deleted.
>>>
>>> Leaving the code as an example of what is required to close in case the topic gets revisited in the future and I get hit by a bus or dementia.
>> Okay although I assume someone will spot this and be tempted to remove it.
>
> I didn't comment on this as I assumed it would contravene core-libs coding guidelines. I'm suprised to see dead code kept this way (there are existing cases elsewhere but it isn't considered good form). Previous code can always be retrieved from the repo history.
>
> Cheers,
> David
>
>> -------------
>> PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

David Holmes
On 4/04/2021 12:02 am, Jim Laskey wrote:
> Is it in bad form to declare a #define CLOSE_JIMAGE 0 ?

Still dead code.

This is a core-libs style call anyway.

Cheers,
David

> 📱
>
>> On Apr 3, 2021, at 9:49 AM, David Holmes <[hidden email]> wrote:
>>
>> On 2/04/2021 5:34 pm, Alan Bateman wrote:
>>> On Thu, 1 Apr 2021 18:48:15 GMT, Jim Laskey <[hidden email]> wrote:
>>>>> src/java.base/share/native/libjimage/imageFile.cpp line 219:
>>>>>
>>>>>> 217: // WARNING: Should never close the jimage file.
>>>>>> 218: //          Threads may still be running at shutdown.
>>>>>> 219: #if 0
>>>>>
>>>>> Are you keeping the code in order to re-visit it again? Just wondering why it's not deleted.
>>>>
>>>> Leaving the code as an example of what is required to close in case the topic gets revisited in the future and I get hit by a bus or dementia.
>>> Okay although I assume someone will spot this and be tempted to remove it.
>>
>> I didn't comment on this as I assumed it would contravene core-libs coding guidelines. I'm suprised to see dead code kept this way (there are existing cases elsewhere but it isn't considered good form). Previous code can always be retrieved from the repo history.
>>
>> Cheers,
>> David
>>
>>> -------------
>>> PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Jim Laskey-2
I’ll remove the code and elucidate in the commentary.

📱

> On Apr 3, 2021, at 8:08 PM, David Holmes <[hidden email]> wrote:
>
> On 4/04/2021 12:02 am, Jim Laskey wrote:
>> Is it in bad form to declare a #define CLOSE_JIMAGE 0 ?
>
> Still dead code.
>
> This is a core-libs style call anyway.
>
> Cheers,
> David
>
>> 📱
>>>> On Apr 3, 2021, at 9:49 AM, David Holmes <[hidden email]> wrote:
>>>
>>> On 2/04/2021 5:34 pm, Alan Bateman wrote:
>>>> On Thu, 1 Apr 2021 18:48:15 GMT, Jim Laskey <[hidden email]> wrote:
>>>>>> src/java.base/share/native/libjimage/imageFile.cpp line 219:
>>>>>>
>>>>>>> 217: // WARNING: Should never close the jimage file.
>>>>>>> 218: //          Threads may still be running at shutdown.
>>>>>>> 219: #if 0
>>>>>>
>>>>>> Are you keeping the code in order to re-visit it again? Just wondering why it's not deleted.
>>>>>
>>>>> Leaving the code as an example of what is required to close in case the topic gets revisited in the future and I get hit by a bus or dementia.
>>>> Okay although I assume someone will spot this and be tempted to remove it.
>>>
>>> I didn't comment on this as I assumed it would contravene core-libs coding guidelines. I'm suprised to see dead code kept this way (there are existing cases elsewhere but it isn't considered good form). Previous code can always be retrieved from the repo history.
>>>
>>> Cheers,
>>> David
>>>
>>>> -------------
>>>> PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28 [v2]

Jim Laskey-3
In reply to this post by Jim Laskey-3
> We should never close the jimage since java threads can still be running after a hard exit().

Jim Laskey has updated the pull request incrementally with one additional commit since the last revision:

  Remove dead code

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3304/files
  - new: https://git.openjdk.java.net/jdk/pull/3304/files/515bba9d..3b6be82f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3304&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3304&range=00-01

  Stats: 23 lines in 2 files changed: 5 ins; 17 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3304.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3304/head:pull/3304

PR: https://git.openjdk.java.net/jdk/pull/3304
Reply | Threaded
Open this post in threaded view
|

Integrated: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Jim Laskey-3
In reply to this post by Jim Laskey-3
On Thu, 1 Apr 2021 11:48:21 GMT, Jim Laskey <[hidden email]> wrote:

> We should never close the jimage since java threads can still be running after a hard exit().

This pull request has now been integrated.

Changeset: a8005efd
Author:    Jim Laskey <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/a8005efd
Stats:     17 lines in 2 files changed: 5 ins; 11 del; 1 mod

8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28

Reviewed-by: iklam, alanb

-------------

PR: https://git.openjdk.java.net/jdk/pull/3304