RFR: 8260341: CDS dump VM init code does not check exceptions

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

RFR: 8260341: CDS dump VM init code does not check exceptions

Ioi Lam-2
When CDS dumping is enabled, special initialization happens during VM init. However, many of these calls do not properly check for exception. Instead, they rely on the implicit knowledge that `metaspace::allocate()` will exit the VM when allocation fails during CDS dumping. This makes the code hard to understand and tightly coupled to `metaspace::allocate()`.

The fix is: all code that makes allocation should be using CHECK macros, so each block of code can be individually understood without considering the behavior of `metaspace::allocate()`.

I added `TRAPS` to a bunch of CDS-related functions that are called during VM init. In some cases, I changed `Thread* THREAD` to `TRAPS`. This also eliminated a few `Thread* THREAD = Thread::current()` calls.

The "root" of these calls, such as `MetaspaceShared::prepare_for_dumping()`, now follows this pattern:

  EXCEPTION_MARK;
  ClassLoader::initialize_shared_path(THREAD);
  if (HAS_PENDING_EXCEPTION) {
    java_lang_Throwable::print(PENDING_EXCEPTION, tty);
    vm_exit_during_initialization("ClassLoader::initialize_shared_path() failed unexpectedly");
  }

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

Commit messages:
 - 8260341: CDS dump VM init code does not check exceptions

Changes: https://git.openjdk.java.net/jdk/pull/2494/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2494&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260341
  Stats: 97 lines in 12 files changed: 18 ins; 11 del; 68 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2494.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2494/head:pull/2494

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions

Harold Seigel-2
On Wed, 10 Feb 2021 05:00:43 GMT, Ioi Lam <[hidden email]> wrote:

> When CDS dumping is enabled, special initialization happens during VM init. However, many of these calls do not properly check for exception. Instead, they rely on the implicit knowledge that `metaspace::allocate()` will exit the VM when allocation fails during CDS dumping. This makes the code hard to understand and tightly coupled to `metaspace::allocate()`.
>
> The fix is: all code that makes allocation should be using CHECK macros, so each block of code can be individually understood without considering the behavior of `metaspace::allocate()`.
>
> I added `TRAPS` to a bunch of CDS-related functions that are called during VM init. In some cases, I changed `Thread* THREAD` to `TRAPS`. This also eliminated a few `Thread* THREAD = Thread::current()` calls.
>
> The "root" of these calls, such as `MetaspaceShared::prepare_for_dumping()`, now follows this pattern:
>
>   EXCEPTION_MARK;
>   ClassLoader::initialize_shared_path(THREAD);
>   if (HAS_PENDING_EXCEPTION) {
>     java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>     vm_exit_during_initialization("ClassLoader::initialize_shared_path() failed unexpectedly");
>   }

Hi Ioi,
Do you avoid using CHECK if it's the last line of a function?  For example, why is THREAD used instead of CHECK at line 1506?
Thanks, Harold

1503 void ClassLoader::initialize_module_path(TRAPS) {
1504   if (Arguments::is_dumping_archive()) {
1505     ClassLoaderExt::setup_module_paths(CHECK);
1506     FileMapInfo::allocate_shared_path_table(THREAD);
1507   }
1508 }

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

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions

Ioi Lam-2
On Wed, 10 Feb 2021 14:33:50 GMT, Harold Seigel <[hidden email]> wrote:

> Hi Ioi,
> Do you avoid using CHECK if it's the last line of a function? For example, why is THREAD used instead of CHECK at line 1506?
> Thanks, Harold
>
> 1503 void ClassLoader::initialize_module_path(TRAPS) {
> 1504 if (Arguments::is_dumping_archive()) {
> 1505 ClassLoaderExt::setup_module_paths(CHECK);
> 1506 FileMapInfo::allocate_shared_path_table(THREAD);
> 1507 }
> 1508 }

