Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2588433002:
abort in delete operators that shouldn't be called (Closed)
Descriptionabort in delete operators that shouldn't be called
Section 3.2 of the C++ standard states that destructor definitions
implicitly "use" operator delete functions. Therefore, these operator
delete functions must be defined even if they are never called by
user code explicitly.
http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#261
gcc allows them to remain as empty definitions. However, not all
compilers allow this. (e.g. xlc on zOS)
This pull request creates definitions which if ever called, result
in an abort.
R=danno@chromium.org,jochen@chromium.org
BUG=
LOG=N
Review-Url: https://codereview.chromium.org/2588433002
Cr-Commit-Position: refs/heads/master@{#41981}
Committed: https://chromium.googlesource.com/v8/v8/+/224d3764e52fd4df91ece9274cee1305c3677286
Patch Set 1 #Patch Set 2 : addressed comments #Patch Set 3 : Add missing return types #Patch Set 4 : add return statements required by some compilers #Patch Set 5 : throw bad_alloc instead of returning null #Patch Set 6 : declare new operators as noreturn #Patch Set 7 : call abort to satisfy windows compiler #Messages
Total messages: 42 (30 generated)
jbarboza@ca.ibm.com changed reviewers: + bjaideep@ca.ibm.com, joransiu@ca.ibm.com, jyan@ca.ibm.com
Section 3.2 of the C++ standard states that destructor definitions implicitly "use" operator delete functions. Therefore, these operator delete functions must be defined even if they are never called by user code explicitly. http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#261 gcc allows them to remain as empty definitions. However, not all compilers allow this. (e.g. xlc on zOS) This pull request creates definitions which if ever called, result in an abort.
sorry, took me a while to locate all relevant sections in the spec. It's indeed undefined behavior to delete operator delete and operator new for classes that have dtors/ctors respectively. Instead of implementing them to invoke abort, I'd propose to just not delete them, does that sound ok? If yes, please also remove the deletions of operator new
> Instead of implementing them to invoke abort, I'd propose to just not delete > them, does that sound ok? This means that the compiler will generate implicit definitions for them. This means that the v8 consumer can new/delete HandleScope objects which we don't want. Instead, I think aborting makes sense. So if/when user does "delete <object of type HandleScope>", we abort at runtime and inform the user that they cannot do this. > If yes, please also remove the deletions of operator new If the abort suggestion makes sense, I can also do this for the new operators. Ins
ok, lgtm with the operator new also being implemented to abort
The CQ bit was checked by jbarboza@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2588433002/#ps20001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/18395) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
The CQ bit was checked by jbarboza@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2588433002/#ps40001 (title: "Add missing return types")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...) v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/20104)
The CQ bit was checked by jbarboza@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2588433002/#ps60001 (title: "add return statements required by some compilers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/18401) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/18373) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14817)
The CQ bit was checked by jbarboza@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/18468) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/14818)
The CQ bit was checked by jbarboza@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/18469) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
The CQ bit was checked by jbarboza@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/20029) v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
The CQ bit was checked by jbarboza@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jbarboza@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2588433002/#ps120001 (title: "call abort to satisfy windows compiler")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482949776478900,
"parent_rev": "734a7615671e4d256b0c0d14a85ed209dc298956", "commit_rev":
"224d3764e52fd4df91ece9274cee1305c3677286"}
Message was sent while issue was closed.
Description was changed from ========== abort in delete operators that shouldn't be called Section 3.2 of the C++ standard states that destructor definitions implicitly "use" operator delete functions. Therefore, these operator delete functions must be defined even if they are never called by user code explicitly. http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#261 gcc allows them to remain as empty definitions. However, not all compilers allow this. (e.g. xlc on zOS) This pull request creates definitions which if ever called, result in an abort. R=danno@chromium.org,jochen@chromium.org BUG= LOG=N ========== to ========== abort in delete operators that shouldn't be called Section 3.2 of the C++ standard states that destructor definitions implicitly "use" operator delete functions. Therefore, these operator delete functions must be defined even if they are never called by user code explicitly. http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#261 gcc allows them to remain as empty definitions. However, not all compilers allow this. (e.g. xlc on zOS) This pull request creates definitions which if ever called, result in an abort. R=danno@chromium.org,jochen@chromium.org BUG= LOG=N Review-Url: https://codereview.chromium.org/2588433002 Cr-Commit-Position: refs/heads/master@{#41981} Committed: https://chromium.googlesource.com/v8/v8/+/224d3764e52fd4df91ece9274cee1305c36... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/224d3764e52fd4df91ece9274cee1305c36... |
