RFR: 8260341: CDS dump VM init code does not check exceptions
Locked
 
|
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 |
Re: RFR: 8260341: CDS dump VM init code does not check exceptions
|
|
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 |
|
|
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 |
|
|
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 |
Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]
|
|
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 |
Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]
|
|
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 |
|
|
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 |
|
|
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 |
|
|
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 |
|
|
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 |
Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]
|
|
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 |
|
|
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 |
Re: RFR: 8260341: CDS dump VM init code does not check exceptions [v2]
|
|
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 |
Re: RFR: 8260341: CDS dump VM init code does not check exceptions
|
|
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 > |
|
|
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 |
Re: CHECK at the end of a void function
|
|
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 > |
Re: CHECK at the end of a void function
|
|
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 >> |
«
Return to OpenJDK Hotspot Virtual Machine
|
1 view|%1 views
| Free forum by Nabble | Edit this page |