I thought it was a commonly used coding convention, but I could only find a few cases where the code wasn't written by me :-(

- https://github.com/openjdk/jdk/blame/b9d4211bc1aa92e257ddfe86c7a2b4e4e60598a0/src/hotspot/share/prims/jvm.cpp#L311
- https://github.com/openjdk/jdk/blame/f03e839e481f905358ce7d95a5d1f5179e7f46fe/src/hotspot/share/classfile/javaClasses.cpp#L2415

I will go back to `CHECK`, since the C++ compiler will elide the last `CHECK` anyway: in both cases, gcc compiles the last call to a direct branch to FileMapInfo::allocate_shared_path_table (i.e., a tail call).

Using `CHECK` makes the code easier to maintain (if you add new code below it).

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

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> When CDS dumping is enabled, special initialization happens during VM init. However, many of these calls do not properly check for exception. Instead, they rely on the implicit knowledge that `metaspace::allocate()` will exit the VM when allocation fails during CDS dumping. This makes the code hard to understand and tightly coupled to `metaspace::allocate()`.
>
> The fix is: all code that makes allocation should be using CHECK macros, so each block of code can be individually understood without considering the behavior of `metaspace::allocate()`.
>
> I added `TRAPS` to a bunch of CDS-related functions that are called during VM init. In some cases, I changed `Thread* THREAD` to `TRAPS`. This also eliminated a few `Thread* THREAD = Thread::current()` calls.
>
> The "root" of these calls, such as `MetaspaceShared::prepare_for_dumping()`, now follows this pattern:
>
>   EXCEPTION_MARK;
>   ClassLoader::initialize_shared_path(THREAD);
>   if (HAS_PENDING_EXCEPTION) {
>     java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>     vm_exit_during_initialization("ClassLoader::initialize_shared_path() failed unexpectedly");
>   }

Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:

  Changed THREAD in tail calls to CHECK

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2494/files
  - new: https://git.openjdk.java.net/jdk/pull/2494/files/54c251c8..218af58f

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

  Stats: 14 lines in 3 files changed: 1 ins; 0 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2494.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2494/head:pull/2494

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]

Coleen Phillimore-3
On Wed, 10 Feb 2021 18:51:53 GMT, Ioi Lam <[hidden email]> wrote:

>> When CDS dumping is enabled, special initialization happens during VM init. However, many of these calls do not properly check for exception. Instead, they rely on the implicit knowledge that `metaspace::allocate()` will exit the VM when allocation fails during CDS dumping. This makes the code hard to understand and tightly coupled to `metaspace::allocate()`.
>>
>> The fix is: all code that makes allocation should be using CHECK macros, so each block of code can be individually understood without considering the behavior of `metaspace::allocate()`.
>>
>> I added `TRAPS` to a bunch of CDS-related functions that are called during VM init. In some cases, I changed `Thread* THREAD` to `TRAPS`. This also eliminated a few `Thread* THREAD = Thread::current()` calls.
>>
>> The "root" of these calls, such as `MetaspaceShared::prepare_for_dumping()`, now follows this pattern:
>>
>>   EXCEPTION_MARK;
>>   ClassLoader::initialize_shared_path(THREAD);
>>   if (HAS_PENDING_EXCEPTION) {
>>     java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>>     vm_exit_during_initialization("ClassLoader::initialize_shared_path() failed unexpectedly");
>>   }
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   Changed THREAD in tail calls to CHECK

What Harold said. We use THREAD if the call is the last call in the function because I thought there was a tail call problem with one of the compilers once.  I think this was the bug I was thinking about and it's only in the return statement:
https://bugs.openjdk.java.net/browse/JDK-6889002
If you verified that the HAS_PENDING_EXCEPTION check evaporates, that's fine then, and better to have CHECK.  As you say, someone might add some code after it.

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

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]

Harold Seigel-2
In reply to this post by Ioi Lam-2
On Wed, 10 Feb 2021 18:51:53 GMT, Ioi Lam <[hidden email]> wrote:

>> When CDS dumping is enabled, special initialization happens during VM init. However, many of these calls do not properly check for exception. Instead, they rely on the implicit knowledge that `metaspace::allocate()` will exit the VM when allocation fails during CDS dumping. This makes the code hard to understand and tightly coupled to `metaspace::allocate()`.
>>
>> The fix is: all code that makes allocation should be using CHECK macros, so each block of code can be individually understood without considering the behavior of `metaspace::allocate()`.
>>
>> I added `TRAPS` to a bunch of CDS-related functions that are called during VM init. In some cases, I changed `Thread* THREAD` to `TRAPS`. This also eliminated a few `Thread* THREAD = Thread::current()` calls.
>>
>> The "root" of these calls, such as `MetaspaceShared::prepare_for_dumping()`, now follows this pattern:
>>
>>   EXCEPTION_MARK;
>>   ClassLoader::initialize_shared_path(THREAD);
>>   if (HAS_PENDING_EXCEPTION) {
>>     java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>>     vm_exit_during_initialization("ClassLoader::initialize_shared_path() failed unexpectedly");
>>   }
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>
>   Changed THREAD in tail calls to CHECK

The changes look good.  Thanks!
Harold

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

Marked as reviewed by hseigel (Reviewer).

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Wed, 10 Feb 2021 21:11:12 GMT, Coleen Phillimore <[hidden email]> wrote:

> What Harold said. We use THREAD if the call is the last call in the function because I thought there was a tail call problem with one of the compilers once. I think this was the bug I was thinking about and it's only in the return statement:
> https://bugs.openjdk.java.net/browse/JDK-6889002
> If you verified that the HAS_PENDING_EXCEPTION check evaporates, that's fine then, and better to have CHECK. As you say, someone might add some code after it.

I think the problem in JDK-6889002 is only with using `CHECK` in a return statement like this that produces unreachable source code.

    return foobar(1, 2, 3, CHECK_0);

->

    return foobar(1, 2, 3, THREAD);
    if (HAS_PENDING_EXCEPTION) return 0;

The cases that I have changed in this PR are functions with `void` returns.

void f(TRAPS)  {
    g(1, 2, 3, CHECK);
}

would be expanded to

void f(TRAPS)  {
    g(1, 2, 3, THREAD);
    if (HAS_PENDING_EXCEPTION) return;
}

I suppose any C++ compiler that can compile HotSpot will elide the `if` line.

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

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v3]

Ioi Lam-2
In reply to this post by Ioi Lam-2
> When CDS dumping is enabled, special initialization happens during VM init. However, many of these calls do not properly check for exception. Instead, they rely on the implicit knowledge that `metaspace::allocate()` will exit the VM when allocation fails during CDS dumping. This makes the code hard to understand and tightly coupled to `metaspace::allocate()`.
>
> The fix is: all code that makes allocation should be using CHECK macros, so each block of code can be individually understood without considering the behavior of `metaspace::allocate()`.
>
> I added `TRAPS` to a bunch of CDS-related functions that are called during VM init. In some cases, I changed `Thread* THREAD` to `TRAPS`. This also eliminated a few `Thread* THREAD = Thread::current()` calls.
>
> The "root" of these calls, such as `MetaspaceShared::prepare_for_dumping()`, now follows this pattern:
>
>   EXCEPTION_MARK;
>   ClassLoader::initialize_shared_path(THREAD);
>   if (HAS_PENDING_EXCEPTION) {
>     java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>     vm_exit_during_initialization("ClassLoader::initialize_shared_path() failed unexpectedly");
>   }

Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:

 - Merge branch 'master' into 8260341-cds-dump-vm-init-does-not-check-exceptions
 - Changed THREAD in tail calls to CHECK
 - 8260341: CDS dump VM init code does not check exceptions

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2494/files
  - new: https://git.openjdk.java.net/jdk/pull/2494/files/218af58f..dd3c7e08

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

  Stats: 1017 lines in 43 files changed: 425 ins; 347 del; 245 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2494.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2494/head:pull/2494

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]

Ioi Lam-2
In reply to this post by Harold Seigel-2
On Wed, 10 Feb 2021 21:14:25 GMT, Harold Seigel <[hidden email]> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Changed THREAD in tail calls to CHECK
>
> The changes look good.  Thanks!
> Harold

Thanks @hseigel and @coleenp for the review!

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

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

Integrated: 8260341: CDS dump VM init code does not check exceptions

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Wed, 10 Feb 2021 05:00:43 GMT, Ioi Lam <[hidden email]> wrote:

> When CDS dumping is enabled, special initialization happens during VM init. However, many of these calls do not properly check for exception. Instead, they rely on the implicit knowledge that `metaspace::allocate()` will exit the VM when allocation fails during CDS dumping. This makes the code hard to understand and tightly coupled to `metaspace::allocate()`.
>
> The fix is: all code that makes allocation should be using CHECK macros, so each block of code can be individually understood without considering the behavior of `metaspace::allocate()`.
>
> I added `TRAPS` to a bunch of CDS-related functions that are called during VM init. In some cases, I changed `Thread* THREAD` to `TRAPS`. This also eliminated a few `Thread* THREAD = Thread::current()` calls.
>
> The "root" of these calls, such as `MetaspaceShared::prepare_for_dumping()`, now follows this pattern:
>
>   EXCEPTION_MARK;
>   ClassLoader::initialize_shared_path(THREAD);
>   if (HAS_PENDING_EXCEPTION) {
>     java_lang_Throwable::print(PENDING_EXCEPTION, tty);
>     vm_exit_during_initialization("ClassLoader::initialize_shared_path() failed unexpectedly");
>   }

This pull request has now been integrated.

Changeset: adca84cc
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/adca84cc
Stats:     104 lines in 12 files changed: 19 ins; 11 del; 74 mod

8260341: CDS dump VM init code does not check exceptions

Reviewed-by: coleenp, hseigel

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

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]

Thomas Stuefe
In reply to this post by Ioi Lam-2
On Thu, 11 Feb 2021 05:11:25 GMT, Ioi Lam <[hidden email]> wrote:

>> The changes look good.  Thanks!
>> Harold
>
> Thanks @hseigel and @coleenp for the review!

Could we then get rid of the special CDS handling in `Metaspace::allocate()`:
https://github.com/openjdk/jdk/blob/837bd8930d0a010110f1318b947c036609d3aa33/src/hotspot/share/memory/metaspace.cpp#L825-L836
?

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

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]

Ioi Lam-2
On Thu, 11 Feb 2021 05:34:35 GMT, Thomas Stuefe <[hidden email]> wrote:

> Could we then get rid of the special CDS handling in `Metaspace::allocate()`:
> https://github.com/openjdk/jdk/blob/837bd8930d0a010110f1318b947c036609d3aa33/src/hotspot/share/memory/metaspace.cpp#L825-L836
>

I filed https://bugs.openjdk.java.net/browse/JDK-8261551 . We need to fix JDK-8261480 first, since we still have some inappropriate use of THREAD during dumping. I split the work into several steps so that the review can be easier.

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

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]

Thomas Stuefe
On Thu, 11 Feb 2021 05:47:12 GMT, Ioi Lam <[hidden email]> wrote:

> > Could we then get rid of the special CDS handling in `Metaspace::allocate()`:
> > https://github.com/openjdk/jdk/blob/837bd8930d0a010110f1318b947c036609d3aa33/src/hotspot/share/memory/metaspace.cpp#L825-L836
>
> I filed https://bugs.openjdk.java.net/browse/JDK-8261551 . We need to fix JDK-8261480 first, since we still have some inappropriate use of THREAD during dumping. I split the work into several steps so that the review can be easier.

Okay, thanks.

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

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

Re: RFR: 8260341: CDS dump VM init code does not check exceptions

David Holmes
In reply to this post by Ioi Lam-2
On 11/02/2021 4:13 am, Ioi Lam wrote:

> On Wed, 10 Feb 2021 14:33:50 GMT, Harold Seigel <[hidden email]> wrote:
>
>> Hi Ioi,
>> Do you avoid using CHECK if it's the last line of a function? For example, why is THREAD used instead of CHECK at line 1506?
>> Thanks, Harold
>>
>> 1503 void ClassLoader::initialize_module_path(TRAPS) {
>> 1504 if (Arguments::is_dumping_archive()) {
>> 1505 ClassLoaderExt::setup_module_paths(CHECK);
>> 1506 FileMapInfo::allocate_shared_path_table(THREAD);
>> 1507 }
>> 1508 }
>
> I thought it was a commonly used coding convention, but I could only find a few cases where the code wasn't written by me :-(

It is something we have been gradually fixing. I'm made a number of
requests to remove CHECK from code that will return anyway.

> - https://github.com/openjdk/jdk/blame/b9d4211bc1aa92e257ddfe86c7a2b4e4e60598a0/src/hotspot/share/prims/jvm.cpp#L311
> - https://github.com/openjdk/jdk/blame/f03e839e481f905358ce7d95a5d1f5179e7f46fe/src/hotspot/share/classfile/javaClasses.cpp#L2415
>
> I will go back to `CHECK`, since the C++ compiler will elide the last `CHECK` anyway: in both cases, gcc compiles the last call to a direct branch to FileMapInfo::allocate_shared_path_table (i.e., a tail call).

Do all our C++ compilers do this? If they do that is great, but if they
only may perhaps sometimes do then not so great.

> Using `CHECK` makes the code easier to maintain (if you add new code below it).

I prefer not to rely on the C++ compiler to remove the exception
checking logic and use THREAD in such cases.

David
-----

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

CHECK at the end of a void function

Ioi Lam
Converting this from a PR discussion
(https://git.openjdk.java.net/jdk/pull/2494) to a regular mail.

What are people's opinion of:

     void bar(TRAPS);

     void foo(TRAPS) {
        bar(CHECK);
     }

vs

     void foo(TRAPS) {
        bar(THREAD);
     }

There's no mention of this in
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md

Advantage of CHECK:

- More readable -- you don't need to ask yourself:
   does the callee need a THREAD, or is the callee a TRAPS function?

- More maintainable. You don't accidentally miss a check if you add new
   code below

     void foo(TRAPS) {
        bar(THREAD);
        baz();       // adding a new call ....
     }

Note that we MUST use THREAD when returning a value (see
https://bugs.openjdk.java.net/browse/JDK-6889002)

     int x(TRAPS);

     int y(TRAPS) {
        return x(THREAD);
     }

so there's inconsistency. However, the compiler will given an error if
you add code below the THREAD. So we don't have the maintenance issue as
void functions:

     int Y(TRAPS) {
        return X(THREAD);
        baz();
     }

Disadvantage of CHECK:

- It's not guaranteed that the C compiler will elide it. The code gets
pre-processed to

     inlined bool ThreadShadow::has_pending_exception() const {
         return _pending_exception != NULL;
     }

     void foo(Thread*  __the_thread__) {
         bar(__the_thread__);
         if (((ThreadShadow*)__the_thread__)->has_pending_exception())
return;
     }

Is it safe to assume that any C compiler that can efficiently compile
HotSpot will always elide the "if" line?

I am a little worried about the maintenance issue. If we really want to
avoid the CHECK, I would prefer to have a new macro like:

     void foo(TRAPS) {
        bar(CHECK_AT_RETURN);
     }

which will be preprocessed to

     void foo(....) {
        bar(_thread__); return;
     }

So you can't accidentally add code below it.

Thanks
  -Ioi

Reply | Threaded
Open this post in threaded view
|

Re: CHECK at the end of a void function

David Holmes
Hi Ioi,

 > CHECK at the end of a void function

This isn't really about void functions, nor check at the end. It is
about using CHECK/CHECK_* on a call that is immediately followed by a
return from the current function as it degenerates to:

if (EXCEPTION_OCCURRED)
   return;
else
   return;

On 18/02/2021 8:16 am, Ioi Lam wrote:
> Converting this from a PR discussion
> (https://git.openjdk.java.net/jdk/pull/2494) to a regular mail.

Thanks for doing that.

> What are people's opinion of:
>
>      void bar(TRAPS);
>
>      void foo(TRAPS) {
>         bar(CHECK);
>      }
>
> vs
>
>      void foo(TRAPS) {
>         bar(THREAD);
>      }
>
> There's no mention of this in
> https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
>
> Advantage of CHECK:
>
> - More readable -- you don't need to ask yourself:
>    does the callee need a THREAD, or is the callee a TRAPS function?

But you also don't need to ask yourself that because it doesn't matter
when the next action is to return anyway.

> - More maintainable. You don't accidentally miss a check if you add new
>    code below
>
>      void foo(TRAPS) {
>         bar(THREAD);
>         baz();       // adding a new call ....
>      }

True but I would argue that you need to think about the behaviour of bar
when adding baz() regardless. This might be wrong:

bar(CHECK);
baz(); // <= critical code that must always be executed no matter what!

> Note that we MUST use THREAD when returning a value (see
> https://bugs.openjdk.java.net/browse/JDK-6889002)
>
>      int x(TRAPS);
>
>      int y(TRAPS) {
>         return x(THREAD);
>      }

I think this is an anti-pattern and we should prefer the more explicit:

RetType ret = x(CHECK_*);
return ret;

if we want to emphasize use of CHECK.

> so there's inconsistency. However, the compiler will given an error if
> you add code below the THREAD. So we don't have the maintenance issue as
> void functions:
>
>      int Y(TRAPS) {
>         return X(THREAD);
>         baz();
>      }

Don't quite follow that as you wouldn't write anything after a return
statement anyway.

> Disadvantage of CHECK:
>
> - It's not guaranteed that the C compiler will elide it. The code gets
> pre-processed to
>
>      inlined bool ThreadShadow::has_pending_exception() const {
>          return _pending_exception != NULL;
>      }
>
>      void foo(Thread*  __the_thread__) {
>          bar(__the_thread__);
>          if (((ThreadShadow*)__the_thread__)->has_pending_exception())
> return;
>      }
>
> Is it safe to assume that any C compiler that can efficiently compile
> HotSpot will always elide the "if" line?

I've no idea, but if we can rely on it then I'm okay with always using
CHECK. It was the redundant code execution that was my concern.

> I am a little worried about the maintenance issue. If we really want to
> avoid the CHECK, I would prefer to have a new macro like:
>
>      void foo(TRAPS) {
>         bar(CHECK_AT_RETURN);
>      }
>
> which will be preprocessed to
>
>      void foo(....) {
>         bar(_thread__); return;
>      }
>
> So you can't accidentally add code below it.

I agree that a new macro might be preferable than just using THREAD.
Coming up with a short but meaningful name will be a problem. :) The
point is we are not actually checking anything in this case.

Thanks,
David

> Thanks
>   -Ioi
>
Reply | Threaded
Open this post in threaded view
|

Re: CHECK at the end of a void function

Coleen Phillimore-2

My preference is to keep THREAD as an argument if you were going to use
CHECK for the last statement of a function.  You have to do this if
you're returning with a function that takes TRAPS. ie:
   return my_fun(THREAD);

For the most part, I don't think it matters if the compiler can optimize
it away.  Do our compilers optimize away the extra check for pending
exception?  I don't know if you answered this.

Lastly, please no, I don't want to see yet another macro for this
special case.

Coleen

On 2/17/21 8:55 PM, David Holmes wrote:

> Hi Ioi,
>
> > CHECK at the end of a void function
>
> This isn't really about void functions, nor check at the end. It is
> about using CHECK/CHECK_* on a call that is immediately followed by a
> return from the current function as it degenerates to:
>
> if (EXCEPTION_OCCURRED)
>   return;
> else
>   return;
>
> On 18/02/2021 8:16 am, Ioi Lam wrote:
>> Converting this from a PR discussion
>> (https://git.openjdk.java.net/jdk/pull/2494) to a regular mail.
>
> Thanks for doing that.
>
>> What are people's opinion of:
>>
>>      void bar(TRAPS);
>>
>>      void foo(TRAPS) {
>>         bar(CHECK);
>>      }
>>
>> vs
>>
>>      void foo(TRAPS) {
>>         bar(THREAD);
>>      }
>>
>> There's no mention of this in
>> https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
>>
>> Advantage of CHECK:
>>
>> - More readable -- you don't need to ask yourself:
>>    does the callee need a THREAD, or is the callee a TRAPS function?
>
> But you also don't need to ask yourself that because it doesn't matter
> when the next action is to return anyway.
>
>> - More maintainable. You don't accidentally miss a check if you add new
>>    code below
>>
>>      void foo(TRAPS) {
>>         bar(THREAD);
>>         baz();       // adding a new call ....
>>      }
>
> True but I would argue that you need to think about the behaviour of
> bar when adding baz() regardless. This might be wrong:
>
> bar(CHECK);
> baz(); // <= critical code that must always be executed no matter what!
>
>> Note that we MUST use THREAD when returning a value (see
>> https://bugs.openjdk.java.net/browse/JDK-6889002)
>>
>>      int x(TRAPS);
>>
>>      int y(TRAPS) {
>>         return x(THREAD);
>>      }
>
> I think this is an anti-pattern and we should prefer the more explicit:
>
> RetType ret = x(CHECK_*);
> return ret;
>
> if we want to emphasize use of CHECK.
>
>> so there's inconsistency. However, the compiler will given an error
>> if you add code below the THREAD. So we don't have the maintenance
>> issue as void functions:
>>
>>      int Y(TRAPS) {
>>         return X(THREAD);
>>         baz();
>>      }
>
> Don't quite follow that as you wouldn't write anything after a return
> statement anyway.
>
>> Disadvantage of CHECK:
>>
>> - It's not guaranteed that the C compiler will elide it. The code
>> gets pre-processed to
>>
>>      inlined bool ThreadShadow::has_pending_exception() const {
>>          return _pending_exception != NULL;
>>      }
>>
>>      void foo(Thread*  __the_thread__) {
>>          bar(__the_thread__);
>>          if
>> (((ThreadShadow*)__the_thread__)->has_pending_exception()) return;
>>      }
>>
>> Is it safe to assume that any C compiler that can efficiently compile
>> HotSpot will always elide the "if" line?
>
> I've no idea, but if we can rely on it then I'm okay with always using
> CHECK. It was the redundant code execution that was my concern.
>
>> I am a little worried about the maintenance issue. If we really want
>> to avoid the CHECK, I would prefer to have a new macro like:
>>
>>      void foo(TRAPS) {
>>         bar(CHECK_AT_RETURN);
>>      }
>>
>> which will be preprocessed to
>>
>>      void foo(....) {
>>         bar(_thread__); return;
>>      }
>>
>> So you can't accidentally add code below it.
>
> I agree that a new macro might be preferable than just using THREAD.
> Coming up with a short but meaningful name will be a problem. :) The
> point is we are not actually checking anything in this case.
>
> Thanks,
> David
>
>> Thanks
>>   -Ioi
>>